improve docs + make diagnostics more restrictive

Author: 0xrusowsky

crates/evm/evm/src/inspectors/revert_diagnostic.rs

@@ -1,12 +1,13 @@
-use alloy_primitives::{Address, U256};
+use alloy_primitives::{Address, Bytes, U256};
 use foundry_evm_core::{
     backend::DatabaseExt,
     constants::{CHEATCODE_ADDRESS, HARDHAT_CONSOLE_ADDRESS},
     decode::DetailedRevertReason,
 };
 use revm::{
     interpreter::{
-        opcode::EXTCODESIZE, CallInputs, CallOutcome, CallScheme, InstructionResult, Interpreter,
+        opcode::{EXTCODESIZE, REVERT},
+        CallInputs, CallOutcome, CallScheme, InstructionResult, Interpreter,
     },
     precompile::{PrecompileSpecId, Precompiles},
     primitives::SpecId,
@@ -15,8 +16,27 @@ use revm::{
 
 const IGNORE: [Address; 2] = [HARDHAT_CONSOLE_ADDRESS, CHEATCODE_ADDRESS];
 
+/// Checks if the call scheme corresponds to any sort of delegate call
+pub fn is_delegatecall(scheme: CallScheme) -> bool {
+    matches!(scheme, CallScheme::DelegateCall | CallScheme::ExtDelegateCall | CallScheme::CallCode)
+}
+
 /// An inspector that tracks call context to enhances revert diagnostics.
 /// Useful for understanding reverts that are not linked to custom errors or revert strings.
+///
+/// Supported diagnostics:
+///  1. **Non-void call to non-contract address:** the soldity compiler adds some validation to the
+///     return data of the call, so despite the call succeeds, as doesn't return data, the
+///     validation causes a revert.
+///
+///     Identified when: a call to an address with no code and non-empty calldata is made, followed
+///     by an empty revert at the same depth
+///
+///  2. **Void call to non-contract address:** in this case the solidity compiler adds some checks
+///     before doing the call, so it never takes place.
+///
+///     Identified when: extcodesize for the target address returns 0 + empty revert at the same
+///     depth
 #[derive(Clone, Debug, Default)]
 pub struct RevertDiagnostic {
     /// Tracks calls with calldata that target an address without executable code.
@@ -30,51 +50,48 @@ pub struct RevertDiagnostic {
 }
 
 impl RevertDiagnostic {
-    fn is_delegatecall(&self, scheme: CallScheme) -> bool {
-        matches!(
-            scheme,
-            CallScheme::DelegateCall | CallScheme::ExtDelegateCall | CallScheme::CallCode
-        )
-    }
-
+    /// Checks if the `target` address is a precompile for the given `spec_id`.
     fn is_precompile(&self, spec_id: SpecId, target: Address) -> bool {
         let precompiles = Precompiles::new(PrecompileSpecId::from_spec_id(spec_id));
         precompiles.contains(&target)
     }
 
+    /// Returns the effective target address whose code would be executed.
+    /// For delegate calls, this is the `bytecode_address`. Otherwise, it's the `target_address`.
     pub fn no_code_target_address(&self, inputs: &mut CallInputs) -> Address {
-        if self.is_delegatecall(inputs.scheme) {
+        if is_delegatecall(inputs.scheme) {
             inputs.bytecode_address
         } else {
             inputs.target_address
         }
     }
 
+    /// Derives the revert reason based on the cached information.
     pub fn reason(&self) -> Option<DetailedRevertReason> {
         if self.reverted {
             if let Some((addr, scheme, _)) = self.non_contract_call {
-                let reason = if self.is_delegatecall(scheme) {
+                let reason = if is_delegatecall(scheme) {
                     DetailedRevertReason::DelegateCallToNonContract(addr)
                 } else {
                     DetailedRevertReason::CallToNonContract(addr)
                 };
 
                 return Some(reason);
             }
-        }
 
-        if let Some((addr, _)) = self.non_contract_size_check {
-            // call never took place, so schema is unknown --> output most generic msg
-            return Some(DetailedRevertReason::CallToNonContract(addr));
+            if let Some((addr, _)) = self.non_contract_size_check {
+                // unknown schema as the call never took place --> output most generic reason
+                return Some(DetailedRevertReason::CallToNonContract(addr));
+            }
         }
 
         None
     }
 }
 
 impl<DB: Database + DatabaseExt> Inspector<DB> for RevertDiagnostic {
-    /// Tracks the first call, with non-zero calldata, that targeted a non-contract address.
-    /// Excludes precompiles and test addresses.
+    /// Tracks the first call with non-zero calldata that targets a non-contract address. Excludes
+    /// precompiles and test addresses.
     fn call(&mut self, ctx: &mut EvmContext<DB>, inputs: &mut CallInputs) -> Option<CallOutcome> {
         let target = self.no_code_target_address(inputs);
 
@@ -90,13 +107,47 @@ impl<DB: Database + DatabaseExt> Inspector<DB> for RevertDiagnostic {
         None
     }
 
-    /// Tracks `EXTCODESIZE` opcodes. Clears the cache when the call stack depth changes.
+    /// If a `non_contract_call` was previously recorded, will check if the call reverted without
+    /// data at the same depth. If so, flags `reverted` as `true`.
+    fn call_end(
+        &mut self,
+        ctx: &mut EvmContext<DB>,
+        _inputs: &CallInputs,
+        outcome: CallOutcome,
+    ) -> CallOutcome {
+        if let Some((_, _, depth)) = self.non_contract_call {
+            if outcome.result.result == InstructionResult::Revert &&
+                outcome.result.output == Bytes::new() &&
+                ctx.journaled_state.depth() == depth - 1
+            {
+                self.reverted = true
+            };
+        }
+
+        outcome
+    }
+
+    /// When the current opcode is `EXTCODESIZE`:
+    ///    - Tracks addresses being checked and the current depth (if not ignored or a precompile)
+    ///      on `non_contract_size_check`.
+    ///
+    /// When `non_contract_size_check` is `Some`:
+    ///    - If the call stack depth changes clears the cached data.
+    ///    - If the current opcode is `REVERT` and its size is zero, sets `reverted` to `true`.
     fn step(&mut self, interp: &mut Interpreter, ctx: &mut EvmContext<DB>) {
         if let Some((_, depth)) = self.non_contract_size_check {
             if depth != ctx.journaled_state.depth() {
                 self.non_contract_size_check = None;
             }
 
+            if REVERT == interp.current_opcode() {
+                if let Ok(size) = interp.stack().peek(1) {
+                    if size == U256::ZERO {
+                        self.reverted = true;
+                    }
+                }
+            }
+
             return;
         }
 
@@ -125,23 +176,4 @@ impl<DB: Database + DatabaseExt> Inspector<DB> for RevertDiagnostic {
             self.is_extcodesize_step = false;
         }
     }
-
-    /// Records whether the call reverted or not. Only if the revert call depth matches the
-    /// inspector cache.
-    fn call_end(
-        &mut self,
-        ctx: &mut EvmContext<DB>,
-        _inputs: &CallInputs,
-        outcome: CallOutcome,
-    ) -> CallOutcome {
-        if let Some((_, _, depth)) = self.non_contract_call {
-            if outcome.result.result == InstructionResult::Revert &&
-                ctx.journaled_state.depth() == depth - 1
-            {
-                self.reverted = true
-            };
-        }
-
-        outcome
-    }
 }

crates/forge/tests/cli/test_cmd.rs

@@ -3662,6 +3662,7 @@ interface ICounter {
 
 contract NonContractCallRevertTest is Test {
     Counter public counter;
+    address constant ADDRESS = 0xdEADBEeF00000000000000000000000000000000;
 
     function setUp() public {
         counter = new Counter();
@@ -3675,12 +3676,12 @@ contract NonContractCallRevertTest is Test {
 
     function test_non_contract_call_failure() public {
         console.log("test non contract call failure");
-        ICounter(address(0xdEADBEeF00000000000000000000000000000000)).number();
+        ICounter(ADDRESS).number();
     }
 
     function test_non_contract_void_call_failure() public {
         console.log("test non contract (void) call failure");
-        ICounter(address(0xdEADBEeF00000000000000000000000000000000)).increment();
+        ICounter(ADDRESS).increment();
     }
 }
      "#,