From ca7b3564038263ab80dc7d02e4c9341302dbda96 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Thu, 9 Mar 2023 22:08:18 +0800 Subject: [PATCH 1/9] Truncate memory offset of Word to Uint64 for Call opcodes when memory length is zero. --- bus-mapping/src/evm/opcodes/callop.rs | 4 +- .../src/evm_circuit/execution/callop.rs | 47 ++++++++++++------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/callop.rs b/bus-mapping/src/evm/opcodes/callop.rs index 1786bf2f7b..ef40082378 100644 --- a/bus-mapping/src/evm/opcodes/callop.rs +++ b/bus-mapping/src/evm/opcodes/callop.rs @@ -31,9 +31,9 @@ impl Opcode for CallOpcode { let geth_step = &geth_steps[0]; let mut exec_step = state.new_step(geth_step)?; - let args_offset = geth_step.stack.nth_last(N_ARGS - 4)?.as_usize(); + let args_offset = geth_step.stack.nth_last(N_ARGS - 4)?.low_u64() as usize; let args_length = geth_step.stack.nth_last(N_ARGS - 3)?.as_usize(); - let ret_offset = geth_step.stack.nth_last(N_ARGS - 2)?.as_usize(); + let ret_offset = geth_step.stack.nth_last(N_ARGS - 2)?.low_u64() as usize; let ret_length = geth_step.stack.nth_last(N_ARGS - 1)?.as_usize(); // we need to keep the memory until parse_call complete diff --git a/zkevm-circuits/src/evm_circuit/execution/callop.rs b/zkevm-circuits/src/evm_circuit/execution/callop.rs index 7aae484769..34d0aaefc2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/callop.rs +++ b/zkevm-circuits/src/evm_circuit/execution/callop.rs @@ -691,31 +691,31 @@ mod test { }, // With memory expansion Stack { - cd_offset: 64, + cd_offset: 64.into(), cd_length: 320, - rd_offset: 0, + rd_offset: Word::zero(), rd_length: 32, ..Default::default() }, Stack { - cd_offset: 0, + cd_offset: Word::zero(), cd_length: 32, - rd_offset: 64, + rd_offset: 64.into(), rd_length: 320, ..Default::default() }, Stack { - cd_offset: 0xFFFFFF, + cd_offset: 0xFFFFFF.into(), cd_length: 0, - rd_offset: 0xFFFFFF, + rd_offset: 0xFFFFFF.into(), rd_length: 0, ..Default::default() }, // With memory expansion and value Stack { - cd_offset: 64, + cd_offset: 64.into(), cd_length: 320, - rd_offset: 0, + rd_offset: 0.into(), rd_length: 32, value: Word::from(10).pow(18.into()), ..Default::default() @@ -732,13 +732,28 @@ mod test { } } + #[test] + fn callop_with_overflow_offset_and_zero_length() { + let stack = Stack { + cd_offset: Word::MAX, + cd_length: 0, + rd_offset: Word::MAX, + rd_length: 0, + ..Default::default() + }; + + TEST_CALL_OPCODES + .iter() + .for_each(|opcode| test_ok(caller(opcode, stack, true), callee(bytecode! {}))); + } + #[derive(Clone, Copy, Debug, Default)] struct Stack { gas: u64, value: Word, - cd_offset: u64, + cd_offset: Word, cd_length: u64, - rd_offset: u64, + rd_offset: Word, rd_length: u64, } @@ -765,9 +780,9 @@ mod test { // Call twice for testing both cold and warm access let mut bytecode = bytecode! { PUSH32(Word::from(stack.rd_length)) - PUSH32(Word::from(stack.rd_offset)) + PUSH32(stack.rd_offset) PUSH32(Word::from(stack.cd_length)) - PUSH32(Word::from(stack.cd_offset)) + PUSH32(stack.cd_offset) }; if is_call_or_callcode { bytecode.push(32, stack.value); @@ -777,9 +792,9 @@ mod test { PUSH32(Word::from(stack.gas)) .write_op(*opcode) PUSH32(Word::from(stack.rd_length)) - PUSH32(Word::from(stack.rd_offset)) + PUSH32(stack.rd_offset) PUSH32(Word::from(stack.cd_length)) - PUSH32(Word::from(stack.cd_offset)) + PUSH32(stack.cd_offset) }); if is_call_or_callcode { bytecode.push(32, stack.value); @@ -807,9 +822,9 @@ mod test { let mut bytecode = bytecode! { PUSH32(Word::from(stack.rd_length)) - PUSH32(Word::from(stack.rd_offset)) + PUSH32(stack.rd_offset) PUSH32(Word::from(stack.cd_length)) - PUSH32(Word::from(stack.cd_offset)) + PUSH32(stack.cd_offset) }; if is_call_or_callcode { bytecode.push(32, stack.value); From fc4ab80b39f998344af6ad4c34b2a954a41b6576 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 09:46:00 +0800 Subject: [PATCH 2/9] Truncate memory offset for `CALLDATACOPY`. --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 4 ++- .../src/evm_circuit/execution/calldatacopy.rs | 30 +++++++++++-------- .../src/evm_circuit/execution/callop.rs | 2 +- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 238b17c993..4573f47a7b 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -105,7 +105,9 @@ fn gen_copy_event( let call_data_offset = state.call()?.call_data_offset; let call_data_length = state.call()?.call_data_length; - let dst_addr = memory_offset.as_u64(); + // Get low Uint64 for memory offset. + // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/instructions.go#L299 + let dst_addr = memory_offset.low_u64(); let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); // Reset start offset to end offset if overflow. diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 219941902b..70ff0b0778 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -263,9 +263,9 @@ mod test { fn test_root_ok( call_data_length: usize, - memory_offset: usize, length: usize, data_offset: Word, + memory_offset: Word, ) { let bytecode = bytecode! { PUSH32(length) @@ -302,9 +302,9 @@ mod test { fn test_internal_ok( call_data_offset: usize, call_data_length: usize, - dst_offset: usize, length: usize, data_offset: Word, + dst_offset: Word, ) { let (addr_a, addr_b) = (mock::MOCK_ACCOUNTS[0], mock::MOCK_ACCOUNTS[1]); @@ -343,31 +343,37 @@ mod test { #[test] fn calldatacopy_gadget_simple() { - test_root_ok(0x40, 0x40, 10, 0x00.into()); - test_internal_ok(0x40, 0x40, 0xA0, 10, 0x10.into()); + test_ok_root(0x40, 10, 0x00.into(), 0x40.into()); + test_ok_internal(0x40, 0x40, 10, 0x10.into(), 0xA0.into()); } #[test] fn calldatacopy_gadget_large() { - test_root_ok(0x204, 0x103, 0x101, 0x102.into()); - test_internal_ok(0x30, 0x204, 0x103, 0x101, 0x102.into()); + test_ok_root(0x204, 0x101, 0x102.into(), 0x103.into()); + test_ok_internal(0x30, 0x204, 0x101, 0x102.into(), 0x103.into()); } #[test] fn calldatacopy_gadget_out_of_bound() { - test_root_ok(0x40, 0x40, 40, 0x20.into()); - test_internal_ok(0x40, 0x20, 0xA0, 10, 0x28.into()); + test_ok_root(0x40, 40, 0x20.into(), 0x40.into()); + test_ok_internal(0x40, 0x20, 10, 0x28.into(), 0xA0.into()); } #[test] fn calldatacopy_gadget_zero_length() { - test_root_ok(0x40, 0x40, 0, 0x00.into()); - test_internal_ok(0x40, 0x40, 0xA0, 0, 0x10.into()); + test_ok_root(0x40, 0, 0x00.into(), 0x40.into()); + test_ok_internal(0x40, 0x40, 0, 0x10.into(), 0xA0.into()); } #[test] fn calldatacopy_gadget_data_offset_overflow() { - test_root_ok(0x40, 0x40, 0, Word::MAX); - test_internal_ok(0x40, 0x40, 0xA0, 0, Word::MAX); + test_ok_root(0x40, 10, Word::MAX, 0x40.into()); + test_ok_internal(0x40, 0x40, 10, Word::MAX, 0xA0.into()); + } + + #[test] + fn calldatacopy_gadget_overflow_memory_offset_and_zero_length() { + test_ok_root(0x40, 0, 0x40.into(), Word::MAX); + test_ok_internal(0x40, 0x40, 0, 0x10.into(), Word::MAX); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/callop.rs b/zkevm-circuits/src/evm_circuit/execution/callop.rs index 34d0aaefc2..f5a78bf06a 100644 --- a/zkevm-circuits/src/evm_circuit/execution/callop.rs +++ b/zkevm-circuits/src/evm_circuit/execution/callop.rs @@ -733,7 +733,7 @@ mod test { } #[test] - fn callop_with_overflow_offset_and_zero_length() { + fn callop_overflow_offset_and_zero_length() { let stack = Stack { cd_offset: Word::MAX, cd_length: 0, From baa64098a66d49b9ed20d7117740e256157789bd Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 09:58:38 +0800 Subject: [PATCH 3/9] Truncate memory offset for `CODECOPY`. --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 2 +- bus-mapping/src/evm/opcodes/codecopy.rs | 4 +++- .../src/evm_circuit/execution/codecopy.rs | 19 ++++++++++++------- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 4573f47a7b..d383bac197 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -105,7 +105,7 @@ fn gen_copy_event( let call_data_offset = state.call()?.call_data_offset; let call_data_length = state.call()?.call_data_length; - // Get low Uint64 for memory offset. + // Get low Uint64 of offset. // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/instructions.go#L299 let dst_addr = memory_offset.low_u64(); let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); diff --git a/bus-mapping/src/evm/opcodes/codecopy.rs b/bus-mapping/src/evm/opcodes/codecopy.rs index 578f1c733f..f5d6d9d854 100644 --- a/bus-mapping/src/evm/opcodes/codecopy.rs +++ b/bus-mapping/src/evm/opcodes/codecopy.rs @@ -78,7 +78,9 @@ fn gen_copy_event( let bytecode: Bytecode = state.code(code_hash)?.into(); let code_size = bytecode.code.len() as u64; - let dst_addr = dst_offset.as_u64(); + // Get low Uint64 of offset. + // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/instructions.go#L367 + let dst_addr = dst_offset.low_u64(); let src_addr_end = code_size; // Reset start offset to end offset if overflow. diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index 020a88fedf..f47fef2936 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -200,7 +200,7 @@ mod tests { use eth_types::{bytecode, Word}; use mock::TestContext; - fn test_ok(code_offset: Word, memory_offset: usize, size: usize, large: bool) { + fn test_ok(code_offset: Word, memory_offset: Word, size: usize, large: bool) { let mut code = bytecode! {}; if large { for _ in 0..size { @@ -210,7 +210,7 @@ mod tests { let tail = bytecode! { PUSH32(Word::from(size)) PUSH32(code_offset) - PUSH32(Word::from(memory_offset)) + PUSH32(memory_offset) CODECOPY STOP }; @@ -224,18 +224,23 @@ mod tests { #[test] fn codecopy_gadget_simple() { - test_ok(0x00.into(), 0x00, 0x20, false); - test_ok(0x30.into(), 0x20, 0x30, false); - test_ok(0x20.into(), 0x10, 0x42, false); + test_ok(0x00.into(), 0x00.into(), 0x20, false); + test_ok(0x30.into(), 0x20.into(), 0x30, false); + test_ok(0x20.into(), 0x10.into(), 0x42, false); } #[test] fn codecopy_gadget_large() { - test_ok(0x102.into(), 0x103, 0x101, true); + test_ok(0x102.into(), 0x103.into(), 0x101, true); } #[test] fn codecopy_gadget_code_offset_overflow() { - test_ok(Word::MAX, 0x103, 0x101, true); + test_ok(Word::MAX, 0x103.into(), 0x101, true); + } + + #[test] + fn codecopy_gadget_overflow_memory_offset_and_zero_size() { + test_ok(0x102.into(), Word::MAX, 0, false); } } From 898bb7e175cdbf33be3f24f73ea16707f36c2bb3 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 10:09:10 +0800 Subject: [PATCH 4/9] Truncate memory offset for `EXTCODECOPY`. --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 1 - bus-mapping/src/evm/opcodes/codecopy.rs | 1 - bus-mapping/src/evm/opcodes/extcodecopy.rs | 3 +- .../src/evm_circuit/execution/extcodecopy.rs | 56 ++++++++++++++----- 4 files changed, 43 insertions(+), 18 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index d383bac197..3bad4e3caf 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -106,7 +106,6 @@ fn gen_copy_event( let call_data_length = state.call()?.call_data_length; // Get low Uint64 of offset. - // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/instructions.go#L299 let dst_addr = memory_offset.low_u64(); let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); diff --git a/bus-mapping/src/evm/opcodes/codecopy.rs b/bus-mapping/src/evm/opcodes/codecopy.rs index f5d6d9d854..48d99621ee 100644 --- a/bus-mapping/src/evm/opcodes/codecopy.rs +++ b/bus-mapping/src/evm/opcodes/codecopy.rs @@ -79,7 +79,6 @@ fn gen_copy_event( let code_size = bytecode.code.len() as u64; // Get low Uint64 of offset. - // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/instructions.go#L367 let dst_addr = dst_offset.low_u64(); let src_addr_end = code_size; diff --git a/bus-mapping/src/evm/opcodes/extcodecopy.rs b/bus-mapping/src/evm/opcodes/extcodecopy.rs index 8160bc9455..4f896a431b 100644 --- a/bus-mapping/src/evm/opcodes/extcodecopy.rs +++ b/bus-mapping/src/evm/opcodes/extcodecopy.rs @@ -137,7 +137,8 @@ fn gen_copy_event( }; let code_size = bytecode.code.len() as u64; - let dst_addr = dst_offset.as_u64(); + // Get low Uint64 of offset. + let dst_addr = dst_offset.low_u64(); let src_addr_end = code_size; // Reset start offset to end offset if overflow. diff --git a/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs b/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs index fcecb36866..ad0d793837 100644 --- a/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs @@ -252,7 +252,7 @@ mod test { fn test_ok( external_account: Option, code_offset: Word, - memory_offset: usize, + memory_offset: Word, length: usize, is_warm: bool, ) { @@ -309,8 +309,8 @@ mod test { #[test] fn extcodecopy_empty_account() { - test_ok(None, 0x00.into(), 0x00, 0x36, true); // warm account - test_ok(None, 0x00.into(), 0x00, 0x36, false); // cold account + test_ok(None, Word::zero(), Word::zero(), 0x36, true); // warm account + test_ok(None, Word::zero(), Word::zero(), 0x36, false); // cold account } #[test] @@ -321,8 +321,8 @@ mod test { code: Bytes::from([10, 40]), ..Default::default() }), - 0x00.into(), - 0x00, + Word::zero(), + Word::zero(), 0x36, true, ); // warm account @@ -333,8 +333,8 @@ mod test { code: Bytes::from([10, 40]), ..Default::default() }), - 0x00.into(), - 0x00, + Word::zero(), + Word::zero(), 0x36, false, ); // cold account @@ -348,8 +348,8 @@ mod test { code: Bytes::from(rand_bytes_array::<256>()), ..Default::default() }), - 0x00.into(), - 0x00, + Word::zero(), + Word::zero(), 0x36, true, ); @@ -359,8 +359,8 @@ mod test { code: Bytes::from(rand_bytes_array::<256>()), ..Default::default() }), - 0x00.into(), - 0x00, + Word::zero(), + Word::zero(), 0x36, false, ); @@ -375,7 +375,7 @@ mod test { ..Default::default() }), 0x20.into(), - 0x00, + Word::zero(), 0x104, true, ); @@ -386,7 +386,7 @@ mod test { ..Default::default() }), 0x20.into(), - 0x00, + Word::zero(), 0x104, false, ); @@ -401,7 +401,7 @@ mod test { ..Default::default() }), Word::MAX, - 0x00, + Word::zero(), 0x36, true, ); @@ -412,9 +412,35 @@ mod test { ..Default::default() }), Word::MAX, - 0x00, + Word::zero(), 0x36, false, ); } + + #[test] + fn extcodecopy_overflow_memory_offset_and_zero_length() { + test_ok( + Some(Account { + address: *EXTERNAL_ADDRESS, + code: Bytes::from(rand_bytes_array::<256>()), + ..Default::default() + }), + 0x20.into(), + Word::MAX, + 0, + true, + ); + test_ok( + Some(Account { + address: *EXTERNAL_ADDRESS, + code: Bytes::from(rand_bytes_array::<256>()), + ..Default::default() + }), + 0x20.into(), + Word::MAX, + 0, + true, + ); + } } From b291b2445b620ab94ad8133316a5df533472c354 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 10:31:04 +0800 Subject: [PATCH 5/9] Truncate memory offset for `RETURNDATACOPY`. --- bus-mapping/src/evm/opcodes/returndatacopy.rs | 3 +- .../evm_circuit/execution/returndatacopy.rs | 36 ++++++++++++++----- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/returndatacopy.rs b/bus-mapping/src/evm/opcodes/returndatacopy.rs index f1a14a4b16..2cb3ab9028 100644 --- a/bus-mapping/src/evm/opcodes/returndatacopy.rs +++ b/bus-mapping/src/evm/opcodes/returndatacopy.rs @@ -123,7 +123,8 @@ fn gen_copy_event( state: &mut CircuitInputStateRef, geth_step: &GethExecStep, ) -> Result { - let dst_addr = geth_step.stack.nth_last(0)?.as_u64(); + // Get low Uint64 of offset. + let dst_addr = geth_step.stack.nth_last(0)?.low_u64(); let data_offset = geth_step.stack.nth_last(1)?.as_u64(); let length = geth_step.stack.nth_last(2)?.as_u64(); diff --git a/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs index 4a16767f58..bdaba7e537 100644 --- a/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs @@ -275,9 +275,9 @@ mod test { fn test_ok_internal( return_data_offset: usize, return_data_size: usize, - dest_offset: usize, - offset: usize, size: usize, + offset: usize, + dest_offset: Word, ) { let (addr_a, addr_b) = (mock::MOCK_ACCOUNTS[0], mock::MOCK_ACCOUNTS[1]); @@ -330,38 +330,56 @@ mod test { #[test] fn returndatacopy_gadget_do_nothing() { - test_ok_internal(0x00, 0x02, 0x10, 0x00, 0x00); + test_ok_internal(0, 2, 0, 0, 0x10.into()); } #[test] fn returndatacopy_gadget_simple() { - test_ok_internal(0x00, 0x02, 0x10, 0x00, 0x02); + test_ok_internal(0, 2, 2, 0, 0x10.into()); } #[test] fn returndatacopy_gadget_large() { - test_ok_internal(0x00, 0x20, 0x20, 0x00, 0x20); + test_ok_internal(0, 0x20, 0x20, 0, 0x20.into()); } #[test] fn returndatacopy_gadget_large_partial() { - test_ok_internal(0x00, 0x20, 0x20, 0x10, 0x10); + test_ok_internal(0, 0x20, 0x10, 0x10, 0x20.into()); } #[test] fn returndatacopy_gadget_zero_length() { - test_ok_internal(0x00, 0x00, 0x20, 0x00, 0x00); + test_ok_internal(0, 0, 0, 0, 0x20.into()); } #[test] fn returndatacopy_gadget_long_length() { // rlc value matters only if length > 255, i.e., size.cells.len() > 1 - test_ok_internal(0x00, 0x200, 0x20, 0x00, 0x150); + test_ok_internal(0, 0x200, 0x150, 0, 0x20.into()); } #[test] fn returndatacopy_gadget_big_offset() { // rlc value matters only if length > 255, i.e., size.cells.len() > 1 - test_ok_internal(0x200, 0x200, 0x200, 0x00, 0x150); + test_ok_internal(0x200, 0x200, 0x150, 0, 0x200.into()); + } + + #[test] + fn returndatacopy_gadget_overflow_offset_and_zero_length() { + test_ok_internal(0, 0x20, 0, 0x20, Word::MAX); } + + // TODO: Add negative cases for out-of-bound and out-of-gas + // #[test] + // #[should_panic] + // fn returndatacopy_gadget_out_of_bound() { + // test_ok_internal(0, 0x10, 0x10, 0x10, 0x20.into()); + // } + + // #[test] + // #[should_panic] + // fn returndatacopy_gadget_out_of_gas() { + // test_ok_internal(0, 0x10, 0x10, 0, 0x2000000.into()); + // } } From 77858d5b1ecfad7dcf869f54038bc22c6e374f32 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 16:17:50 +0800 Subject: [PATCH 6/9] Truncate memory offset for `RETURN` and `REVERT`. --- bus-mapping/src/evm/opcodes/return_revert.rs | 3 ++- .../evm_circuit/execution/return_revert.rs | 19 +++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return_revert.rs b/bus-mapping/src/evm/opcodes/return_revert.rs index d3bd76fd2b..12a46d03b0 100644 --- a/bus-mapping/src/evm/opcodes/return_revert.rs +++ b/bus-mapping/src/evm/opcodes/return_revert.rs @@ -40,7 +40,8 @@ impl Opcode for ReturnRevert { call.is_success.to_word(), ); - let offset = offset.as_usize(); + // Get low Uint64 of offset. + let offset = offset.low_u64() as usize; let length = length.as_usize(); // Case A in the spec. diff --git a/zkevm-circuits/src/evm_circuit/execution/return_revert.rs b/zkevm-circuits/src/evm_circuit/execution/return_revert.rs index e584a2d8d5..cad355c2bc 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return_revert.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return_revert.rs @@ -355,16 +355,16 @@ mod test { const CALLEE_ADDRESS: Address = Address::repeat_byte(0xff); const CALLER_ADDRESS: Address = Address::repeat_byte(0x34); - fn callee_bytecode(is_return: bool, offset: u64, length: u64) -> Bytecode { - let memory_bytes = [0x60; 10]; + fn callee_bytecode(is_return: bool, offset: u128, length: u64) -> Bytecode { + let memory_bytes = [0x60; 6]; let memory_address = 0; let memory_value = Word::from_big_endian(&memory_bytes); let mut code = bytecode! { - PUSH10(memory_value) + PUSH6(memory_value) PUSH1(memory_address) MSTORE PUSH2(length) - PUSH2(32u64 - u64::try_from(memory_bytes.len()).unwrap() + offset) + PUSH17(Word::from(offset) + 32 - memory_bytes.len()) }; code.write_op(if is_return { OpcodeId::RETURN @@ -651,4 +651,15 @@ mod test { let text_ctx = TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode).unwrap(); CircuitTestBuilder::new_from_test_ctx(text_ctx).run(); } + + #[test] + fn test_return_overflow_offset_and_zero_length() { + for is_return in [true, false] { + let code = callee_bytecode(is_return, u128::MAX, 0); + CircuitTestBuilder::new_from_test_ctx( + TestContext::<2, 1>::simple_ctx_with_bytecode(code).unwrap(), + ) + .run(); + } + } } From 9d24ae1754179ea9088161843eed7990cb6bb90d Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 16:48:19 +0800 Subject: [PATCH 7/9] Truncate memory offset for `SHA3` (`KECCAK256`). --- bus-mapping/src/evm/opcodes/sha3.rs | 6 +++--- zkevm-circuits/src/evm_circuit/execution/sha3.rs | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/sha3.rs b/bus-mapping/src/evm/opcodes/sha3.rs index 65f4408817..c34e48a7db 100644 --- a/bus-mapping/src/evm/opcodes/sha3.rs +++ b/bus-mapping/src/evm/opcodes/sha3.rs @@ -40,7 +40,7 @@ impl Opcode for Sha3 { let memory = state .call_ctx()? .memory - .read_chunk(offset.as_usize().into(), size.as_usize().into()); + .read_chunk(offset.low_u64().into(), size.as_usize().into()); // keccak-256 hash of the given data in memory. let sha3 = keccak256(&memory); @@ -65,8 +65,8 @@ impl Opcode for Sha3 { state.push_copy( &mut exec_step, CopyEvent { - src_addr: offset.as_u64(), - src_addr_end: offset.as_u64() + size.as_u64(), + src_addr: offset.low_u64(), + src_addr_end: offset.low_u64().checked_add(size.as_u64()).unwrap_or(u64::MAX), src_type: CopyDataType::Memory, src_id: NumberOrHash::Number(call_id), dst_addr: 0, diff --git a/zkevm-circuits/src/evm_circuit/execution/sha3.rs b/zkevm-circuits/src/evm_circuit/execution/sha3.rs index 6d948f1880..85ee66afcb 100644 --- a/zkevm-circuits/src/evm_circuit/execution/sha3.rs +++ b/zkevm-circuits/src/evm_circuit/execution/sha3.rs @@ -192,4 +192,18 @@ mod tests { test_ok(Sha3CodeGen::mem_eq_size(0x303, 0x404)); test_ok(Sha3CodeGen::mem_gt_size(0x404, 0x505)); } + + #[test] + fn sha3_gadget_overflow_offset_and_zero_size() { + let bytecode = bytecode! { + PUSH1(0) + PUSH32(Word::MAX) + SHA3 + }; + + CircuitTestBuilder::new_from_test_ctx( + TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode).unwrap(), + ) + .run(); + } } From 81e589872c29a051f92177297c8644c6430edd95 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 10 Mar 2023 17:16:03 +0800 Subject: [PATCH 8/9] Truncate memory offset for `CREATE` (and `CREATE2`). --- bus-mapping/src/circuit_input_builder.rs | 15 ++++++++---- bus-mapping/src/evm/opcodes/create.rs | 3 ++- bus-mapping/src/evm/opcodes/sha3.rs | 5 +++- .../src/evm_circuit/execution/calldatacopy.rs | 24 +++++++++---------- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs index 057458d073..11e676d561 100644 --- a/bus-mapping/src/circuit_input_builder.rs +++ b/bus-mapping/src/circuit_input_builder.rs @@ -366,10 +366,17 @@ pub fn get_create_init_code<'a>( call_ctx: &'a CallContext, step: &GethExecStep, ) -> Result<&'a [u8], Error> { - let offset = step.stack.nth_last(1)?; - let length = step.stack.nth_last(2)?; - Ok(&call_ctx.memory.0 - [offset.low_u64() as usize..(offset.low_u64() + length.low_u64()) as usize]) + let offset = step.stack.nth_last(1)?.low_u64() as usize; + let length = step.stack.nth_last(2)?.as_usize(); + + let mem_len = call_ctx.memory.0.len(); + if offset >= mem_len { + return Ok(&[]); + } + + let offset_end = offset.checked_add(length).unwrap_or(mem_len); + + Ok(&call_ctx.memory.0[offset..offset_end]) } /// Retrieve the memory offset and length of call. diff --git a/bus-mapping/src/evm/opcodes/create.rs b/bus-mapping/src/evm/opcodes/create.rs index d40fa678d3..444f1ce5fe 100644 --- a/bus-mapping/src/evm/opcodes/create.rs +++ b/bus-mapping/src/evm/opcodes/create.rs @@ -18,7 +18,8 @@ impl Opcode for DummyCreate { // TODO: replace dummy create here let geth_step = &geth_steps[0]; - let offset = geth_step.stack.nth_last(1)?.as_usize(); + // Get low Uint64 of offset. + let offset = geth_step.stack.nth_last(1)?.low_u64() as usize; let length = geth_step.stack.nth_last(2)?.as_usize(); let curr_memory_word_size = (state.call_ctx()?.memory.len() as u64) / 32; diff --git a/bus-mapping/src/evm/opcodes/sha3.rs b/bus-mapping/src/evm/opcodes/sha3.rs index c34e48a7db..418dd871dc 100644 --- a/bus-mapping/src/evm/opcodes/sha3.rs +++ b/bus-mapping/src/evm/opcodes/sha3.rs @@ -66,7 +66,10 @@ impl Opcode for Sha3 { &mut exec_step, CopyEvent { src_addr: offset.low_u64(), - src_addr_end: offset.low_u64().checked_add(size.as_u64()).unwrap_or(u64::MAX), + src_addr_end: offset + .low_u64() + .checked_add(size.as_u64()) + .unwrap_or(u64::MAX), src_type: CopyDataType::Memory, src_id: NumberOrHash::Number(call_id), dst_addr: 0, diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 70ff0b0778..6d1fb475cc 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -343,37 +343,37 @@ mod test { #[test] fn calldatacopy_gadget_simple() { - test_ok_root(0x40, 10, 0x00.into(), 0x40.into()); - test_ok_internal(0x40, 0x40, 10, 0x10.into(), 0xA0.into()); + test_root_ok(0x40, 10, 0x00.into(), 0x40.into()); + test_internal_ok(0x40, 0x40, 10, 0x10.into(), 0xA0.into()); } #[test] fn calldatacopy_gadget_large() { - test_ok_root(0x204, 0x101, 0x102.into(), 0x103.into()); - test_ok_internal(0x30, 0x204, 0x101, 0x102.into(), 0x103.into()); + test_root_ok(0x204, 0x101, 0x102.into(), 0x103.into()); + test_internal_ok(0x30, 0x204, 0x101, 0x102.into(), 0x103.into()); } #[test] fn calldatacopy_gadget_out_of_bound() { - test_ok_root(0x40, 40, 0x20.into(), 0x40.into()); - test_ok_internal(0x40, 0x20, 10, 0x28.into(), 0xA0.into()); + test_root_ok(0x40, 40, 0x20.into(), 0x40.into()); + test_internal_ok(0x40, 0x20, 10, 0x28.into(), 0xA0.into()); } #[test] fn calldatacopy_gadget_zero_length() { - test_ok_root(0x40, 0, 0x00.into(), 0x40.into()); - test_ok_internal(0x40, 0x40, 0, 0x10.into(), 0xA0.into()); + test_root_ok(0x40, 0, 0x00.into(), 0x40.into()); + test_internal_ok(0x40, 0x40, 0, 0x10.into(), 0xA0.into()); } #[test] fn calldatacopy_gadget_data_offset_overflow() { - test_ok_root(0x40, 10, Word::MAX, 0x40.into()); - test_ok_internal(0x40, 0x40, 10, Word::MAX, 0xA0.into()); + test_root_ok(0x40, 10, Word::MAX, 0x40.into()); + test_internal_ok(0x40, 0x40, 10, Word::MAX, 0xA0.into()); } #[test] fn calldatacopy_gadget_overflow_memory_offset_and_zero_length() { - test_ok_root(0x40, 0, 0x40.into(), Word::MAX); - test_ok_internal(0x40, 0x40, 0, 0x10.into(), Word::MAX); + test_root_ok(0x40, 0, 0x40.into(), Word::MAX); + test_internal_ok(0x40, 0x40, 0, 0x10.into(), Word::MAX); } } From 90304550ee7749f58ed83a61e5021a06aecbe686 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Wed, 17 May 2023 20:30:44 +0800 Subject: [PATCH 9/9] Add more comments. --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 3 ++- bus-mapping/src/evm/opcodes/callop.rs | 2 ++ bus-mapping/src/evm/opcodes/codecopy.rs | 3 ++- bus-mapping/src/evm/opcodes/create.rs | 3 ++- bus-mapping/src/evm/opcodes/extcodecopy.rs | 3 ++- bus-mapping/src/evm/opcodes/return_revert.rs | 3 ++- bus-mapping/src/evm/opcodes/returndatacopy.rs | 3 ++- bus-mapping/src/evm/opcodes/sha3.rs | 2 ++ .../src/evm_circuit/execution/returndatacopy.rs | 13 ------------- zkevm-circuits/src/evm_circuit/execution/sha3.rs | 3 ++- 10 files changed, 18 insertions(+), 20 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 3bad4e3caf..eb724c7e5e 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -105,7 +105,8 @@ fn gen_copy_event( let call_data_offset = state.call()?.call_data_offset; let call_data_length = state.call()?.call_data_length; - // Get low Uint64 of offset. + // Get low Uint64 of memory offset to generate copy steps. Since memory + // offset could be Uint64 overflow if memory length is zero. let dst_addr = memory_offset.low_u64(); let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); diff --git a/bus-mapping/src/evm/opcodes/callop.rs b/bus-mapping/src/evm/opcodes/callop.rs index ef40082378..b4fbdef3ec 100644 --- a/bus-mapping/src/evm/opcodes/callop.rs +++ b/bus-mapping/src/evm/opcodes/callop.rs @@ -31,6 +31,8 @@ impl Opcode for CallOpcode { let geth_step = &geth_steps[0]; let mut exec_step = state.new_step(geth_step)?; + // In offset and length are truncated to Uint64 for call opcodes as: + // let args_offset = geth_step.stack.nth_last(N_ARGS - 4)?.low_u64() as usize; let args_length = geth_step.stack.nth_last(N_ARGS - 3)?.as_usize(); let ret_offset = geth_step.stack.nth_last(N_ARGS - 2)?.low_u64() as usize; diff --git a/bus-mapping/src/evm/opcodes/codecopy.rs b/bus-mapping/src/evm/opcodes/codecopy.rs index 48d99621ee..372c8d22af 100644 --- a/bus-mapping/src/evm/opcodes/codecopy.rs +++ b/bus-mapping/src/evm/opcodes/codecopy.rs @@ -78,7 +78,8 @@ fn gen_copy_event( let bytecode: Bytecode = state.code(code_hash)?.into(); let code_size = bytecode.code.len() as u64; - // Get low Uint64 of offset. + // Get low Uint64 of offset to generate copy steps. Since offset could be + // Uint64 overflow if length is zero. let dst_addr = dst_offset.low_u64(); let src_addr_end = code_size; diff --git a/bus-mapping/src/evm/opcodes/create.rs b/bus-mapping/src/evm/opcodes/create.rs index 444f1ce5fe..2f6d40d409 100644 --- a/bus-mapping/src/evm/opcodes/create.rs +++ b/bus-mapping/src/evm/opcodes/create.rs @@ -18,7 +18,8 @@ impl Opcode for DummyCreate { // TODO: replace dummy create here let geth_step = &geth_steps[0]; - // Get low Uint64 of offset. + // Get low Uint64 of offset to generate copy steps. Since offset could + // be Uint64 overflow if length is zero. let offset = geth_step.stack.nth_last(1)?.low_u64() as usize; let length = geth_step.stack.nth_last(2)?.as_usize(); diff --git a/bus-mapping/src/evm/opcodes/extcodecopy.rs b/bus-mapping/src/evm/opcodes/extcodecopy.rs index 4f896a431b..bd0dc455de 100644 --- a/bus-mapping/src/evm/opcodes/extcodecopy.rs +++ b/bus-mapping/src/evm/opcodes/extcodecopy.rs @@ -137,7 +137,8 @@ fn gen_copy_event( }; let code_size = bytecode.code.len() as u64; - // Get low Uint64 of offset. + // Get low Uint64 of offset to generate copy steps. Since offset could be + // Uint64 overflow if length is zero. let dst_addr = dst_offset.low_u64(); let src_addr_end = code_size; diff --git a/bus-mapping/src/evm/opcodes/return_revert.rs b/bus-mapping/src/evm/opcodes/return_revert.rs index 12a46d03b0..d81d63b021 100644 --- a/bus-mapping/src/evm/opcodes/return_revert.rs +++ b/bus-mapping/src/evm/opcodes/return_revert.rs @@ -40,7 +40,8 @@ impl Opcode for ReturnRevert { call.is_success.to_word(), ); - // Get low Uint64 of offset. + // Get low Uint64 of offset to generate copy steps. Since offset could + // be Uint64 overflow if length is zero. let offset = offset.low_u64() as usize; let length = length.as_usize(); diff --git a/bus-mapping/src/evm/opcodes/returndatacopy.rs b/bus-mapping/src/evm/opcodes/returndatacopy.rs index 2cb3ab9028..a7830523d5 100644 --- a/bus-mapping/src/evm/opcodes/returndatacopy.rs +++ b/bus-mapping/src/evm/opcodes/returndatacopy.rs @@ -123,7 +123,8 @@ fn gen_copy_event( state: &mut CircuitInputStateRef, geth_step: &GethExecStep, ) -> Result { - // Get low Uint64 of offset. + // Get low Uint64 of destination offset to generate copy steps. Since it + // could be Uint64 overflow if length is zero. let dst_addr = geth_step.stack.nth_last(0)?.low_u64(); let data_offset = geth_step.stack.nth_last(1)?.as_u64(); let length = geth_step.stack.nth_last(2)?.as_u64(); diff --git a/bus-mapping/src/evm/opcodes/sha3.rs b/bus-mapping/src/evm/opcodes/sha3.rs index 418dd871dc..ca2cc1a430 100644 --- a/bus-mapping/src/evm/opcodes/sha3.rs +++ b/bus-mapping/src/evm/opcodes/sha3.rs @@ -40,6 +40,8 @@ impl Opcode for Sha3 { let memory = state .call_ctx()? .memory + // Get low Uint64 of offset to generate copy steps. Since offset + // could be Uint64 overflow if length is zero. .read_chunk(offset.low_u64().into(), size.as_usize().into()); // keccak-256 hash of the given data in memory. diff --git a/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs index bdaba7e537..aed699e68a 100644 --- a/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/returndatacopy.rs @@ -369,17 +369,4 @@ mod test { fn returndatacopy_gadget_overflow_offset_and_zero_length() { test_ok_internal(0, 0x20, 0, 0x20, Word::MAX); } - - // TODO: Add negative cases for out-of-bound and out-of-gas - // #[test] - // #[should_panic] - // fn returndatacopy_gadget_out_of_bound() { - // test_ok_internal(0, 0x10, 0x10, 0x10, 0x20.into()); - // } - - // #[test] - // #[should_panic] - // fn returndatacopy_gadget_out_of_gas() { - // test_ok_internal(0, 0x10, 0x10, 0, 0x2000000.into()); - // } } diff --git a/zkevm-circuits/src/evm_circuit/execution/sha3.rs b/zkevm-circuits/src/evm_circuit/execution/sha3.rs index 85ee66afcb..736b961dfa 100644 --- a/zkevm-circuits/src/evm_circuit/execution/sha3.rs +++ b/zkevm-circuits/src/evm_circuit/execution/sha3.rs @@ -158,6 +158,7 @@ impl ExecutionGadget for Sha3Gadget { mod tests { use crate::test_util::CircuitTestBuilder; use bus_mapping::{circuit_input_builder::CircuitsParams, evm::Sha3CodeGen}; + use eth_types::{bytecode, U256}; use mock::TestContext; fn test_ok(mut gen: Sha3CodeGen) { @@ -197,7 +198,7 @@ mod tests { fn sha3_gadget_overflow_offset_and_zero_size() { let bytecode = bytecode! { PUSH1(0) - PUSH32(Word::MAX) + PUSH32(U256::MAX) SHA3 };