From 77c16e4ae1bfa1187a38d8d9be3d9791b389a534 Mon Sep 17 00:00:00 2001 From: Akase Cho Date: Tue, 12 Sep 2023 14:25:10 +0800 Subject: [PATCH] fix unconstrained is_warm in sload/sstore (#1596) ### Description This PR fixes the soundness problem in the implementation of SLOAD and SSTORE opcodes by adding constraints to the `is_warm` cell. The problem is detailed in issue #1049. ### Issue Link #1049 ### Type of change - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update ### Contents - Update `bus-mapping/src/evm/opcodes/sload.rs` - Update `bus-mapping/src/evm/opcodes/sstore.rs` - Update `zkevm-circuits/src/evm_circuit/execution/sload.rs` - Update `zkevm-circuits/src/evm_circuit/execution/sstore.rs` ### Rationale The current implementation of SSTORE and SLOAD didn't constrain the `is_warm` cell, which could result in any value and lead to soundness problems. This PR adds constraints to the `is_warm` cell in order to ensure correctness. ### How Has This Been Tested? - --- bus-mapping/src/evm/opcodes/sload.rs | 13 ++++++++++++- bus-mapping/src/evm/opcodes/sstore.rs | 17 ++++++++++++++--- .../src/evm_circuit/execution/sload.rs | 10 ++++++++-- .../src/evm_circuit/execution/sstore.rs | 12 +++++++++--- 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/sload.rs b/bus-mapping/src/evm/opcodes/sload.rs index 8f891d50e0..9bd164db46 100644 --- a/bus-mapping/src/evm/opcodes/sload.rs +++ b/bus-mapping/src/evm/opcodes/sload.rs @@ -82,6 +82,17 @@ impl Opcode for Sload { // First stack write state.stack_write(&mut exec_step, stack_position, value)?; + state.push_op( + &mut exec_step, + RW::READ, + TxAccessListAccountStorageOp { + tx_id: state.tx_ctx.id(), + address: contract_addr, + key, + is_warm, + is_warm_prev: is_warm, + }, + ); state.push_op_reversible( &mut exec_step, TxAccessListAccountStorageOp { @@ -189,7 +200,7 @@ mod sload_tests { ); let access_list_op = &builder.block.container.tx_access_list_account_storage - [step.bus_mapping_instance[7].as_usize()]; + [step.bus_mapping_instance[8].as_usize()]; assert_eq!( (access_list_op.rw(), access_list_op.op()), ( diff --git a/bus-mapping/src/evm/opcodes/sstore.rs b/bus-mapping/src/evm/opcodes/sstore.rs index a5527aa72d..0b1eadeca5 100644 --- a/bus-mapping/src/evm/opcodes/sstore.rs +++ b/bus-mapping/src/evm/opcodes/sstore.rs @@ -1,10 +1,9 @@ use super::Opcode; use crate::{ circuit_input_builder::{CircuitInputStateRef, ExecStep}, - operation::{CallContextField, StorageOp, TxAccessListAccountStorageOp, TxRefundOp}, + operation::{CallContextField, StorageOp, TxAccessListAccountStorageOp, TxRefundOp, RW}, Error, }; - use eth_types::{GethExecStep, ToWord, Word}; /// Placeholder structure used to implement [`Opcode`] trait over it @@ -86,6 +85,17 @@ impl Opcode for Sstore { ), )?; + state.push_op( + &mut exec_step, + RW::READ, + TxAccessListAccountStorageOp { + tx_id: state.tx_ctx.id(), + address: contract_addr, + key, + is_warm, + is_warm_prev: is_warm, + }, + ); state.push_op_reversible( &mut exec_step, TxAccessListAccountStorageOp { @@ -250,7 +260,8 @@ mod sstore_tests { ) ) ); - let refund_op = &builder.block.container.tx_refund[step.bus_mapping_instance[9].as_usize()]; + let refund_op = + &builder.block.container.tx_refund[step.bus_mapping_instance[10].as_usize()]; assert_eq!( (refund_op.rw(), refund_op.op()), ( diff --git a/zkevm-circuits/src/evm_circuit/execution/sload.rs b/zkevm-circuits/src/evm_circuit/execution/sload.rs index 3bee3fa2c9..12ee777aa6 100644 --- a/zkevm-circuits/src/evm_circuit/execution/sload.rs +++ b/zkevm-circuits/src/evm_circuit/execution/sload.rs @@ -61,6 +61,12 @@ impl ExecutionGadget for SloadGadget { cb.stack_push(value.to_word()); let is_warm = cb.query_bool(); + cb.account_storage_access_list_read( + tx_id.expr(), + callee_address.to_word(), + key.to_word(), + Word::from_lo_unchecked(is_warm.expr()), + ); cb.account_storage_access_list_write( tx_id.expr(), callee_address.to_word(), @@ -72,7 +78,7 @@ impl ExecutionGadget for SloadGadget { let gas_cost = SloadGasGadget::construct(cb, is_warm.expr()).expr(); let step_state_transition = StepStateTransition { - rw_counter: Delta(8.expr()), + rw_counter: Delta(9.expr()), program_counter: Delta(1.expr()), reversible_write_counter: Delta(1.expr()), gas_left: Delta(-gas_cost), @@ -123,7 +129,7 @@ impl ExecutionGadget for SloadGadget { self.committed_value .assign_u256(region, offset, committed_value)?; - let (_, is_warm) = block.get_rws(step, 7).tx_access_list_value_pair(); + let (_, is_warm) = block.get_rws(step, 8).tx_access_list_value_pair(); self.is_warm .assign(region, offset, Value::known(F::from(is_warm as u64)))?; diff --git a/zkevm-circuits/src/evm_circuit/execution/sstore.rs b/zkevm-circuits/src/evm_circuit/execution/sstore.rs index 46f030b0a3..fe1fd45f0b 100644 --- a/zkevm-circuits/src/evm_circuit/execution/sstore.rs +++ b/zkevm-circuits/src/evm_circuit/execution/sstore.rs @@ -84,6 +84,12 @@ impl ExecutionGadget for SstoreGadget { ); let is_warm = cb.query_bool(); + cb.account_storage_access_list_read( + tx_id.expr(), + callee_address.to_word(), + key.to_word(), + Word::from_lo_unchecked(is_warm.expr()), + ); cb.account_storage_access_list_write( tx_id.expr(), callee_address.to_word(), @@ -129,7 +135,7 @@ impl ExecutionGadget for SstoreGadget { ); let step_state_transition = StepStateTransition { - rw_counter: Delta(10.expr()), + rw_counter: Delta(11.expr()), program_counter: Delta(1.expr()), stack_pointer: Delta(2.expr()), reversible_write_counter: Delta(3.expr()), @@ -190,11 +196,11 @@ impl ExecutionGadget for SstoreGadget { self.original_value .assign_u256(region, offset, original_value)?; - let (_, is_warm) = block.get_rws(step, 8).tx_access_list_value_pair(); + let (_, is_warm) = block.get_rws(step, 9).tx_access_list_value_pair(); self.is_warm .assign(region, offset, Value::known(F::from(is_warm as u64)))?; - let (tx_refund, tx_refund_prev) = block.get_rws(step, 9).tx_refund_value_pair(); + let (tx_refund, tx_refund_prev) = block.get_rws(step, 10).tx_refund_value_pair(); self.tx_refund_prev .assign(region, offset, Some(tx_refund_prev.to_le_bytes()))?;