fix: conversion of stack values in opcode execution (#1005)

* fix: conversion of stack values in opcode execution * fix test
kkrt-labs · Oct 1, 2024 · 0a76b4f · 0a76b4f
1 parent 4afdbfe
commit 0a76b4f
Show file tree

Hide file tree

Showing 9 changed files with 44 additions and 29 deletions.
diff --git 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<CreateArgs, EVMError> {
         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
@@ -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
@@ -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<u8> = 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
@@ -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<u256> = 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
@@ -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,14 +286,15 @@ 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;
         let memory_expansion = gas::memory_expansion(
             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
@@ -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
@@ -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
@@ -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
@@ -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
     ///