Skip to content
This repository has been archived by the owner on Oct 22, 2023. It is now read-only.

Security review TODO #904

Draft
wants to merge 1 commit into
base: to-review-base
Choose a base branch
from
Draft

Security review TODO #904

wants to merge 1 commit into from

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Sep 23, 2019

No description provided.

Cargo.toml Show resolved Hide resolved
let hash = Hash::digest_bytes(&buffer);

let mut nonce = [0u8; NONCE_SIZE];
nonce[..NONCE_TAG_SIZE].copy_from_slice(&hash.as_ref()[..NONCE_TAG_SIZE]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: NONCE_TAG_SIZE < NONCE_SIZE

fn get(&self, key: Vec<u8>) -> Fallible<Vec<u8>> {
self.0
.lock()
.unwrap()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: if lock doesn't block and is about to unwrap a failure, something has gone horribly wrong, so panicking is an okay strategy

@@ -0,0 +1,27 @@
-----BEGIN RSA PRIVATE KEY-----
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: some security audit tools will complain about private keys checked into the repo, which necessitates a filter branch and rewriting history.

reference

.map(move |blk| {
let mut polls = polls.lock();
// +1, since we don't want to include the current block.
let id = polls.create_poll(PollFilter::Block(blk.number_u64() + 1));
Copy link
Contributor Author

@nhynes nhynes Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TODO(nhynes) does translator ever return u64::max_value()?

update: it might, but that would imply consensus failure, which is already catastrophic

},
extra_info: {
lazy_static! {
// Dummy PoW-related block extras.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this a lazy_static!?

})
.and_then(|logs: Vec<LocalizedLogEntry>| {
let mut logs = logs;
logs.sort_by(|a, b| a.block_number.partial_cmp(&b.block_number).unwrap());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logs.sort_by(|a, b| a.block_number.partial_cmp(&b.block_number).unwrap());
logs.sort_by(|a, b| a.block_number.cmp(&b.block_number));

id: BlockId,
) -> impl Future<Item = U256, Error = CallError> {
self.simulate_transaction(transaction, id)
.map(|executed| executed.gas_used + executed.refunded)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't overflow because gas_used + refunded <= U256::max_value() else parity is buggy

}

// Check whether transaction fits in the block.
let gas_remaining = U256::from(BLOCK_GAS_LIMIT) - ectx.env_info.gas_used;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ectx.env_info.gas_used should not exceed BLOCK_GAS_LIMIT since no tx will be added if it wouldn't fit (see next line)

src/methods.rs Show resolved Hide resolved
src/methods.rs Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants