[EVM-Equivalence-YUL] Fix overflow checks (#29)

* Remove invalid todos adding overflow checks * Remove old memory check * Add preprocessed evm interpreter * Remove unnecesary memory checks * Update hash for evm interpreter
lambdaclass · Sep 13, 2024 · 32c05a1 · 32c05a1
1 parent 220e983
commit 32c05a1
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 56 deletions.
diff --git a/system-contracts/SystemContractsHashes.json b/system-contracts/SystemContractsHashes.json
@@ -129,8 +129,8 @@
     "contractName": "EvmInterpreter",
     "bytecodePath": "contracts-preprocessed/artifacts/EvmInterpreter.yul.zbin",
     "sourceCodePath": "contracts-preprocessed/EvmInterpreter.yul",
-    "bytecodeHash": "0x01000cef160515b2631803991c1d49b6b44492406197fb6dc22a8cf05cebd5d5",
-    "sourceCodeHash": "0x6c1e3d4c2f94342792df4fc671a0929fbb2d5aba1b5e388c70f4dc1ee96cfa74"
+    "bytecodeHash": "0x01000cdf5bb7dd8a97faf231a5e1e20f2fe308d6f200c3295c6e3629547cc4a4",
+    "sourceCodeHash": "0xe1133c2af9e4fc38e845f7fcc23f3cbab37b857013f55b6e7e9bdea28f331c40"
   },
   {
     "contractName": "CodeOracle",

diff --git a/system-contracts/contracts/EvmInterpreter.yul b/system-contracts/contracts/EvmInterpreter.yul
@@ -747,12 +747,6 @@ object "EVMInterpreter" {
             ret := farCallAbi
         }
 
-        function ensureAcceptableMemLocation(location) {
-            if gt(location,MAX_POSSIBLE_MEM()) {
-                revert(0,0) // Check if this is what's needed
-            }
-        }
-
         function addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft) -> newOffset,newSize {
             newOffset := offset
             newSize := size
@@ -1976,13 +1970,8 @@ object "EVMInterpreter" {
                     offset, sp := popStackItemWithoutCheck(sp)
                     size, sp := popStackItemWithoutCheck(sp)
 
-                    checkMultipleOverflow(offset,size,MEM_OFFSET_INNER(), evmGasLeft)
-                    checkMultipleOverflow(destOffset,size,MEM_OFFSET_INNER(), evmGasLeft)
-
-                    // TODO invalid?
-                    if or(gt(add(add(offset, size), MEM_OFFSET_INNER()), MAX_POSSIBLE_MEM()), gt(add(add(destOffset, size), MEM_OFFSET_INNER()), MAX_POSSIBLE_MEM())) {
-                        $llvm_AlwaysInline_llvm$_memsetToZero(add(destOffset, MEM_OFFSET_INNER()), size)
-                    }
+                    checkOverflow(destOffset, size, evmGasLeft)
+                    checkMemOverflowByOffset(add(destOffset,size), evmGasLeft)
 
                     // dynamicGas = 3 * minimum_word_size + memory_expansion_cost
                     // minimum_word_size = (size + 31) / 32
@@ -2020,6 +2009,7 @@ object "EVMInterpreter" {
                     offset := add(add(offset, BYTECODE_OFFSET()), 32)
 
                     checkOverflow(dst,len, evmGasLeft)
+                    checkOverflow(offset,len, evmGasLeft)
                     checkMemOverflow(add(dst, len), evmGasLeft)
                     // Check bytecode overflow
                     if gt(add(offset, len), sub(MEM_OFFSET(), 1)) {
@@ -2378,7 +2368,8 @@ object "EVMInterpreter" {
                     offset, sp := popStackItemWithoutCheck(sp)
                     size, sp := popStackItemWithoutCheck(sp)
 
-                    // TODO overflow checks
+                    checkOverflow(offset, size, evmGasLeft)
+                    checkOverflow(destOffset, size, evmGasLeft)
                     checkMemOverflowByOffset(add(offset, size), evmGasLeft)
                     checkMemOverflowByOffset(add(destOffset, size), evmGasLeft)
 
@@ -2971,10 +2962,12 @@ object "EVMInterpreter" {
                     size, sp := popStackItemWithoutCheck(sp)
 
                     checkOverflow(offset,size, evmGasLeft)
+                    checkMemOverflowByOffset(add(offset,size), evmGasLeft)
                     evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size)))
 
                     returnLen := size
-                    checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft)
+
+                    // Don't check overflow here since previous checks are enough to ensure this is safe
                     returnOffset := add(MEM_OFFSET_INNER(), offset)
                     break
                 }
@@ -3010,11 +3003,12 @@ object "EVMInterpreter" {
                     offset, sp := popStackItemWithoutCheck(sp)
                     size, sp := popStackItemWithoutCheck(sp)
 
-                    // TODO invalid?
-                    ensureAcceptableMemLocation(offset)
-                    ensureAcceptableMemLocation(size)
+                    checkOverflow(offset,size, evmGasLeft)
+                    checkMemOverflowByOffset(add(offset, size), evmGasLeft)
                     evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size)))
 
+
+                    // Don't check overflow here since previous checks are enough to ensure this is safe
                     offset := add(offset, MEM_OFFSET_INNER())
                     offset,size := addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft)
 
@@ -3729,12 +3723,6 @@ object "EVMInterpreter" {
                 ret := farCallAbi
             }
 
-            function ensureAcceptableMemLocation(location) {
-                if gt(location,MAX_POSSIBLE_MEM()) {
-                    revert(0,0) // Check if this is what's needed
-                }
-            }
-
             function addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft) -> newOffset,newSize {
                 newOffset := offset
                 newSize := size
@@ -4958,13 +4946,8 @@ object "EVMInterpreter" {
                         offset, sp := popStackItemWithoutCheck(sp)
                         size, sp := popStackItemWithoutCheck(sp)
 
-                        checkMultipleOverflow(offset,size,MEM_OFFSET_INNER(), evmGasLeft)
-                        checkMultipleOverflow(destOffset,size,MEM_OFFSET_INNER(), evmGasLeft)
-
-                        // TODO invalid?
-                        if or(gt(add(add(offset, size), MEM_OFFSET_INNER()), MAX_POSSIBLE_MEM()), gt(add(add(destOffset, size), MEM_OFFSET_INNER()), MAX_POSSIBLE_MEM())) {
-                            $llvm_AlwaysInline_llvm$_memsetToZero(add(destOffset, MEM_OFFSET_INNER()), size)
-                        }
+                        checkOverflow(destOffset, size, evmGasLeft)
+                        checkMemOverflowByOffset(add(destOffset,size), evmGasLeft)
 
                         // dynamicGas = 3 * minimum_word_size + memory_expansion_cost
                         // minimum_word_size = (size + 31) / 32
@@ -5002,6 +4985,7 @@ object "EVMInterpreter" {
                         offset := add(add(offset, BYTECODE_OFFSET()), 32)
 
                         checkOverflow(dst,len, evmGasLeft)
+                        checkOverflow(offset,len, evmGasLeft)
                         checkMemOverflow(add(dst, len), evmGasLeft)
                         // Check bytecode overflow
                         if gt(add(offset, len), sub(MEM_OFFSET(), 1)) {
@@ -5360,7 +5344,8 @@ object "EVMInterpreter" {
                         offset, sp := popStackItemWithoutCheck(sp)
                         size, sp := popStackItemWithoutCheck(sp)
 
-                        // TODO overflow checks
+                        checkOverflow(offset, size, evmGasLeft)
+                        checkOverflow(destOffset, size, evmGasLeft)
                         checkMemOverflowByOffset(add(offset, size), evmGasLeft)
                         checkMemOverflowByOffset(add(destOffset, size), evmGasLeft)
 
@@ -5953,10 +5938,12 @@ object "EVMInterpreter" {
                         size, sp := popStackItemWithoutCheck(sp)
 
                         checkOverflow(offset,size, evmGasLeft)
+                        checkMemOverflowByOffset(add(offset,size), evmGasLeft)
                         evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size)))
 
                         returnLen := size
-                        checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft)
+
+                        // Don't check overflow here since previous checks are enough to ensure this is safe
                         returnOffset := add(MEM_OFFSET_INNER(), offset)
                         break
                     }
@@ -5992,11 +5979,12 @@ object "EVMInterpreter" {
                         offset, sp := popStackItemWithoutCheck(sp)
                         size, sp := popStackItemWithoutCheck(sp)
 
-                        // TODO invalid?
-                        ensureAcceptableMemLocation(offset)
-                        ensureAcceptableMemLocation(size)
+                        checkOverflow(offset,size, evmGasLeft)
+                        checkMemOverflowByOffset(add(offset, size), evmGasLeft)
                         evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size)))
 
+
+                        // Don't check overflow here since previous checks are enough to ensure this is safe
                         offset := add(offset, MEM_OFFSET_INNER())
                         offset,size := addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft)
 

diff --git a/system-contracts/evm-interpreter/EvmInterpreterFunctions.template.yul b/system-contracts/evm-interpreter/EvmInterpreterFunctions.template.yul
@@ -665,12 +665,6 @@ function getFarCallABI(
     ret := farCallAbi
 }
 
-function ensureAcceptableMemLocation(location) {
-    if gt(location,MAX_POSSIBLE_MEM()) {
-        revert(0,0) // Check if this is what's needed
-    }
-}
-
 function addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft) -> newOffset,newSize {
     newOffset := offset
     newSize := size

diff --git a/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul b/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul
@@ -410,13 +410,8 @@ for { } true { } {
         offset, sp := popStackItemWithoutCheck(sp)
         size, sp := popStackItemWithoutCheck(sp)
 
-        checkMultipleOverflow(offset,size,MEM_OFFSET_INNER(), evmGasLeft)
-        checkMultipleOverflow(destOffset,size,MEM_OFFSET_INNER(), evmGasLeft)
-
-        // TODO invalid?
-        if or(gt(add(add(offset, size), MEM_OFFSET_INNER()), MAX_POSSIBLE_MEM()), gt(add(add(destOffset, size), MEM_OFFSET_INNER()), MAX_POSSIBLE_MEM())) {
-            $llvm_AlwaysInline_llvm$_memsetToZero(add(destOffset, MEM_OFFSET_INNER()), size)
-        }
+        checkOverflow(destOffset, size, evmGasLeft)
+        checkMemOverflowByOffset(add(destOffset,size), evmGasLeft)
 
         // dynamicGas = 3 * minimum_word_size + memory_expansion_cost
         // minimum_word_size = (size + 31) / 32
@@ -454,6 +449,7 @@ for { } true { } {
         offset := add(add(offset, BYTECODE_OFFSET()), 32)
 
         checkOverflow(dst,len, evmGasLeft)
+        checkOverflow(offset,len, evmGasLeft)
         checkMemOverflow(add(dst, len), evmGasLeft)
         // Check bytecode overflow
         if gt(add(offset, len), sub(MEM_OFFSET(), 1)) {
@@ -812,7 +808,8 @@ for { } true { } {
         offset, sp := popStackItemWithoutCheck(sp)
         size, sp := popStackItemWithoutCheck(sp)
 
-        // TODO overflow checks
+        checkOverflow(offset, size, evmGasLeft)
+        checkOverflow(destOffset, size, evmGasLeft)
         checkMemOverflowByOffset(add(offset, size), evmGasLeft)
         checkMemOverflowByOffset(add(destOffset, size), evmGasLeft)
 
@@ -1405,10 +1402,12 @@ for { } true { } {
         size, sp := popStackItemWithoutCheck(sp)
 
         checkOverflow(offset,size, evmGasLeft)
+        checkMemOverflowByOffset(add(offset,size), evmGasLeft)
         evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size)))
 
         returnLen := size
-        checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft)
+
+        // Don't check overflow here since previous checks are enough to ensure this is safe
         returnOffset := add(MEM_OFFSET_INNER(), offset)
         break
     }
@@ -1444,11 +1443,12 @@ for { } true { } {
         offset, sp := popStackItemWithoutCheck(sp)
         size, sp := popStackItemWithoutCheck(sp)
 
-        // TODO invalid?
-        ensureAcceptableMemLocation(offset)
-        ensureAcceptableMemLocation(size)
+        checkOverflow(offset,size, evmGasLeft)
+        checkMemOverflowByOffset(add(offset, size), evmGasLeft)
         evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size)))
 
+
+        // Don't check overflow here since previous checks are enough to ensure this is safe
         offset := add(offset, MEM_OFFSET_INNER())
         offset,size := addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft)