From dd680ee75f007c272cbe11cf57127c9ba436d020 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jerem=C3=ADas=20Salom=C3=B3n?= <48994069+JereSalo@users.noreply.github.com> Date: Mon, 9 Dec 2024 18:02:04 -0300 Subject: [PATCH] fix(levm): memory size calculations (#1445) **Motivation** - Currently we have some errors in gas calculations related to memory when "size" is set to 0. Memory shouldn't ever expand on that scenario. - Tidy code for calculating memory size. **Description** - Create function `calculate_memory_size` in memory module - Use function in every opcode that needs it **Results** - 7 more tests pass when comparing directly to last main changes. Closes #1442 --- crates/vm/levm/src/memory.rs | 11 ++++ .../levm/src/opcode_handlers/environment.rs | 48 ++++++------------ crates/vm/levm/src/opcode_handlers/keccak.rs | 16 ++---- crates/vm/levm/src/opcode_handlers/logging.rs | 12 ++--- .../stack_memory_storage_flow.rs | 50 ++++++------------- crates/vm/levm/src/opcode_handlers/system.rs | 13 ++--- crates/vm/levm/tests/edge_case_tests.rs | 10 ++++ 7 files changed, 64 insertions(+), 96 deletions(-) diff --git a/crates/vm/levm/src/memory.rs b/crates/vm/levm/src/memory.rs index 6a2de0bf7..cce7a1b1e 100644 --- a/crates/vm/levm/src/memory.rs +++ b/crates/vm/levm/src/memory.rs @@ -207,3 +207,14 @@ fn cost(memory_size: usize) -> Result { ) .ok_or(OutOfGasError::MemoryExpansionCostOverflow)?) } + +pub fn calculate_memory_size(offset: usize, size: usize) -> Result { + if size == 0 { + return Ok(0); + } + + offset + .checked_add(size) + .and_then(|sum| sum.checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE)) + .ok_or(VMError::OutOfOffset) +} diff --git a/crates/vm/levm/src/opcode_handlers/environment.rs b/crates/vm/levm/src/opcode_handlers/environment.rs index ca16c99f3..4f04f1656 100644 --- a/crates/vm/levm/src/opcode_handlers/environment.rs +++ b/crates/vm/levm/src/opcode_handlers/environment.rs @@ -1,8 +1,8 @@ use crate::{ call_frame::CallFrame, - constants::WORD_SIZE_IN_BYTES_USIZE, errors::{InternalError, OpcodeSuccess, VMError}, - gas_cost, memory, + gas_cost, + memory::{self, calculate_memory_size}, vm::{word_to_address, VM}, }; use ethrex_core::U256; @@ -150,17 +150,11 @@ impl VM { .try_into() .map_err(|_err| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(dest_offset, size)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::calldatacopy( - dest_offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - size, - )?, + gas_cost::calldatacopy(new_memory_size, current_call_frame.memory.len(), size)?, )?; if size == 0 { @@ -229,17 +223,11 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(destination_offset, size)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::codecopy( - destination_offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - size, - )?, + gas_cost::codecopy(new_memory_size, current_call_frame.memory.len(), size)?, )?; if size == 0 { @@ -318,14 +306,12 @@ impl VM { let (account_info, address_was_cold) = self.access_account(address); + let new_memory_size = calculate_memory_size(dest_offset, size)?; + self.increase_consumed_gas( current_call_frame, gas_cost::extcodecopy( - dest_offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, + new_memory_size, current_call_frame.memory.len(), address_was_cold, )?, @@ -388,17 +374,11 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(dest_offset, size)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::returndatacopy( - dest_offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - size, - )?, + gas_cost::returndatacopy(new_memory_size, current_call_frame.memory.len(), size)?, )?; if size == 0 { diff --git a/crates/vm/levm/src/opcode_handlers/keccak.rs b/crates/vm/levm/src/opcode_handlers/keccak.rs index 2be644af9..3e0b3c909 100644 --- a/crates/vm/levm/src/opcode_handlers/keccak.rs +++ b/crates/vm/levm/src/opcode_handlers/keccak.rs @@ -1,8 +1,8 @@ use crate::{ call_frame::CallFrame, - constants::WORD_SIZE_IN_BYTES_USIZE, errors::{OpcodeSuccess, VMError}, - gas_cost, memory, + gas_cost, + memory::{self, calculate_memory_size}, vm::VM, }; use ethrex_core::U256; @@ -27,17 +27,11 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(offset, size)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::keccak256( - offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - size, - )?, + gas_cost::keccak256(new_memory_size, current_call_frame.memory.len(), size)?, )?; let mut hasher = Keccak256::new(); diff --git a/crates/vm/levm/src/opcode_handlers/logging.rs b/crates/vm/levm/src/opcode_handlers/logging.rs index 867f19bab..648b8d73d 100644 --- a/crates/vm/levm/src/opcode_handlers/logging.rs +++ b/crates/vm/levm/src/opcode_handlers/logging.rs @@ -1,8 +1,8 @@ use crate::{ call_frame::CallFrame, - constants::WORD_SIZE_IN_BYTES_USIZE, errors::{OpcodeSuccess, VMError}, - gas_cost, memory, + gas_cost, + memory::{self, calculate_memory_size}, vm::VM, }; use bytes::Bytes; @@ -40,14 +40,12 @@ impl VM { topics.push(H256::from_slice(&topic_bytes)); } + let new_memory_size = calculate_memory_size(offset, size)?; + self.increase_consumed_gas( current_call_frame, gas_cost::log( - offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, + new_memory_size, current_call_frame.memory.len(), size, number_of_topics, diff --git a/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs b/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs index 1b7abab42..274c54b10 100644 --- a/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs +++ b/crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs @@ -2,7 +2,8 @@ use crate::{ call_frame::CallFrame, constants::{WORD_SIZE, WORD_SIZE_IN_BYTES_USIZE}, errors::{OpcodeSuccess, OutOfGasError, VMError}, - gas_cost, memory, + gas_cost, + memory::{self, calculate_memory_size}, opcodes::Opcode, vm::VM, }; @@ -64,16 +65,11 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(offset, WORD_SIZE_IN_BYTES_USIZE)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::mload( - offset - .checked_add(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - )?, + gas_cost::mload(new_memory_size, current_call_frame.memory.len())?, )?; current_call_frame @@ -94,16 +90,11 @@ impl VM { .try_into() .map_err(|_err| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(offset, WORD_SIZE_IN_BYTES_USIZE)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::mstore( - offset - .checked_add(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - )?, + gas_cost::mstore(new_memory_size, current_call_frame.memory.len())?, )?; let value = current_call_frame.stack.pop()?; @@ -127,16 +118,11 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; + let new_memory_size = calculate_memory_size(offset, 1)?; + self.increase_consumed_gas( current_call_frame, - gas_cost::mstore8( - offset - .checked_add(1) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?, - current_call_frame.memory.len(), - )?, + gas_cost::mstore8(new_memory_size, current_call_frame.memory.len())?, )?; let value = current_call_frame.stack.pop()?; @@ -295,17 +281,9 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; - let new_memory_size_for_dest = dest_offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?; - - let new_memory_size_for_src = src_offset - .checked_add(size) - .ok_or(VMError::OutOfOffset)? - .checked_next_multiple_of(WORD_SIZE_IN_BYTES_USIZE) - .ok_or(VMError::OutOfOffset)?; + let new_memory_size_for_dest = calculate_memory_size(dest_offset, size)?; + + let new_memory_size_for_src = calculate_memory_size(src_offset, size)?; self.increase_consumed_gas( current_call_frame, diff --git a/crates/vm/levm/src/opcode_handlers/system.rs b/crates/vm/levm/src/opcode_handlers/system.rs index 973242785..8316aead6 100644 --- a/crates/vm/levm/src/opcode_handlers/system.rs +++ b/crates/vm/levm/src/opcode_handlers/system.rs @@ -1,8 +1,9 @@ use crate::{ call_frame::CallFrame, constants::WORD_SIZE_IN_BYTES_USIZE, - errors::{InternalError, OpcodeSuccess, OutOfGasError, ResultReason, VMError}, - gas_cost, memory, + errors::{InternalError, OpcodeSuccess, ResultReason, VMError}, + gas_cost, + memory::{self, calculate_memory_size}, vm::{word_to_address, VM}, }; use ethrex_core::{types::TxKind, Address, U256}; @@ -193,9 +194,7 @@ impl VM { .try_into() .map_err(|_| VMError::VeryLargeNumber)?; - let new_memory_size = offset - .checked_add(size) - .ok_or(VMError::OutOfGas(OutOfGasError::GasCostOverflow))?; + let new_memory_size = calculate_memory_size(offset, size)?; self.increase_consumed_gas( current_call_frame, memory::expansion_cost(new_memory_size, current_call_frame.memory.len())?.into(), @@ -461,9 +460,7 @@ impl VM { .try_into() .map_err(|_err| VMError::VeryLargeNumber)?; - let new_memory_size = offset - .checked_add(size) - .ok_or(VMError::OutOfGas(OutOfGasError::GasCostOverflow))?; + let new_memory_size = calculate_memory_size(offset, size)?; self.increase_consumed_gas( current_call_frame, memory::expansion_cost(new_memory_size, current_call_frame.memory.len())?.into(), diff --git a/crates/vm/levm/tests/edge_case_tests.rs b/crates/vm/levm/tests/edge_case_tests.rs index 3e98e653c..256a020c9 100644 --- a/crates/vm/levm/tests/edge_case_tests.rs +++ b/crates/vm/levm/tests/edge_case_tests.rs @@ -288,3 +288,13 @@ fn test_non_compliance_codecopy_memory_resize() { &U256::from(14400) ); } + +#[test] +fn test_non_compliance_log_gas_cost() { + let mut vm = new_vm_with_bytecode(Bytes::copy_from_slice(&[56, 68, 68, 68, 131, 163])).unwrap(); + vm.env.gas_price = U256::zero(); + vm.env.gas_limit = U256::from(100_000_000); + vm.env.block_gas_limit = U256::from(100_000_001); + let res = vm.transact().unwrap(); + assert_eq!(res.gas_used, 22511); +}