fix(levm): prevent unsafe arithmetic #1095
Conversation
| .checked_add(WORD_SIZE) | ||
| .ok_or(VMError::VeryLargeNumber)? | ||
| .saturating_sub(1)) | ||
| / WORD_SIZE; |
There was a problem hiding this comment.
Shouldn't we use checked_div here?
There was a problem hiding this comment.
checked_div only checks if denominator is zero, and WORD_SIZE is a non-zero constant
| let memory_byte_size = (args_offset | ||
| .checked_add(args_size) | ||
| .ok_or(VMError::VeryLargeNumber)?) | ||
| .max( | ||
| ret_offset | ||
| .checked_add(ret_size) | ||
| .ok_or(VMError::VeryLargeNumber)?, | ||
| ); |
There was a problem hiding this comment.
Nit. The parenthesis is not needed (do not commit this, it needs to be formatted).
| let memory_byte_size = (args_offset | |
| .checked_add(args_size) | |
| .ok_or(VMError::VeryLargeNumber)?) | |
| .max( | |
| ret_offset | |
| .checked_add(ret_size) | |
| .ok_or(VMError::VeryLargeNumber)?, | |
| ); | |
| let memory_byte_size = args_offset | |
| .checked_add(args_size) | |
| .ok_or(VMError::VeryLargeNumber)? | |
| .max( | |
| ret_offset | |
| .checked_add(ret_size) | |
| .ok_or(VMError::VeryLargeNumber)?, | |
| ); |
| call_opcode::NON_ZERO_VALUE_COST + call_opcode::BASIC_FALLBACK_FUNCTION_STIPEND | ||
| call_opcode::NON_ZERO_VALUE_COST | ||
| .checked_add(call_opcode::BASIC_FALLBACK_FUNCTION_STIPEND) | ||
| .ok_or(VMError::OverflowInArithmeticOp)? |
There was a problem hiding this comment.
What are the criteria for returning a VMError::OverflowInArithmeticOp and VMError::InternalError?
There was a problem hiding this comment.
The Internal's are those that should not happen in any case of execution, and the OverflowInArithmeticOp's are those that should not happen normally.
There was a problem hiding this comment.
Now I understand, should be an Internal because should not happen in any case
crates/vm/levm/src/vm.rs
Outdated
| current_call_frame.gas_used = match current_call_frame | ||
| .gas_used | ||
| .checked_add(tx_report.gas_used.into()) | ||
| { | ||
| Some(gas) => gas, | ||
| None => { | ||
| return Err(VMError::ConsumedGasOverflow); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Nit. We could use .ok_or here (it is manually implemented).
crates/vm/levm/src/vm.rs
Outdated
| @@ -737,11 +813,23 @@ impl VM { | |||
| current_call_frame: &mut CallFrame, | |||
| gas: U256, | |||
| ) -> Result<(), VMError> { | |||
| if current_call_frame.gas_used + gas > current_call_frame.gas_limit { | |||
| let potential_consumed_gas = match current_call_frame.gas_used.checked_add(gas) { | |||
crates/vm/levm/src/vm.rs
Outdated
| self.env.consumed_gas += gas; | ||
|
|
||
| current_call_frame.gas_used = potential_consumed_gas; | ||
| self.env.consumed_gas = match self.env.consumed_gas.checked_add(gas) { |
Motivation
The idea is to prevent arithmetic overflows and underflows in the levm code.
Description
Design desitions:
checked_addand added aConsumedGasOverflowerror.checked_addand theStackOverflowerrorVM::executemethod I decided to do not raise an error, because the previous behavior was to do not raise errors, so used the saturating functionsVMError::Internalbecause the value was restricted on input an is never going to cause problemsGasCostOverflowInvalidOpcodeerrorCloses #1075