From bbac2a0a28d3ab635d42221a92ae9b85ca639c6b Mon Sep 17 00:00:00 2001 From: IAvecilla Date: Tue, 10 Sep 2024 15:21:53 -0300 Subject: [PATCH 1/5] Remove invalid todos adding overflow checks --- system-contracts/contracts/EvmInterpreter.yul | 48 +++++++------------ .../EvmInterpreterFunctions.template.yul | 6 --- .../EvmInterpreterLoop.template.yul | 18 +++---- 3 files changed, 27 insertions(+), 45 deletions(-) diff --git a/system-contracts/contracts/EvmInterpreter.yul b/system-contracts/contracts/EvmInterpreter.yul index 077560eb2..33f672759 100644 --- a/system-contracts/contracts/EvmInterpreter.yul +++ b/system-contracts/contracts/EvmInterpreter.yul @@ -741,12 +741,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 @@ -1970,13 +1964,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 @@ -2014,6 +2003,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)) { @@ -2370,7 +2360,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) @@ -2963,6 +2954,7 @@ object "EVMInterpreter" { size, sp := popStackItemWithoutCheck(sp) checkOverflow(offset,size, evmGasLeft) + checkMemOverflowByOffset(add(offset,size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) returnLen := size @@ -3002,11 +2994,13 @@ 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))) + checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft) offset := add(offset, MEM_OFFSET_INNER()) offset,size := addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft) @@ -3715,12 +3709,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 @@ -4944,13 +4932,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 @@ -4988,6 +4971,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)) { @@ -5344,7 +5328,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) @@ -5937,6 +5922,7 @@ object "EVMInterpreter" { size, sp := popStackItemWithoutCheck(sp) checkOverflow(offset,size, evmGasLeft) + checkMemOverflowByOffset(add(offset,size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) returnLen := size @@ -5976,11 +5962,13 @@ 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))) + checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft) 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 index f53c079d8..2fcc76667 100644 --- a/system-contracts/evm-interpreter/EvmInterpreterFunctions.template.yul +++ b/system-contracts/evm-interpreter/EvmInterpreterFunctions.template.yul @@ -659,12 +659,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 index 5880cd1f4..f31c2c0ce 100644 --- 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)) { @@ -810,7 +806,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) @@ -1403,6 +1400,7 @@ for { } true { } { size, sp := popStackItemWithoutCheck(sp) checkOverflow(offset,size, evmGasLeft) + checkMemOverflowByOffset(add(offset,size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) returnLen := size @@ -1442,11 +1440,13 @@ 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))) + checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft) offset := add(offset, MEM_OFFSET_INNER()) offset,size := addGasIfEvmRevert(isCallerEVM,offset,size,evmGasLeft) From 4083e2f96f89166bb368a39e7ecbe64aa7821281 Mon Sep 17 00:00:00 2001 From: IAvecilla Date: Tue, 10 Sep 2024 15:27:25 -0300 Subject: [PATCH 2/5] Remove old memory check --- .../evm-interpreter/EvmInterpreterLoop.template.yul | 2 -- 1 file changed, 2 deletions(-) diff --git a/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul b/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul index f31c2c0ce..ab4e3d61a 100644 --- a/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul +++ b/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul @@ -1440,8 +1440,6 @@ for { } true { } { offset, sp := popStackItemWithoutCheck(sp) size, sp := popStackItemWithoutCheck(sp) - ensureAcceptableMemLocation(offset) - ensureAcceptableMemLocation(size) checkOverflow(offset,size, evmGasLeft) checkMemOverflowByOffset(add(offset, size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) From d3d66a95e259369905d41c59866068c0b1e172f3 Mon Sep 17 00:00:00 2001 From: IAvecilla Date: Tue, 10 Sep 2024 15:28:30 -0300 Subject: [PATCH 3/5] Add preprocessed evm interpreter --- system-contracts/contracts/EvmInterpreter.yul | 4 ---- 1 file changed, 4 deletions(-) diff --git a/system-contracts/contracts/EvmInterpreter.yul b/system-contracts/contracts/EvmInterpreter.yul index 33f672759..ea10abe3d 100644 --- a/system-contracts/contracts/EvmInterpreter.yul +++ b/system-contracts/contracts/EvmInterpreter.yul @@ -2994,8 +2994,6 @@ object "EVMInterpreter" { offset, sp := popStackItemWithoutCheck(sp) size, sp := popStackItemWithoutCheck(sp) - ensureAcceptableMemLocation(offset) - ensureAcceptableMemLocation(size) checkOverflow(offset,size, evmGasLeft) checkMemOverflowByOffset(add(offset, size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) @@ -5962,8 +5960,6 @@ object "EVMInterpreter" { offset, sp := popStackItemWithoutCheck(sp) size, sp := popStackItemWithoutCheck(sp) - ensureAcceptableMemLocation(offset) - ensureAcceptableMemLocation(size) checkOverflow(offset,size, evmGasLeft) checkMemOverflowByOffset(add(offset, size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) From f5dae8e25496bc80e89ea98cfa4768fb6e095e68 Mon Sep 17 00:00:00 2001 From: IAvecilla Date: Fri, 13 Sep 2024 12:33:51 -0300 Subject: [PATCH 4/5] Remove unnecesary memory checks --- system-contracts/contracts/EvmInterpreter.yul | 12 ++++++++---- .../evm-interpreter/EvmInterpreterLoop.template.yul | 6 ++++-- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/system-contracts/contracts/EvmInterpreter.yul b/system-contracts/contracts/EvmInterpreter.yul index 85a575b02..dca506aab 100644 --- a/system-contracts/contracts/EvmInterpreter.yul +++ b/system-contracts/contracts/EvmInterpreter.yul @@ -2966,7 +2966,8 @@ object "EVMInterpreter" { 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 } @@ -3006,7 +3007,8 @@ object "EVMInterpreter" { checkMemOverflowByOffset(add(offset, size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) - checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft) + + // 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) @@ -5940,7 +5942,8 @@ object "EVMInterpreter" { 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 } @@ -5980,7 +5983,8 @@ object "EVMInterpreter" { checkMemOverflowByOffset(add(offset, size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) - checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft) + + // 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/EvmInterpreterLoop.template.yul b/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul index c6fba46c4..eb2cac4ec 100644 --- a/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul +++ b/system-contracts/evm-interpreter/EvmInterpreterLoop.template.yul @@ -1406,7 +1406,8 @@ for { } true { } { 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 } @@ -1446,7 +1447,8 @@ for { } true { } { checkMemOverflowByOffset(add(offset, size), evmGasLeft) evmGasLeft := chargeGas(evmGasLeft,expandMemory(add(offset,size))) - checkOverflow(offset,MEM_OFFSET_INNER(), evmGasLeft) + + // 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) From 4f5a8843b3f2328c47769c2cf0af1fc1b9c6f6f7 Mon Sep 17 00:00:00 2001 From: IAvecilla Date: Fri, 13 Sep 2024 12:56:38 -0300 Subject: [PATCH 5/5] Update hash for evm interpreter --- system-contracts/SystemContractsHashes.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system-contracts/SystemContractsHashes.json b/system-contracts/SystemContractsHashes.json index b7f8725c2..c7357d0c1 100644 --- 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",