feat(forge): revert diagnostic inspector #10446
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| 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::{Cfg, ContextTr, JournalTr}, | ||
| inspector::JournalExt, | ||
| interpreter::{ | ||
| interpreter::EthInterpreter, interpreter_types::Jumps, CallInputs, CallOutcome, CallScheme, | ||
| InstructionResult, Interpreter, InterpreterAction, InterpreterResult, | ||
| }, | ||
| precompile::{PrecompileSpecId, Precompiles}, | ||
| primitives::hardfork::SpecId, | ||
| 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 { | ||
| /// 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`. | ||
| 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<DetailedRevertReason> { | ||
| 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<CTX, D> Inspector<CTX, EthInterpreter> for RevertDiagnostic | ||
| where | ||
| D: Database<Error = DatabaseError>, | ||
| CTX: ContextTr<Db = D>, | ||
| 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<CallOutcome> { | ||
| let target = self.code_target_address(inputs); | ||
|
|
||
| if IGNORE.contains(&target) || self.is_precompile(ctx.cfg().spec().into(), target) { | ||
0xrusowsky marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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().depth())); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. note: pattern There was a problem hiding this comment. i'll open a new issue for that one and the unsupported selector. just to be certain that i don't miss anything:
There was a problem hiding this comment.
I think it's likely that Solidity generates some common chunk of code that's always reached in such cases. you could try writing a contract that has many such calls that are failing to decode the calldata in different methods. I'd expect all of those calls to end up in a similar
not sure if the JUMPI approach would work because iirc there are some checks around call value also, we could similarly try to see how the fallbackless contract revert looks like, I'd expect Solidity to generate similar bytecode for such |
||
| 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) { | ||
|
There was a problem hiding this comment. Please remember that Please look at cheatcodes inspector on how to outline the if conditions inside of step and step_end; in this case you should do something like this: |
||
| // REVERT (offset, size) | ||
| if REVERT == interp.bytecode.opcode() { | ||
| if let Ok(size) = interp.stack.peek(1) { | ||
| if size == U256::ZERO { | ||
0xrusowsky marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Check empty revert with same depth as a non-contract call | ||
| if let Some((_, _, depth)) = self.non_contract_call { | ||
| if ctx.journal().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().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) || self.is_precompile(ctx.cfg().spec().into(), addr) { | ||
| return; | ||
|
There was a problem hiding this comment. these |
||
| } | ||
|
|
||
| // Optimistically cache --> validated and cleared (if necessary) at `fn step_end()` | ||
| self.non_contract_size_check = Some((addr, ctx.journal().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; | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.