From 0a76b4f1b442061a67565f8a8531986fe854c654 Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Tue, 1 Oct 2024 18:59:01 +0200 Subject: [PATCH] fix: conversion of stack values in opcode execution (#1005) * fix: conversion of stack values in opcode execution * fix test --- crates/evm/src/create_helpers.cairo | 2 +- .../src/instructions/block_information.cairo | 8 ++++++-- .../environmental_information.cairo | 17 +++++++++-------- .../src/instructions/logging_operations.cairo | 4 +++- .../src/instructions/memory_operations.cairo | 13 +++++++++---- crates/evm/src/instructions/sha3.cairo | 6 ++++-- .../src/instructions/system_operations.cairo | 18 +++++++++--------- crates/evm/src/interpreter.cairo | 2 +- crates/evm/src/stack.cairo | 3 ++- 9 files changed, 44 insertions(+), 29 deletions(-) diff --git a/crates/evm/src/create_helpers.cairo b/crates/evm/src/create_helpers.cairo index 28e8dc40b..3b8c39eca 100644 --- a/crates/evm/src/create_helpers.cairo +++ b/crates/evm/src/create_helpers.cairo @@ -40,7 +40,7 @@ pub impl CreateHelpersImpl of CreateHelpers { fn prepare_create(ref self: VM, create_type: CreateType) -> Result { let value = self.stack.pop()?; let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let memory_expansion = gas::memory_expansion(self.memory.size(), [(offset, size)].span())?; self.memory.ensure_length(memory_expansion.new_size); diff --git a/crates/evm/src/instructions/block_information.cairo b/crates/evm/src/instructions/block_information.cairo index f5e61db79..3f834a917 100644 --- a/crates/evm/src/instructions/block_information.cairo +++ b/crates/evm/src/instructions/block_information.cairo @@ -1,5 +1,6 @@ //! Block Information. +use core::num::traits::SaturatingAdd; use core::starknet::SyscallResultTrait; use core::starknet::syscalls::get_block_hash_syscall; @@ -20,7 +21,9 @@ pub impl BlockInformation of BlockInformationTrait { fn exec_blockhash(ref self: VM) -> Result<(), EVMError> { self.charge_gas(gas::BLOCKHASH)?; - let block_number = self.stack.pop_u64()?; + // Saturate to MAX_U64 to avoid a revert when the hash requested is too big. It should just + // push 0. + let block_number = self.stack.pop_saturating_u64()?; let current_block = self.env.block_number; // If input block number is lower than current_block - 256, return 0 @@ -31,7 +34,8 @@ pub impl BlockInformation of BlockInformationTrait { // TODO: monitor the changes in the `get_block_hash_syscall` syscall. // source: // https://docs.starknet.io/documentation/architecture_and_concepts/Smart_Contracts/system-calls-cairo1/#get_block_hash - if block_number + 10 > current_block || block_number + 256 < current_block { + if block_number.saturating_add(10) > current_block + || block_number.saturating_add(256) < current_block { return self.stack.push(0); } diff --git a/crates/evm/src/instructions/environmental_information.cairo b/crates/evm/src/instructions/environmental_information.cairo index d675193ff..9da876792 100644 --- a/crates/evm/src/instructions/environmental_information.cairo +++ b/crates/evm/src/instructions/environmental_information.cairo @@ -72,7 +72,8 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { fn exec_calldataload(ref self: VM) -> Result<(), EVMError> { self.charge_gas(gas::VERYLOW)?; - let offset: usize = self.stack.pop_usize()?; + // Don't error out if the offset is too big. It should just push 0. + let offset: usize = self.stack.pop_saturating_usize()?; let calldata = self.message().data; let calldata_len = calldata.len(); @@ -113,7 +114,7 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { fn exec_calldatacopy(ref self: VM) -> Result<(), EVMError> { let dest_offset = self.stack.pop_saturating_usize()?; let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let words_size = bytes_32_words_size(size).into(); let copy_gas_cost = gas::COPY * words_size; @@ -143,7 +144,7 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { fn exec_codecopy(ref self: VM) -> Result<(), EVMError> { let dest_offset = self.stack.pop_saturating_usize()?; let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let words_size = bytes_32_words_size(size).into(); let copy_gas_cost = gas::COPY * words_size; @@ -192,7 +193,7 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { let evm_address = self.stack.pop_eth_address()?; let dest_offset = self.stack.pop_saturating_usize()?; let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. // GAS let words_size = bytes_32_words_size(size).into(); @@ -229,7 +230,7 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { fn exec_returndatacopy(ref self: VM) -> Result<(), EVMError> { let dest_offset = self.stack.pop_saturating_usize()?; let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let return_data: Span = self.return_data(); let (last_returndata_index, overflow) = offset.overflowing_add(size); @@ -549,7 +550,7 @@ mod tests { #[test] - fn test_calldataload_with_offset_conversion_error() { + fn test_calldataload_with_offset_bigger_usize_succeeds() { // Given let calldata = u256_to_bytes_array( 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF @@ -562,8 +563,8 @@ mod tests { let result = vm.exec_calldataload(); // Then - assert!(result.is_err()); - assert_eq!(result.unwrap_err(), EVMError::TypeConversionError(TYPE_CONVERSION_ERROR)); + assert!(result.is_ok()); + assert_eq!(vm.stack.pop().unwrap(), 0); } // ************************************************************************* diff --git a/crates/evm/src/instructions/logging_operations.cairo b/crates/evm/src/instructions/logging_operations.cairo index 6e8b1cd4a..12c86a30d 100644 --- a/crates/evm/src/instructions/logging_operations.cairo +++ b/crates/evm/src/instructions/logging_operations.cairo @@ -61,11 +61,13 @@ fn exec_log_i(ref self: VM, topics_len: u8) -> Result<(), EVMError> { // TODO(optimization): check benefits of n `pop` instead of `pop_n` let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let topics: Array = self.stack.pop_n(topics_len.into())?; let memory_expansion = gas::memory_expansion(self.memory.size(), [(offset, size)].span())?; self.memory.ensure_length(memory_expansion.new_size); + + // TODO: avoid addition overflows here. We should use checked arithmetic. self .charge_gas( gas::LOG diff --git a/crates/evm/src/instructions/memory_operations.cairo b/crates/evm/src/instructions/memory_operations.cairo index 3c60ee4b4..fc2d81b61 100644 --- a/crates/evm/src/instructions/memory_operations.cairo +++ b/crates/evm/src/instructions/memory_operations.cairo @@ -36,7 +36,9 @@ pub impl MemoryOperation of MemoryOperationTrait { /// MLOAD operation. /// Load word from memory and push to stack. fn exec_mload(ref self: VM) -> Result<(), EVMError> { - let offset: usize = self.stack.pop_usize()?; + let offset: usize = self + .stack + .pop_usize()?; // Any offset bigger than a usize would MemoryOOG. let memory_expansion = gas::memory_expansion(self.memory.size(), [(offset, 32)].span())?; self.memory.ensure_length(memory_expansion.new_size); @@ -50,7 +52,9 @@ pub impl MemoryOperation of MemoryOperationTrait { /// Save word to memory. /// # Specification: https://www.evm.codes/#52?fork=shanghai fn exec_mstore(ref self: VM) -> Result<(), EVMError> { - let offset: usize = self.stack.pop_usize()?; + let offset: usize = self + .stack + .pop_usize()?; // Any offset bigger than a usize would MemoryOOG. let value: u256 = self.stack.pop()?; let memory_expansion = gas::memory_expansion(self.memory.size(), [(offset, 32)].span())?; self.memory.ensure_length(memory_expansion.new_size); @@ -64,7 +68,7 @@ pub impl MemoryOperation of MemoryOperationTrait { /// Save single byte to memory /// # Specification: https://www.evm.codes/#53?fork=shanghai fn exec_mstore8(ref self: VM) -> Result<(), EVMError> { - let offset = self.stack.pop_saturating_usize()?; + let offset = self.stack.pop_usize()?; // Any offset bigger than a usize would MemoryOOG. let value = self.stack.pop()?; let value: u8 = (value.low & 0xFF).try_into().unwrap(); @@ -282,7 +286,7 @@ pub impl MemoryOperation of MemoryOperationTrait { fn exec_mcopy(ref self: VM) -> Result<(), EVMError> { let dest_offset = self.stack.pop_saturating_usize()?; let source_offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let words_size = bytes_32_words_size(size).into(); let copy_gas_cost = gas::COPY * words_size; @@ -290,6 +294,7 @@ pub impl MemoryOperation of MemoryOperationTrait { self.memory.size(), [(max(dest_offset, source_offset), size)].span() )?; self.memory.ensure_length(memory_expansion.new_size); + //TODO: handle add overflows self.charge_gas(gas::VERYLOW + copy_gas_cost + memory_expansion.expansion_cost)?; if size == 0 { diff --git a/crates/evm/src/instructions/sha3.cairo b/crates/evm/src/instructions/sha3.cairo index 28f740398..622abf23f 100644 --- a/crates/evm/src/instructions/sha3.cairo +++ b/crates/evm/src/instructions/sha3.cairo @@ -23,8 +23,10 @@ pub impl Sha3Impl of Sha3Trait { /// /// # Specification: https://www.evm.codes/#20?fork=shanghai fn exec_sha3(ref self: VM) -> Result<(), EVMError> { - let offset: usize = self.stack.pop_usize()?; - let mut size: usize = self.stack.pop_usize()?; + let offset: usize = self.stack.pop_saturating_usize()?; + let mut size: usize = self + .stack + .pop_usize()?; // Any size bigger than a usize would MemoryOOG. let words_size = bytes_32_words_size(size).into(); let word_gas_cost = gas::KECCAK256WORD * words_size; diff --git a/crates/evm/src/instructions/system_operations.cairo b/crates/evm/src/instructions/system_operations.cairo index fcfb1a633..5511b6017 100644 --- a/crates/evm/src/instructions/system_operations.cairo +++ b/crates/evm/src/instructions/system_operations.cairo @@ -30,9 +30,9 @@ pub impl SystemOperations of SystemOperationsTrait { let to = self.stack.pop_eth_address()?; let value = self.stack.pop()?; let args_offset = self.stack.pop_saturating_usize()?; - let args_size = self.stack.pop_usize()?; + let args_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let ret_offset = self.stack.pop_saturating_usize()?; - let ret_size = self.stack.pop_usize()?; + let ret_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. // GAS let memory_expansion = gas::memory_expansion( @@ -109,9 +109,9 @@ pub impl SystemOperations of SystemOperationsTrait { let code_address = self.stack.pop_eth_address()?; let value = self.stack.pop()?; let args_offset = self.stack.pop_saturating_usize()?; - let args_size = self.stack.pop_usize()?; + let args_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let ret_offset = self.stack.pop_saturating_usize()?; - let ret_size = self.stack.pop_usize()?; + let ret_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let to = self.message().target.evm; @@ -193,9 +193,9 @@ pub impl SystemOperations of SystemOperationsTrait { let gas = self.stack.pop_saturating_u64()?; let code_address = self.stack.pop_eth_address()?; let args_offset = self.stack.pop_saturating_usize()?; - let args_size = self.stack.pop_usize()?; + let args_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let ret_offset = self.stack.pop_saturating_usize()?; - let ret_size = self.stack.pop_usize()?; + let ret_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. // GAS let memory_expansion = gas::memory_expansion( @@ -246,9 +246,9 @@ pub impl SystemOperations of SystemOperationsTrait { let gas = self.stack.pop_saturating_u64()?; let to = self.stack.pop_eth_address()?; let args_offset = self.stack.pop_saturating_usize()?; - let args_size = self.stack.pop_usize()?; + let args_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let ret_offset = self.stack.pop_saturating_usize()?; - let ret_size = self.stack.pop_usize()?; + let ret_size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. // GAS let memory_expansion = gas::memory_expansion( @@ -293,7 +293,7 @@ pub impl SystemOperations of SystemOperationsTrait { /// # Specification: https://www.evm.codes/#fd?fork=shanghai fn exec_revert(ref self: VM) -> Result<(), EVMError> { let offset = self.stack.pop_saturating_usize()?; - let size = self.stack.pop_usize()?; + let size = self.stack.pop_usize()?; // Any size bigger than a usize would MemoryOOG. let memory_expansion = gas::memory_expansion(self.memory.size(), [(offset, size)].span())?; self.memory.ensure_length(memory_expansion.new_size); diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index d17883c1e..ccf14cda7 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -19,7 +19,7 @@ use crate::model::account::{Account, AccountTrait}; use crate::model::vm::{VM, VMTrait}; use crate::model::{ Message, Environment, Transfer, ExecutionSummary, ExecutionResult, ExecutionResultTrait, - ExecutionResultStatus, AddressTrait, TransactionResult, TransactionResultTrait, Address + ExecutionResultStatus, AddressTrait, TransactionResult, Address }; use crate::precompiles::Precompiles; use crate::precompiles::eth_precompile_addresses; diff --git a/crates/evm/src/stack.cairo b/crates/evm/src/stack.cairo index 506e87d9f..74ddcba68 100644 --- a/crates/evm/src/stack.cairo +++ b/crates/evm/src/stack.cairo @@ -186,7 +186,8 @@ impl StackImpl of StackTrait { Result::Ok(item.low) } - /// Calls `Stack::pop` and converts it to usize + /// Calls `Stack::pop` and converts it to an EthAddress + /// If the value is bigger than an EthAddress, it will be truncated to keep the lower 160 bits. /// /// # Errors ///