From b06108cc1f421dcee8f39497685919e291f41432 Mon Sep 17 00:00:00 2001 From: cheme Date: Mon, 11 Feb 2019 16:52:11 +0100 Subject: [PATCH 1/2] Tag sensible place (ECHECH) --- ethcore/evm/src/interpreter/mod.rs | 2 +- ethcore/src/executive.rs | 18 +++++-- ethcore/src/state/account.rs | 4 +- ethcore/src/state/mod.rs | 2 +- miner/src/pool/queue.rs | 2 +- miner/src/pool/ready.rs | 4 +- miner/src/pool/verifier.rs | 3 +- rpc/src/v1/tests/mocked/signing_unsafe.rs | 59 +++++++++++++++++++++++ 8 files changed, 81 insertions(+), 13 deletions(-) diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index bd47c7ed713..7556e0315d8 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -574,7 +574,7 @@ impl Interpreter { let call_gas = call_gas + value.map_or_else(|| Cost::from(0), |val| match val.is_zero() { false => Cost::from(ext.schedule().call_stipend), true => Cost::from(0), - }); + }); // ECHECH costless saturating add // Get sender & receive addresses, check if we have balance let (sender_address, receive_address, has_balance, call_type) = match instruction { diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 97758994a17..100aff4d7c3 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -312,7 +312,7 @@ impl<'a> CallCreateExecutive<'a> { let prev_bal = state.balance(¶ms.address)?; if let ActionValue::Transfer(val) = params.value { state.sub_balance(¶ms.sender, &val, &mut substate.to_cleanup_mode(&schedule))?; - state.new_contract(¶ms.address, val + prev_bal, nonce_offset)?; + state.new_contract(¶ms.address, val + prev_bal, nonce_offset)?; /// ECHECH } else { state.new_contract(¶ms.address, prev_bal, nonce_offset)?; } @@ -1103,9 +1103,9 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let refunded = cmp::min(refunds_bound, (t.gas - gas_left_prerefund) >> 1); let gas_left = gas_left_prerefund + refunded; - let gas_used = t.gas - gas_left; - let refund_value = gas_left * t.gas_price; - let fees_value = gas_used * t.gas_price; + let gas_used = t.gas - gas_left; // ECHECH + let refund_value = gas_left * t.gas_price; // ECHECH obvious overflow here at left close toh h256 max and price same + let fees_value = gas_used * t.gas_price; // ECHECH overflow when close to H256 trace!("exec::finalize: t.gas={}, sstore_refunds={}, suicide_refunds={}, refunds_bound={}, gas_left_prerefund={}, refunded={}, gas_left={}, gas_used={}, refund_value={}, fees_value={}\n", t.gas, sstore_refunds, suicide_refunds, refunds_bound, gas_left_prerefund, refunded, gas_left, gas_used, refund_value, fees_value); @@ -1123,7 +1123,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { } // perform garbage-collection - let min_balance = if schedule.kill_dust != CleanDustMode::Off { Some(U256::from(schedule.tx_gas) * t.gas_price) } else { None }; + let min_balance = if schedule.kill_dust != CleanDustMode::Off { Some(U256::from(schedule.tx_gas) * t.gas_price) } else { None }; // ECHECH overflow next (as gas price from tx self.state.kill_garbage(&substate.touched, schedule.kill_empty, &min_balance, schedule.kill_dust == CleanDustMode::WithCodeAndStorage)?; match result { @@ -2178,3 +2178,11 @@ mod tests { assert_eq!(output[..], [0u8; 20][..]); } } + +#[test] +fn assert_no_overflow() { + let gas_price = U256::from(&vec![255u8;32][..]); + let gas = U256::from(&vec![255u8;32][..]); + let gas_cost = gas.full_mul(gas_price); + let total_cost = U512::from(U256::from(&vec![255u8;32][..])) + gas_cost; +} diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index e4553b906f3..49de58aca51 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -466,12 +466,12 @@ impl Account { /// Increment the nonce of the account by one. pub fn inc_nonce(&mut self) { - self.nonce = self.nonce + U256::from(1u8); + self.nonce = self.nonce + U256::from(1u8); // ECHECH overflow check here for case of nonce set by raw tx at U256.max } /// Increase account balance. pub fn add_balance(&mut self, x: &U256) { - self.balance = self.balance + *x; + self.balance = self.balance + *x; // ECHECH overflow check even if not needed here } /// Decrease account balance. diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 4d24a1b1573..74b36f9422f 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -500,7 +500,7 @@ impl State { /// it will have its code reset, ready for `init_code()`. pub fn new_contract(&mut self, contract: &Address, balance: U256, nonce_offset: U256) -> TrieResult<()> { let original_storage_root = self.original_storage_root(contract)?; - self.insert_cache(contract, AccountEntry::new_dirty(Some(Account::new_contract(balance, self.account_start_nonce + nonce_offset, original_storage_root)))); + self.insert_cache(contract, AccountEntry::new_dirty(Some(Account::new_contract(balance, self.account_start_nonce + nonce_offset, original_storage_root)))); // ECHECH Ok(()) } diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index 22278c3e858..0f91e1fa6a7 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -474,7 +474,7 @@ impl TransactionQueue { self.pool.read().pending_from_sender(state_readiness, address) .last() - .map(|tx| tx.signed().nonce + 1) + .map(|tx| tx.signed().nonce + 1) // ECHECH this seems risky } /// Retrieve a transaction from the pool. diff --git a/miner/src/pool/ready.rs b/miner/src/pool/ready.rs index 20507186e7a..c0b4e3ee0cd 100644 --- a/miner/src/pool/ready.rs +++ b/miner/src/pool/ready.rs @@ -95,7 +95,7 @@ impl txpool::Ready for State { }, cmp::Ordering::Less => txpool::Readiness::Stale, cmp::Ordering::Equal => { - *nonce = *nonce + 1; + *nonce = *nonce + 1; // ECHECH txpool::Readiness::Ready }, } @@ -159,7 +159,7 @@ impl Option> txpool::Ready for Opt cmp::Ordering::Greater => txpool::Readiness::Future, cmp::Ordering::Less => txpool::Readiness::Stale, cmp::Ordering::Equal => { - *nonce = *nonce + 1; + *nonce = *nonce + 1; // ECHECH terminal nonce ignore? txpool::Readiness::Ready }, } diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index 65835230d8e..cd3557e5d76 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -283,7 +283,8 @@ impl txpool::Verifier for Verifier Date: Mon, 11 Feb 2019 18:45:57 +0100 Subject: [PATCH 2/2] Additional overflows checks. --- ethcore/evm/src/interpreter/mod.rs | 4 +- ethcore/light/src/transaction_queue.rs | 4 +- ethcore/src/executive.rs | 22 ++++----- ethcore/src/state/account.rs | 4 +- ethcore/src/state/mod.rs | 7 ++- miner/src/pool/queue.rs | 2 +- miner/src/pool/ready.rs | 4 +- miner/src/pool/verifier.rs | 15 +++++- rpc/src/v1/tests/mocked/signing_unsafe.rs | 59 ----------------------- 9 files changed, 37 insertions(+), 84 deletions(-) diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index 7556e0315d8..d699e61cbec 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -571,10 +571,10 @@ impl Interpreter { let out_size = self.stack.pop_back(); // Add stipend (only CALL|CALLCODE when value > 0) - let call_gas = call_gas + value.map_or_else(|| Cost::from(0), |val| match val.is_zero() { + let call_gas = call_gas.overflow_add(value.map_or_else(|| Cost::from(0), |val| match val.is_zero() { false => Cost::from(ext.schedule().call_stipend), true => Cost::from(0), - }); // ECHECH costless saturating add + })).0; // Get sender & receive addresses, check if we have balance let (sender_address, receive_address, has_balance, call_type) = match instruction { diff --git a/ethcore/light/src/transaction_queue.rs b/ethcore/light/src/transaction_queue.rs index 53359368da9..65e646d8466 100644 --- a/ethcore/light/src/transaction_queue.rs +++ b/ethcore/light/src/transaction_queue.rs @@ -95,7 +95,7 @@ impl AccountTransactions { } fn next_nonce(&self) -> U256 { - self.current.last().map(|last| last.nonce + 1) + self.current.last().map(|last| last.nonce.saturating_add(1.into())) .unwrap_or_else(|| *self.cur_nonce.value()) } @@ -107,7 +107,7 @@ impl AccountTransactions { while let Some(tx) = self.future.remove(&next_nonce) { promoted.push(tx.hash); self.current.push(tx); - next_nonce = next_nonce + 1; + next_nonce = next_nonce.saturating_add(1.into()); } promoted diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 100aff4d7c3..68d4edc7948 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -312,7 +312,7 @@ impl<'a> CallCreateExecutive<'a> { let prev_bal = state.balance(¶ms.address)?; if let ActionValue::Transfer(val) = params.value { state.sub_balance(¶ms.sender, &val, &mut substate.to_cleanup_mode(&schedule))?; - state.new_contract(¶ms.address, val + prev_bal, nonce_offset)?; /// ECHECH + state.new_contract(¶ms.address, val.saturating_add(prev_bal), nonce_offset)?; } else { state.new_contract(¶ms.address, prev_bal, nonce_offset)?; } @@ -1103,9 +1103,13 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let refunded = cmp::min(refunds_bound, (t.gas - gas_left_prerefund) >> 1); let gas_left = gas_left_prerefund + refunded; - let gas_used = t.gas - gas_left; // ECHECH - let refund_value = gas_left * t.gas_price; // ECHECH obvious overflow here at left close toh h256 max and price same - let fees_value = gas_used * t.gas_price; // ECHECH overflow when close to H256 + let gas_used = t.gas.saturating_sub(gas_left); + let (refund_value, overflow_1) = gas_left.overflowing_mul(t.gas_price); + let (fees_value, overflow_2) = gas_used.overflowing_mul(t.gas_price); + if overflow_1 || overflow_2 { + return Err(ExecutionError::TransactionMalformed("U256 Overflow".to_string())); + } + trace!("exec::finalize: t.gas={}, sstore_refunds={}, suicide_refunds={}, refunds_bound={}, gas_left_prerefund={}, refunded={}, gas_left={}, gas_used={}, refund_value={}, fees_value={}\n", t.gas, sstore_refunds, suicide_refunds, refunds_bound, gas_left_prerefund, refunded, gas_left, gas_used, refund_value, fees_value); @@ -1123,7 +1127,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { } // perform garbage-collection - let min_balance = if schedule.kill_dust != CleanDustMode::Off { Some(U256::from(schedule.tx_gas) * t.gas_price) } else { None }; // ECHECH overflow next (as gas price from tx + let min_balance = if schedule.kill_dust != CleanDustMode::Off { Some(U256::from(schedule.tx_gas).overflowing_mul(t.gas_price).0) } else { None }; self.state.kill_garbage(&substate.touched, schedule.kill_empty, &min_balance, schedule.kill_dust == CleanDustMode::WithCodeAndStorage)?; match result { @@ -2178,11 +2182,3 @@ mod tests { assert_eq!(output[..], [0u8; 20][..]); } } - -#[test] -fn assert_no_overflow() { - let gas_price = U256::from(&vec![255u8;32][..]); - let gas = U256::from(&vec![255u8;32][..]); - let gas_cost = gas.full_mul(gas_price); - let total_cost = U512::from(U256::from(&vec![255u8;32][..])) + gas_cost; -} diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index 49de58aca51..483df7ba193 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -466,12 +466,12 @@ impl Account { /// Increment the nonce of the account by one. pub fn inc_nonce(&mut self) { - self.nonce = self.nonce + U256::from(1u8); // ECHECH overflow check here for case of nonce set by raw tx at U256.max + self.nonce = self.nonce.saturating_add(U256::from(1u8)); } /// Increase account balance. pub fn add_balance(&mut self, x: &U256) { - self.balance = self.balance + *x; // ECHECH overflow check even if not needed here + self.balance = self.balance.saturating_add(*x); } /// Decrease account balance. diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 74b36f9422f..8cc06fa83cc 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -500,7 +500,12 @@ impl State { /// it will have its code reset, ready for `init_code()`. pub fn new_contract(&mut self, contract: &Address, balance: U256, nonce_offset: U256) -> TrieResult<()> { let original_storage_root = self.original_storage_root(contract)?; - self.insert_cache(contract, AccountEntry::new_dirty(Some(Account::new_contract(balance, self.account_start_nonce + nonce_offset, original_storage_root)))); // ECHECH + let (nonce, overflow) = self.account_start_nonce.overflowing_add(nonce_offset); + if overflow { + return Err(Box::new(TrieError::DecoderError(H256::from(contract), + rlp::DecoderError::Custom("Nonce overflow".into())))); + } + self.insert_cache(contract, AccountEntry::new_dirty(Some(Account::new_contract(balance, nonce, original_storage_root)))); Ok(()) } diff --git a/miner/src/pool/queue.rs b/miner/src/pool/queue.rs index 0f91e1fa6a7..51c46ad823c 100644 --- a/miner/src/pool/queue.rs +++ b/miner/src/pool/queue.rs @@ -474,7 +474,7 @@ impl TransactionQueue { self.pool.read().pending_from_sender(state_readiness, address) .last() - .map(|tx| tx.signed().nonce + 1) // ECHECH this seems risky + .map(|tx| tx.signed().nonce.saturating_add(U256::from(1))) } /// Retrieve a transaction from the pool. diff --git a/miner/src/pool/ready.rs b/miner/src/pool/ready.rs index c0b4e3ee0cd..3accba13903 100644 --- a/miner/src/pool/ready.rs +++ b/miner/src/pool/ready.rs @@ -95,7 +95,7 @@ impl txpool::Ready for State { }, cmp::Ordering::Less => txpool::Readiness::Stale, cmp::Ordering::Equal => { - *nonce = *nonce + 1; // ECHECH + *nonce = nonce.saturating_add(U256::from(1)); txpool::Readiness::Ready }, } @@ -159,7 +159,7 @@ impl Option> txpool::Ready for Opt cmp::Ordering::Greater => txpool::Readiness::Future, cmp::Ordering::Less => txpool::Readiness::Stale, cmp::Ordering::Equal => { - *nonce = *nonce + 1; // ECHECH terminal nonce ignore? + *nonce = nonce.saturating_add(U256::from(1)); txpool::Readiness::Ready }, } diff --git a/miner/src/pool/verifier.rs b/miner/src/pool/verifier.rs index cd3557e5d76..1fded37630e 100644 --- a/miner/src/pool/verifier.rs +++ b/miner/src/pool/verifier.rs @@ -283,8 +283,19 @@ impl txpool::Verifier for Verifier