feat: panic on invalid account lookups (privacy-scaling-explorations#…

…1092)

* feat: panic on invalid account lookups

- Account field lookups for fields different than CodeHash are invalid if the
  account doesn't exist.
- All account field lookups should contain a value_prev that matches with the
  field value found in the StateDB for that account.

If any of these two points don't hold, the corresponding chain of MPT proofs
can't be generated.

Test with:
`cargo test --profile release evm_circuit --all-features -- --nocapture 2>&1 | tee /tmp/zkevm-tests.log` from `zkevm-circuits`.

* Fix error_oog_call

* Clean up

* Fix: is_create tx was reading wrong code_hash

* Make clippy happy

* Fix error_oog_call after rebase

* apply suggestions from @lispc and @ChihChengLiang

bus-mapping/src/circuit_input_builder/input_state_ref.rs

@@ -100,6 +100,9 @@ impl<'a> CircuitInputStateRef<'a> {
     /// bus-mapping instance of the current [`ExecStep`].  Then increase the
     /// block_ctx [`RWCounter`](crate::operation::RWCounter) by one.
     pub fn push_op<T: Op>(&mut self, step: &mut ExecStep, rw: RW, op: T) {
+        if let OpEnum::Account(op) = op.clone().into_enum() {
+            self.check_update_sdb_account(rw, &op)
+        }
         let op_ref =
             self.block
                 .container
@@ -262,6 +265,63 @@ impl<'a> CircuitInputStateRef<'a> {
         Ok(())
     }
 
+    /// First check the validity and consistency of the rw operation against the
+    /// account in the StateDB, then if the rw operation is a write, apply
+    /// it to the corresponding account in the StateDB.
+    fn check_update_sdb_account(&mut self, rw: RW, op: &AccountOp) {
+        let account = self.sdb.get_account_mut(&op.address).1;
+        // -- sanity check begin --
+        // Verify that a READ doesn't change the field value
+        if matches!(rw, RW::READ) && op.value_prev != op.value {
+            panic!(
+                "RWTable Account field read where value_prev != value rwc: {}, op: {:?}",
+                self.block_ctx.rwc.0, op
+            )
+        }
+        let account_value_prev = match op.field {
+            AccountField::Nonce => account.nonce,
+            AccountField::Balance => account.balance,
+            AccountField::CodeHash => {
+                if account.is_empty() {
+                    Word::zero()
+                } else {
+                    account.code_hash.to_word()
+                }
+            }
+        };
+        // Verify that the previous value matches the account field value in the StateDB
+        if op.value_prev != account_value_prev {
+            panic!("RWTable Account field {:?} lookup doesn't match account value account: {:?}, rwc: {}, op: {:?}",
+                rw,
+                account,
+                self.block_ctx.rwc.0,
+                op
+            );
+        }
+        // Verify that no read is done to a field other than CodeHash to a non-existing
+        // account (only CodeHash reads with value=0 can be done to non-existing
+        // accounts, which the State Circuit translates to MPT
+        // AccountNonExisting proofs lookups).
+        if (!matches!(op.field, AccountField::CodeHash)
+            && (matches!(rw, RW::READ) || (op.value_prev.is_zero() && op.value.is_zero())))
+            && account.is_empty()
+        {
+            panic!(
+                "RWTable Account field {:?} lookup to non-existing account rwc: {}, op: {:?}",
+                rw, self.block_ctx.rwc.0, op
+            );
+        }
+        // -- sanity check end --
+        // Perform the write to the account in the StateDB
+        if matches!(rw, RW::WRITE) {
+            match op.field {
+                AccountField::Nonce => account.nonce = op.value,
+                AccountField::Balance => account.balance = op.value,
+                AccountField::CodeHash => account.code_hash = H256::from(op.value.to_be_bytes()),
+            }
+        }
+    }
+
     /// Push a read type [`AccountOp`] into the
     /// [`OperationContainer`](crate::operation::OperationContainer) with the
     /// next [`RWCounter`](crate::operation::RWCounter), and then
@@ -276,11 +336,8 @@ impl<'a> CircuitInputStateRef<'a> {
         value: Word,
         value_prev: Word,
     ) -> Result<(), Error> {
-        self.push_op(
-            step,
-            RW::READ,
-            AccountOp::new(address, field, value, value_prev),
-        );
+        let op = AccountOp::new(address, field, value, value_prev);
+        self.push_op(step, RW::READ, op);
         Ok(())
     }
 
@@ -298,11 +355,8 @@ impl<'a> CircuitInputStateRef<'a> {
         value: Word,
         value_prev: Word,
     ) -> Result<(), Error> {
-        self.push_op(
-            step,
-            RW::WRITE,
-            AccountOp::new(address, field, value, value_prev),
-        );
+        let op = AccountOp::new(address, field, value, value_prev);
+        self.push_op(step, RW::WRITE, op);
         Ok(())
     }
 
@@ -759,7 +813,6 @@ impl<'a> CircuitInputStateRef<'a> {
                     AccountField::Nonce => account.nonce = op.value,
                     AccountField::Balance => account.balance = op.value,
                     AccountField::CodeHash => account.code_hash = op.value.to_be_bytes().into(),
-                    AccountField::NonExisting => (),
                 }
             }
             OpEnum::TxRefund(op) => {

bus-mapping/src/evm/opcodes.rs

@@ -347,13 +347,13 @@ pub fn gen_begin_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
 
     // Increase caller's nonce
     let caller_address = call.caller_address;
-    let nonce_prev = state.sdb.increase_nonce(&caller_address);
+    let nonce_prev = state.sdb.get_account(&caller_address).1.nonce;
     state.account_write(
         &mut exec_step,
         caller_address,
         AccountField::Nonce,
-        (nonce_prev + 1).into(),
-        nonce_prev.into(),
+        nonce_prev + 1,
+        nonce_prev,
     )?;
 
     // Add caller and callee into access list
@@ -391,24 +391,25 @@ pub fn gen_begin_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
     )?;
 
     // Get code_hash of callee
-    let (_, callee_account) = state.sdb.get_account(&call.address);
-    let code_hash = callee_account.code_hash;
+    let callee_code_hash = call.code_hash;
+    let callee_exists = !state.sdb.get_account(&call.address).1.is_empty();
+    let (callee_code_hash_word, is_empty_code_hash) = if callee_exists {
+        (
+            callee_code_hash.to_word(),
+            callee_code_hash.to_fixed_bytes() == *EMPTY_HASH,
+        )
+    } else {
+        (Word::zero(), true)
+    };
 
     // There are 4 branches from here.
     match (
         call.is_create(),
         state.is_precompiled(&call.address),
-        code_hash.to_fixed_bytes() == *EMPTY_HASH,
+        is_empty_code_hash,
     ) {
         // 1. Creation transaction.
         (true, _, _) => {
-            state.account_read(
-                &mut exec_step,
-                call.address,
-                AccountField::CodeHash,
-                call.code_hash.to_word(),
-                call.code_hash.to_word(),
-            )?;
             for (field, value) in [
                 (CallContextField::Depth, call.depth.into()),
                 (
@@ -447,8 +448,8 @@ pub fn gen_begin_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
                 &mut exec_step,
                 call.address,
                 AccountField::CodeHash,
-                code_hash.to_word(),
-                code_hash.to_word(),
+                callee_code_hash_word,
+                callee_code_hash_word,
             )?;
 
             // 3. Call to account with empty code.
@@ -479,7 +480,7 @@ pub fn gen_begin_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
                 (CallContextField::LastCalleeReturnDataLength, 0.into()),
                 (CallContextField::IsRoot, 1.into()),
                 (CallContextField::IsCreate, 0.into()),
-                (CallContextField::CodeHash, code_hash.to_word()),
+                (CallContextField::CodeHash, callee_code_hash_word),
             ] {
                 state.call_context_write(&mut exec_step, call.call_id, field, value);
             }
@@ -519,15 +520,13 @@ pub fn gen_end_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
 
     let effective_refund =
         refund.min((state.tx.gas - exec_step.gas_left.0) / MAX_REFUND_QUOTIENT_OF_GAS_USED as u64);
-    let (found, caller_account) = state.sdb.get_account_mut(&call.caller_address);
+    let (found, caller_account) = state.sdb.get_account(&call.caller_address);
     if !found {
         return Err(Error::AccountNotFound(call.caller_address));
     }
     let caller_balance_prev = caller_account.balance;
     let caller_balance =
         caller_balance_prev + state.tx.gas_price * (exec_step.gas_left.0 + effective_refund);
-    caller_account.balance = caller_balance;
-
     state.account_write(
         &mut exec_step,
         call.caller_address,
@@ -537,14 +536,13 @@ pub fn gen_end_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
     )?;
 
     let effective_tip = state.tx.gas_price - state.block.base_fee;
-    let (found, coinbase_account) = state.sdb.get_account_mut(&state.block.coinbase);
+    let (found, coinbase_account) = state.sdb.get_account(&state.block.coinbase);
     if !found {
         return Err(Error::AccountNotFound(state.block.coinbase));
     }
     let coinbase_balance_prev = coinbase_account.balance;
     let coinbase_balance =
         coinbase_balance_prev + effective_tip * (state.tx.gas - exec_step.gas_left.0);
-    coinbase_account.balance = coinbase_balance;
     state.account_write(
         &mut exec_step,
         state.block.coinbase,

bus-mapping/src/evm/opcodes/callop.rs

@@ -89,6 +89,25 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
             (call.is_success as u64).into(),
         )?;
 
+        let callee_code_hash = call.code_hash;
+        let callee_exists = !state.sdb.get_account(&callee_address).1.is_empty();
+
+        let (callee_code_hash_word, is_empty_code_hash) = if callee_exists {
+            (
+                callee_code_hash.to_word(),
+                callee_code_hash.to_fixed_bytes() == *EMPTY_HASH,
+            )
+        } else {
+            (Word::zero(), true)
+        };
+        state.account_read(
+            &mut exec_step,
+            callee_address,
+            AccountField::CodeHash,
+            callee_code_hash_word,
+            callee_code_hash_word,
+        )?;
+
         let is_warm = state.sdb.check_account_in_access_list(&callee_address);
         state.push_op_reversible(
             &mut exec_step,
@@ -140,11 +159,9 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
             caller_balance,
         )?;
 
-        let callee_code_hash = call.code_hash;
-        let callee_exists = !state.sdb.get_account(&callee_address).1.is_empty();
-
-        // Transfer value only for CALL opcode. only when insufficient_balance = false.
-        if call.kind == CallKind::Call && !insufficient_balance {
+        // Transfer value only for CALL opcode, insufficient_balance = false
+        // and value > 0.
+        if call.kind == CallKind::Call && !insufficient_balance && !call.value.is_zero() {
             state.transfer(
                 &mut exec_step,
                 call.caller_address,
@@ -153,22 +170,6 @@ impl<const N_ARGS: usize> Opcode for CallOpcode<N_ARGS> {
             )?;
         }
 
-        let (callee_code_hash_word, is_empty_code_hash) = if callee_exists {
-            (
-                callee_code_hash.to_word(),
-                callee_code_hash.to_fixed_bytes() == *EMPTY_HASH,
-            )
-        } else {
-            (Word::zero(), true)
-        };
-        state.account_read(
-            &mut exec_step,
-            callee_address,
-            AccountField::CodeHash,
-            callee_code_hash_word,
-            callee_code_hash_word,
-        )?;
-
         // Calculate next_memory_word_size and callee_gas_left manually in case
         // there isn't next geth_step (e.g. callee doesn't have code).
         debug_assert_eq!(exec_step.memory_size % 32, 0);

bus-mapping/src/evm/opcodes/error_oog_call.rs

@@ -4,7 +4,7 @@ use crate::{
     operation::{AccountField, CallContextField, TxAccessListAccountOp, RW},
     Error,
 };
-use eth_types::{GethExecStep, ToAddress, ToWord};
+use eth_types::{GethExecStep, ToAddress, ToWord, Word};
 
 /// Placeholder structure used to implement [`Opcode`] trait over it
 /// corresponding to the `OpcodeId::CALL` `OpcodeId`.
@@ -60,6 +60,23 @@ impl Opcode for OOGCall {
             (0u64).into(), // must fail
         )?;
 
+        let (_, callee_account) = state.sdb.get_account(&call_address);
+        let callee_exists = !callee_account.is_empty();
+        let callee_code_hash = callee_account.code_hash;
+        let callee_code_hash_word = if callee_exists {
+            callee_code_hash.to_word()
+        } else {
+            Word::zero()
+        };
+
+        state.account_read(
+            &mut exec_step,
+            call_address,
+            AccountField::CodeHash,
+            callee_code_hash_word,
+            callee_code_hash_word,
+        )?;
+
         let is_warm = state.sdb.check_account_in_access_list(&call_address);
         state.push_op(
             &mut exec_step,
@@ -72,15 +89,6 @@ impl Opcode for OOGCall {
             },
         );
 
-        let (_, callee_account) = state.sdb.get_account(&call_address);
-        let callee_code_hash = callee_account.code_hash;
-        for (field, value) in [
-            (AccountField::Balance, callee_account.balance),
-            (AccountField::CodeHash, callee_code_hash.to_word()),
-        ] {
-            state.account_read(&mut exec_step, call_address, field, value, value)?;
-        }
-
         state.gen_restore_context_ops(&mut exec_step, geth_steps)?;
         state.handle_return(geth_step)?;
         Ok(vec![exec_step])

bus-mapping/src/operation.rs

@@ -531,16 +531,14 @@ impl Op for TxRefundOp {
 
 /// Represents a field parameter of the Account that can be accessed via EVM
 /// execution.
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
 pub enum AccountField {
     /// Account Nonce
     Nonce,
     /// Account Balance
     Balance,
     /// Account Code Hash
     CodeHash,
-    /// Account non existing
-    NonExisting,
 }
 
 /// Represents a change in the Account field implied by a `BeginTx`,

bus-mapping/src/state_db.rs

@@ -178,7 +178,7 @@ impl StateDB {
     }
 
     /// Get nonce of account with `addr`.
-    pub fn get_nonce(&mut self, addr: &Address) -> u64 {
+    pub fn get_nonce(&self, addr: &Address) -> u64 {
         let (_, account) = self.get_account(addr);
         account.nonce.as_u64()
     }