diff --git a/crates/evm/evm/src/inspectors/mod.rs b/crates/evm/evm/src/inspectors/mod.rs index fcf6836e2b9d8..85ed1354ea11a 100644 --- a/crates/evm/evm/src/inspectors/mod.rs +++ b/crates/evm/evm/src/inspectors/mod.rs @@ -21,3 +21,6 @@ pub use script::ScriptExecutionInspector; mod stack; pub use stack::{InspectorData, InspectorStack, InspectorStackBuilder}; + +mod revert_diagnostic; +pub use revert_diagnostic::RevertDiagnostic; diff --git a/crates/evm/evm/src/inspectors/revert_diagnostic.rs b/crates/evm/evm/src/inspectors/revert_diagnostic.rs new file mode 100644 index 0000000000000..30f44aec1bb18 --- /dev/null +++ b/crates/evm/evm/src/inspectors/revert_diagnostic.rs @@ -0,0 +1,207 @@ +use alloy_primitives::{Address, U256}; +use alloy_sol_types::SolValue; +use foundry_evm_core::{ + backend::DatabaseError, + constants::{CHEATCODE_ADDRESS, HARDHAT_CONSOLE_ADDRESS}, +}; +use revm::{ + bytecode::opcode::{EXTCODESIZE, REVERT}, + context::{ContextTr, JournalTr}, + inspector::JournalExt, + interpreter::{ + interpreter::EthInterpreter, interpreter_types::Jumps, CallInputs, CallOutcome, CallScheme, + InstructionResult, Interpreter, InterpreterAction, InterpreterResult, + }, + Database, Inspector, +}; +use std::fmt; + +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) +} + +#[derive(Debug, Clone, Copy)] +pub enum DetailedRevertReason { + CallToNonContract(Address), + DelegateCallToNonContract(Address), +} + +impl fmt::Display for DetailedRevertReason { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::CallToNonContract(addr) => { + write!(f, "call to non-contract address {addr}") + } + Self::DelegateCallToNonContract(addr) => write!( + f, + "delegatecall to non-contract address {addr} (usually an unliked library)" + ), + } + } +} + +/// 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 with non-empty calldata is made to an address without bytecode, +/// 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. + pub non_contract_call: Option<(Address, CallScheme, usize)>, + /// Tracks EXTCODESIZE checks that target an address without executable code. + pub non_contract_size_check: Option<(Address, usize)>, + /// Whether the step opcode is EXTCODESIZE or not. + pub is_extcodesize_step: bool, +} + +impl RevertDiagnostic { + /// Returns the effective target address whose code would be executed. + /// For delegate calls, this is the `bytecode_address`. Otherwise, it's the `target_address`. + fn code_target_address(&self, inputs: &mut CallInputs) -> Address { + if is_delegatecall(inputs.scheme) { + inputs.bytecode_address + } else { + inputs.target_address + } + } + + /// Derives the revert reason based on the cached data. Should only be called after a revert. + fn reason(&self) -> Option { + if let Some((addr, scheme, _)) = self.non_contract_call { + 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 { + // unknown schema as the call never took place --> output most generic reason + return Some(DetailedRevertReason::CallToNonContract(addr)); + } + + None + } + + /// Injects the revert diagnostic into the debug traces. Should only be called after a revert. + fn handle_revert_diagnostic(&self, interp: &mut Interpreter) { + if let Some(reason) = self.reason() { + interp.control.instruction_result = InstructionResult::Revert; + interp.control.next_action = InterpreterAction::Return { + result: InterpreterResult { + output: reason.to_string().abi_encode().into(), + gas: interp.control.gas, + result: InstructionResult::Revert, + }, + }; + } + } +} + +impl Inspector for RevertDiagnostic +where + D: Database, + CTX: ContextTr, + CTX::Journal: JournalExt, +{ + /// 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 CTX, inputs: &mut CallInputs) -> Option { + let target = self.code_target_address(inputs); + + if IGNORE.contains(&target) || ctx.journal_ref().precompile_addresses().contains(&target) { + return None; + } + + if let Ok(state) = ctx.journal().code(target) { + if state.is_empty() && !inputs.input.is_empty() { + self.non_contract_call = Some((target, inputs.scheme, ctx.journal_ref().depth())); + } + } + None + } + + /// Handles `REVERT` and `EXTCODESIZE` opcodes for diagnostics. + /// + /// When a `REVERT` opcode with zero data size occurs: + /// - if `non_contract_call` was set at the current depth, `handle_revert_diagnostic` is + /// called. Otherwise, it is cleared. + /// - if `non_contract_call` was set at the current depth, `handle_revert_diagnostic` is + /// called. Otherwise, it is cleared. + /// + /// When an `EXTCODESIZE` opcode occurs: + /// - Optimistically caches the target address and current depth in `non_contract_size_check`, + /// pending later validation. + fn step(&mut self, interp: &mut Interpreter, ctx: &mut CTX) { + // REVERT (offset, size) + if REVERT == interp.bytecode.opcode() { + if let Ok(size) = interp.stack.peek(1) { + if size.is_zero() { + // Check empty revert with same depth as a non-contract call + if let Some((_, _, depth)) = self.non_contract_call { + if ctx.journal_ref().depth() == depth { + self.handle_revert_diagnostic(interp); + } else { + self.non_contract_call = None; + } + return; + } + + // Check empty revert with same depth as a non-contract size check + if let Some((_, depth)) = self.non_contract_size_check { + if depth == ctx.journal_ref().depth() { + self.handle_revert_diagnostic(interp); + } else { + self.non_contract_size_check = None; + } + } + } + } + } + // EXTCODESIZE (address) + else if EXTCODESIZE == interp.bytecode.opcode() { + if let Ok(word) = interp.stack.peek(0) { + let addr = Address::from_word(word.into()); + if IGNORE.contains(&addr) || + ctx.journal_ref().precompile_addresses().contains(&addr) + { + return; + } + + // Optimistically cache --> validated and cleared (if necessary) at `fn step_end()` + self.non_contract_size_check = Some((addr, ctx.journal_ref().depth())); + self.is_extcodesize_step = true; + } + } + } + + /// Tracks `EXTCODESIZE` output. If the bytecode size is 0, clears the cache. + fn step_end(&mut self, interp: &mut Interpreter, _ctx: &mut CTX) { + if self.is_extcodesize_step { + if let Ok(size) = interp.stack.peek(0) { + if size != U256::ZERO { + self.non_contract_size_check = None; + } + } + + self.is_extcodesize_step = false; + } + } +} diff --git a/crates/evm/evm/src/inspectors/stack.rs b/crates/evm/evm/src/inspectors/stack.rs index de08972d2efa5..08c608d9a9006 100644 --- a/crates/evm/evm/src/inspectors/stack.rs +++ b/crates/evm/evm/src/inspectors/stack.rs @@ -1,6 +1,6 @@ use super::{ Cheatcodes, CheatsConfig, ChiselState, CoverageCollector, CustomPrintTracer, Fuzzer, - LogCollector, ScriptExecutionInspector, TracingInspector, + LogCollector, RevertDiagnostic, ScriptExecutionInspector, TracingInspector, }; use alloy_evm::{eth::EthEvmContext, Evm}; use alloy_primitives::{ @@ -50,7 +50,7 @@ pub struct InspectorStackBuilder { pub cheatcodes: Option>, /// The fuzzer inspector and its state, if it exists. pub fuzzer: Option, - /// Whether to enable tracing. + /// Whether to enable tracing and revert diagnostics. pub trace_mode: TraceMode, /// Whether logs should be collected. pub logs: Option, @@ -143,6 +143,7 @@ impl InspectorStackBuilder { } /// Set whether to enable the tracer. + /// Revert diagnostic inspector is activated when `mode != TraceMode::None` #[inline] pub fn trace_mode(mut self, mode: TraceMode) -> Self { if self.trace_mode < mode { @@ -304,6 +305,7 @@ pub struct InspectorStackInner { pub enable_isolation: bool, pub odyssey: bool, pub create2_deployer: Address, + pub revert_diag: Option, /// Flag marking if we are in the inner EVM context. pub in_inner_context: bool, @@ -439,8 +441,15 @@ impl InspectorStack { } /// Set whether to enable the tracer. + /// Revert diagnostic inspector is activated when `mode != TraceMode::None` #[inline] pub fn tracing(&mut self, mode: TraceMode) { + if mode.is_none() { + self.revert_diag = None; + } else { + self.revert_diag = Some(RevertDiagnostic::default()); + } + if let Some(config) = mode.into_config() { *self.tracer.get_or_insert_with(Default::default).config_mut() = config; } else { @@ -520,7 +529,13 @@ impl InspectorStackRefMut<'_> { let result = outcome.result.result; call_inspectors!( #[ret] - [&mut self.fuzzer, &mut self.tracer, &mut self.cheatcodes, &mut self.printer], + [ + &mut self.fuzzer, + &mut self.tracer, + &mut self.cheatcodes, + &mut self.printer, + &mut self.revert_diag + ], |inspector| { let previous_outcome = outcome.clone(); inspector.call_end(ecx, inputs, outcome); @@ -801,7 +816,8 @@ impl Inspector> for InspectorStackRefMut<'_> &mut self.coverage, &mut self.cheatcodes, &mut self.script_execution_inspector, - &mut self.printer + &mut self.printer, + &mut self.revert_diag ], |inspector| inspector.step(interpreter, ecx), ); @@ -813,7 +829,13 @@ impl Inspector> for InspectorStackRefMut<'_> ecx: &mut EthEvmContext<&mut dyn DatabaseExt>, ) { call_inspectors!( - [&mut self.tracer, &mut self.cheatcodes, &mut self.chisel_state, &mut self.printer], + [ + &mut self.tracer, + &mut self.cheatcodes, + &mut self.chisel_state, + &mut self.printer, + &mut self.revert_diag + ], |inspector| inspector.step_end(interpreter, ecx), ); } @@ -846,7 +868,13 @@ impl Inspector> for InspectorStackRefMut<'_> call_inspectors!( #[ret] - [&mut self.fuzzer, &mut self.tracer, &mut self.log_collector, &mut self.printer], + [ + &mut self.fuzzer, + &mut self.tracer, + &mut self.log_collector, + &mut self.printer, + &mut self.revert_diag + ], |inspector| { let mut out = None; if let Some(output) = inspector.call(ecx, call) { diff --git a/crates/evm/traces/src/decoder/mod.rs b/crates/evm/traces/src/decoder/mod.rs index 483ebc0b90785..44b9c9cd729b6 100644 --- a/crates/evm/traces/src/decoder/mod.rs +++ b/crates/evm/traces/src/decoder/mod.rs @@ -123,6 +123,9 @@ pub struct CallTraceDecoder { /// Contract addresses that have fallback functions, mapped to function selectors of that /// contract. pub fallback_contracts: HashMap>, + /// Contract addresses that have do NOT have fallback functions, mapped to function selectors + /// of that contract. + pub non_fallback_contracts: HashMap>, /// All known functions. pub functions: HashMap>, @@ -176,6 +179,7 @@ impl CallTraceDecoder { ]), receive_contracts: Default::default(), fallback_contracts: Default::default(), + non_fallback_contracts: Default::default(), functions: console::hh::abi::functions() .into_values() @@ -307,6 +311,9 @@ impl CallTraceDecoder { if abi.fallback.is_some() { self.fallback_contracts .insert(address, abi.functions().map(|f| f.selector()).collect()); + } else { + self.non_fallback_contracts + .insert(address, abi.functions().map(|f| f.selector()).collect()); } } } @@ -364,6 +371,45 @@ impl CallTraceDecoder { &functions } }; + + // Check if unsupported fn selector: calldata dooes NOT point to one of its selectors + + // non-fallback contract + no receive + if let Some(contract_selectors) = self.non_fallback_contracts.get(&trace.address) { + if !contract_selectors.contains(&selector) && + (!cdata.is_empty() || !self.receive_contracts.contains(&trace.address)) + { + let return_data = if !trace.success { + let revert_msg = + self.revert_decoder.decode(&trace.output, Some(trace.status)); + + if trace.output.is_empty() || revert_msg.contains("EvmError: Revert") { + Some(format!( + "unrecognized function selector {} for contract {}, which has no fallback function.", + selector, trace.address + )) + } else { + Some(revert_msg) + } + } else { + None + }; + + if let Some(func) = functions.first() { + return DecodedCallTrace { + label, + call_data: Some(self.decode_function_input(trace, func)), + return_data, + }; + } else { + return DecodedCallTrace { + label, + call_data: self.fallback_call_data(trace), + return_data, + }; + }; + } + } + let [func, ..] = &functions[..] else { return DecodedCallTrace { label, @@ -482,7 +528,7 @@ impl CallTraceDecoder { "parseJsonBytes32Array" | "writeJson" | // `keyExists` is being deprecated in favor of `keyExistsJson`. It will be removed in future versions. - "keyExists" | + "keyExists" | "keyExistsJson" | "serializeBool" | "serializeUint" | @@ -498,7 +544,7 @@ impl CallTraceDecoder { let mut decoded = func.abi_decode_input(&data[SELECTOR_LEN..]).ok()?; let token = if func.name.as_str() == "parseJson" || // `keyExists` is being deprecated in favor of `keyExistsJson`. It will be removed in future versions. - func.name.as_str() == "keyExists" || + func.name.as_str() == "keyExists" || func.name.as_str() == "keyExistsJson" { "" diff --git a/crates/forge/tests/cli/test_cmd.rs b/crates/forge/tests/cli/test_cmd.rs index cd3b734194c88..af4b2aed7d0e7 100644 --- a/crates/forge/tests/cli/test_cmd.rs +++ b/crates/forge/tests/cli/test_cmd.rs @@ -3650,3 +3650,181 @@ Encountered a total of 1 failing tests, 0 tests succeeded "#]]); }); + +#[cfg(not(feature = "isolate-by-default"))] +forgetest_init!(detailed_revert_when_calling_non_contract_address, |prj, cmd| { + prj.add_test( + "NonContractCallRevertTest.t.sol", + r#" +import "forge-std/Test.sol"; +import {Counter} from "../src/Counter.sol"; + +interface ICounter { + function increment() external; + function number() external returns (uint256); + function random() external returns (uint256); +} + +contract NonContractCallRevertTest is Test { + Counter public counter; + address constant ADDRESS = 0xdEADBEeF00000000000000000000000000000000; + + function setUp() public { + counter = new Counter(); + counter.setNumber(1); + } + + function test_non_supported_selector_call_failure() public { + console.log("test non supported fn selector call failure"); + ICounter(address(counter)).random(); + } + + function test_non_contract_call_failure() public { + console.log("test non contract call failure"); + ICounter(ADDRESS).number(); + } + + function test_non_contract_void_call_failure() public { + console.log("test non contract (void) call failure"); + ICounter(ADDRESS).increment(); + } +} + "#, + ) + .unwrap(); + + cmd.args(["test", "--mc", "NonContractCallRevertTest", "-vvv"]) + .assert_failure() + .stdout_eq(str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 3 tests for test/NonContractCallRevertTest.t.sol:NonContractCallRevertTest +[FAIL: call to non-contract address 0xdEADBEeF00000000000000000000000000000000] test_non_contract_call_failure() ([GAS]) +Logs: + test non contract call failure + +Traces: + [6350] NonContractCallRevertTest::test_non_contract_call_failure() + ├─ [0] console::log("test non contract call failure") [staticcall] + │ └─ ← [Stop] + ├─ [0] 0xdEADBEeF00000000000000000000000000000000::number() + │ └─ ← [Stop] + └─ ← [Revert] call to non-contract address 0xdEADBEeF00000000000000000000000000000000 + +[FAIL: call to non-contract address 0xdEADBEeF00000000000000000000000000000000] test_non_contract_void_call_failure() ([GAS]) +Logs: + test non contract (void) call failure + +Traces: + [6215] NonContractCallRevertTest::test_non_contract_void_call_failure() + ├─ [0] console::log("test non contract (void) call failure") [staticcall] + │ └─ ← [Stop] + └─ ← [Revert] call to non-contract address 0xdEADBEeF00000000000000000000000000000000 + +[FAIL: EvmError: Revert] test_non_supported_selector_call_failure() ([GAS]) +Logs: + test non supported fn selector call failure + +Traces: + [8620] NonContractCallRevertTest::test_non_supported_selector_call_failure() + ├─ [0] console::log("test non supported fn selector call failure") [staticcall] + │ └─ ← [Stop] + ├─ [145] Counter::random() + │ └─ ← [Revert] unrecognized function selector 0x5ec01e4d for contract 0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f, which has no fallback function. + └─ ← [Revert] EvmError: Revert + +Suite result: FAILED. 0 passed; 3 failed; 0 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 3 failed, 0 skipped (3 total tests) + +Failing tests: +Encountered 3 failing tests in test/NonContractCallRevertTest.t.sol:NonContractCallRevertTest +[FAIL: call to non-contract address 0xdEADBEeF00000000000000000000000000000000] test_non_contract_call_failure() ([GAS]) +[FAIL: call to non-contract address 0xdEADBEeF00000000000000000000000000000000] test_non_contract_void_call_failure() ([GAS]) +[FAIL: EvmError: Revert] test_non_supported_selector_call_failure() ([GAS]) + +Encountered a total of 3 failing tests, 0 tests succeeded + +"#]]); +}); + +#[cfg(not(feature = "isolate-by-default"))] +forgetest_init!(detailed_revert_when_delegatecalling_unlinked_library, |prj, cmd| { + prj.add_test( + "NonContractDelegateCallRevertTest.t.sol", + r#" +import "forge-std/Test.sol"; + +library TestLibrary { + function foo(uint256 a) public pure returns (uint256) { + return a * 2; + } +} + +contract LibraryCaller { + address public lib; + + constructor(address _lib) { + lib = _lib; + } + + function foobar(uint256 val) public returns (uint256) { + (bool success, bytes memory data) = lib.delegatecall( + abi.encodeWithSelector(TestLibrary.foo.selector, val) + ); + + assert(success); + return abi.decode(data, (uint256)); + } +} + +contract NonContractDelegateCallRevertTest is Test { + function test_unlinked_library_call_failure() public { + console.log("Test: Simulating call to unlinked library"); + LibraryCaller caller = new LibraryCaller(0xdEADBEeF00000000000000000000000000000000); + + caller.foobar(10); + } +} + "#, + ) + .unwrap(); + + cmd.args(["test", "--mc", "NonContractDelegateCallRevertTest", "-vvv"]) + .assert_failure() + .stdout_eq(str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 1 test for test/NonContractDelegateCallRevertTest.t.sol:NonContractDelegateCallRevertTest +[FAIL: delegatecall to non-contract address 0xdEADBEeF00000000000000000000000000000000 (usually an unliked library)] test_unlinked_library_call_failure() ([GAS]) +Logs: + Test: Simulating call to unlinked library + +Traces: + [255303] NonContractDelegateCallRevertTest::test_unlinked_library_call_failure() + ├─ [0] console::log("Test: Simulating call to unlinked library") [staticcall] + │ └─ ← [Stop] + ├─ [214746] → new LibraryCaller@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f + │ └─ ← [Return] 960 bytes of code + ├─ [3896] LibraryCaller::foobar(10) + │ ├─ [0] 0xdEADBEeF00000000000000000000000000000000::foo(10) [delegatecall] + │ │ └─ ← [Stop] + │ └─ ← [Revert] delegatecall to non-contract address 0xdEADBEeF00000000000000000000000000000000 (usually an unliked library) + └─ ← [Revert] delegatecall to non-contract address 0xdEADBEeF00000000000000000000000000000000 (usually an unliked library) + +Suite result: FAILED. 0 passed; 1 failed; 0 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 1 failed, 0 skipped (1 total tests) + +Failing tests: +Encountered 1 failing test in test/NonContractDelegateCallRevertTest.t.sol:NonContractDelegateCallRevertTest +[FAIL: delegatecall to non-contract address 0xdEADBEeF00000000000000000000000000000000 (usually an unliked library)] test_unlinked_library_call_failure() ([GAS]) + +Encountered a total of 1 failing tests, 0 tests succeeded + +"#]]); +});