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
2 participants