Skip to content

Commit 4332dc4

Browse files
authored
feat(forge): revert diagnostic inspector (#10446)
* chore: revert diagnostic inspector * style: clippy + fmt * style: more readable code + docs * fix: track call stack depth + EXTCODESIZE checks * chore(traces decoder): non-supported fn selector call * disable diagnostic revert when verbosity < '-vvv' * fix: do not warm address * fix: call inspector * fix: config revert diag with `fn tracing()` * improve docs + make diagnostics more restrictive * inject revert reason directly into interpreter * style: clippy + fmt * integrate new revm version * style: nits
1 parent 7b6a9f3 commit 4332dc4

File tree

5 files changed

+470
-8
lines changed

5 files changed

+470
-8
lines changed

crates/evm/evm/src/inspectors/mod.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,6 @@ pub use script::ScriptExecutionInspector;
2121

2222
mod stack;
2323
pub use stack::{InspectorData, InspectorStack, InspectorStackBuilder};
24+
25+
mod revert_diagnostic;
26+
pub use revert_diagnostic::RevertDiagnostic;
Lines changed: 207 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,207 @@
1+
use alloy_primitives::{Address, U256};
2+
use alloy_sol_types::SolValue;
3+
use foundry_evm_core::{
4+
backend::DatabaseError,
5+
constants::{CHEATCODE_ADDRESS, HARDHAT_CONSOLE_ADDRESS},
6+
};
7+
use revm::{
8+
bytecode::opcode::{EXTCODESIZE, REVERT},
9+
context::{ContextTr, JournalTr},
10+
inspector::JournalExt,
11+
interpreter::{
12+
interpreter::EthInterpreter, interpreter_types::Jumps, CallInputs, CallOutcome, CallScheme,
13+
InstructionResult, Interpreter, InterpreterAction, InterpreterResult,
14+
},
15+
Database, Inspector,
16+
};
17+
use std::fmt;
18+
19+
const IGNORE: [Address; 2] = [HARDHAT_CONSOLE_ADDRESS, CHEATCODE_ADDRESS];
20+
21+
/// Checks if the call scheme corresponds to any sort of delegate call
22+
pub fn is_delegatecall(scheme: CallScheme) -> bool {
23+
matches!(scheme, CallScheme::DelegateCall | CallScheme::ExtDelegateCall | CallScheme::CallCode)
24+
}
25+
26+
#[derive(Debug, Clone, Copy)]
27+
pub enum DetailedRevertReason {
28+
CallToNonContract(Address),
29+
DelegateCallToNonContract(Address),
30+
}
31+
32+
impl fmt::Display for DetailedRevertReason {
33+
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
34+
match self {
35+
Self::CallToNonContract(addr) => {
36+
write!(f, "call to non-contract address {addr}")
37+
}
38+
Self::DelegateCallToNonContract(addr) => write!(
39+
f,
40+
"delegatecall to non-contract address {addr} (usually an unliked library)"
41+
),
42+
}
43+
}
44+
}
45+
46+
/// An inspector that tracks call context to enhances revert diagnostics.
47+
/// Useful for understanding reverts that are not linked to custom errors or revert strings.
48+
///
49+
/// Supported diagnostics:
50+
/// 1. **Non-void call to non-contract address:** the soldity compiler adds some validation to the
51+
/// return data of the call, so despite the call succeeds, as doesn't return data, the
52+
/// validation causes a revert.
53+
///
54+
/// Identified when: a call with non-empty calldata is made to an address without bytecode,
55+
/// followed by an empty revert at the same depth.
56+
///
57+
/// 2. **Void call to non-contract address:** in this case the solidity compiler adds some checks
58+
/// before doing the call, so it never takes place.
59+
///
60+
/// Identified when: extcodesize for the target address returns 0 + empty revert at the same
61+
/// depth.
62+
#[derive(Clone, Debug, Default)]
63+
pub struct RevertDiagnostic {
64+
/// Tracks calls with calldata that target an address without executable code.
65+
pub non_contract_call: Option<(Address, CallScheme, usize)>,
66+
/// Tracks EXTCODESIZE checks that target an address without executable code.
67+
pub non_contract_size_check: Option<(Address, usize)>,
68+
/// Whether the step opcode is EXTCODESIZE or not.
69+
pub is_extcodesize_step: bool,
70+
}
71+
72+
impl RevertDiagnostic {
73+
/// Returns the effective target address whose code would be executed.
74+
/// For delegate calls, this is the `bytecode_address`. Otherwise, it's the `target_address`.
75+
fn code_target_address(&self, inputs: &mut CallInputs) -> Address {
76+
if is_delegatecall(inputs.scheme) {
77+
inputs.bytecode_address
78+
} else {
79+
inputs.target_address
80+
}
81+
}
82+
83+
/// Derives the revert reason based on the cached data. Should only be called after a revert.
84+
fn reason(&self) -> Option<DetailedRevertReason> {
85+
if let Some((addr, scheme, _)) = self.non_contract_call {
86+
let reason = if is_delegatecall(scheme) {
87+
DetailedRevertReason::DelegateCallToNonContract(addr)
88+
} else {
89+
DetailedRevertReason::CallToNonContract(addr)
90+
};
91+
92+
return Some(reason);
93+
}
94+
95+
if let Some((addr, _)) = self.non_contract_size_check {
96+
// unknown schema as the call never took place --> output most generic reason
97+
return Some(DetailedRevertReason::CallToNonContract(addr));
98+
}
99+
100+
None
101+
}
102+
103+
/// Injects the revert diagnostic into the debug traces. Should only be called after a revert.
104+
fn handle_revert_diagnostic(&self, interp: &mut Interpreter) {
105+
if let Some(reason) = self.reason() {
106+
interp.control.instruction_result = InstructionResult::Revert;
107+
interp.control.next_action = InterpreterAction::Return {
108+
result: InterpreterResult {
109+
output: reason.to_string().abi_encode().into(),
110+
gas: interp.control.gas,
111+
result: InstructionResult::Revert,
112+
},
113+
};
114+
}
115+
}
116+
}
117+
118+
impl<CTX, D> Inspector<CTX, EthInterpreter> for RevertDiagnostic
119+
where
120+
D: Database<Error = DatabaseError>,
121+
CTX: ContextTr<Db = D>,
122+
CTX::Journal: JournalExt,
123+
{
124+
/// Tracks the first call with non-zero calldata that targets a non-contract address. Excludes
125+
/// precompiles and test addresses.
126+
fn call(&mut self, ctx: &mut CTX, inputs: &mut CallInputs) -> Option<CallOutcome> {
127+
let target = self.code_target_address(inputs);
128+
129+
if IGNORE.contains(&target) || ctx.journal_ref().precompile_addresses().contains(&target) {
130+
return None;
131+
}
132+
133+
if let Ok(state) = ctx.journal().code(target) {
134+
if state.is_empty() && !inputs.input.is_empty() {
135+
self.non_contract_call = Some((target, inputs.scheme, ctx.journal_ref().depth()));
136+
}
137+
}
138+
None
139+
}
140+
141+
/// Handles `REVERT` and `EXTCODESIZE` opcodes for diagnostics.
142+
///
143+
/// When a `REVERT` opcode with zero data size occurs:
144+
/// - if `non_contract_call` was set at the current depth, `handle_revert_diagnostic` is
145+
/// called. Otherwise, it is cleared.
146+
/// - if `non_contract_call` was set at the current depth, `handle_revert_diagnostic` is
147+
/// called. Otherwise, it is cleared.
148+
///
149+
/// When an `EXTCODESIZE` opcode occurs:
150+
/// - Optimistically caches the target address and current depth in `non_contract_size_check`,
151+
/// pending later validation.
152+
fn step(&mut self, interp: &mut Interpreter, ctx: &mut CTX) {
153+
// REVERT (offset, size)
154+
if REVERT == interp.bytecode.opcode() {
155+
if let Ok(size) = interp.stack.peek(1) {
156+
if size.is_zero() {
157+
// Check empty revert with same depth as a non-contract call
158+
if let Some((_, _, depth)) = self.non_contract_call {
159+
if ctx.journal_ref().depth() == depth {
160+
self.handle_revert_diagnostic(interp);
161+
} else {
162+
self.non_contract_call = None;
163+
}
164+
return;
165+
}
166+
167+
// Check empty revert with same depth as a non-contract size check
168+
if let Some((_, depth)) = self.non_contract_size_check {
169+
if depth == ctx.journal_ref().depth() {
170+
self.handle_revert_diagnostic(interp);
171+
} else {
172+
self.non_contract_size_check = None;
173+
}
174+
}
175+
}
176+
}
177+
}
178+
// EXTCODESIZE (address)
179+
else if EXTCODESIZE == interp.bytecode.opcode() {
180+
if let Ok(word) = interp.stack.peek(0) {
181+
let addr = Address::from_word(word.into());
182+
if IGNORE.contains(&addr) ||
183+
ctx.journal_ref().precompile_addresses().contains(&addr)
184+
{
185+
return;
186+
}
187+
188+
// Optimistically cache --> validated and cleared (if necessary) at `fn step_end()`
189+
self.non_contract_size_check = Some((addr, ctx.journal_ref().depth()));
190+
self.is_extcodesize_step = true;
191+
}
192+
}
193+
}
194+
195+
/// Tracks `EXTCODESIZE` output. If the bytecode size is 0, clears the cache.
196+
fn step_end(&mut self, interp: &mut Interpreter, _ctx: &mut CTX) {
197+
if self.is_extcodesize_step {
198+
if let Ok(size) = interp.stack.peek(0) {
199+
if size != U256::ZERO {
200+
self.non_contract_size_check = None;
201+
}
202+
}
203+
204+
self.is_extcodesize_step = false;
205+
}
206+
}
207+
}

crates/evm/evm/src/inspectors/stack.rs

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use super::{
22
Cheatcodes, CheatsConfig, ChiselState, CoverageCollector, CustomPrintTracer, Fuzzer,
3-
LogCollector, ScriptExecutionInspector, TracingInspector,
3+
LogCollector, RevertDiagnostic, ScriptExecutionInspector, TracingInspector,
44
};
55
use alloy_evm::{eth::EthEvmContext, Evm};
66
use alloy_primitives::{
@@ -50,7 +50,7 @@ pub struct InspectorStackBuilder {
5050
pub cheatcodes: Option<Arc<CheatsConfig>>,
5151
/// The fuzzer inspector and its state, if it exists.
5252
pub fuzzer: Option<Fuzzer>,
53-
/// Whether to enable tracing.
53+
/// Whether to enable tracing and revert diagnostics.
5454
pub trace_mode: TraceMode,
5555
/// Whether logs should be collected.
5656
pub logs: Option<bool>,
@@ -143,6 +143,7 @@ impl InspectorStackBuilder {
143143
}
144144

145145
/// Set whether to enable the tracer.
146+
/// Revert diagnostic inspector is activated when `mode != TraceMode::None`
146147
#[inline]
147148
pub fn trace_mode(mut self, mode: TraceMode) -> Self {
148149
if self.trace_mode < mode {
@@ -304,6 +305,7 @@ pub struct InspectorStackInner {
304305
pub enable_isolation: bool,
305306
pub odyssey: bool,
306307
pub create2_deployer: Address,
308+
pub revert_diag: Option<RevertDiagnostic>,
307309

308310
/// Flag marking if we are in the inner EVM context.
309311
pub in_inner_context: bool,
@@ -439,8 +441,15 @@ impl InspectorStack {
439441
}
440442

441443
/// Set whether to enable the tracer.
444+
/// Revert diagnostic inspector is activated when `mode != TraceMode::None`
442445
#[inline]
443446
pub fn tracing(&mut self, mode: TraceMode) {
447+
if mode.is_none() {
448+
self.revert_diag = None;
449+
} else {
450+
self.revert_diag = Some(RevertDiagnostic::default());
451+
}
452+
444453
if let Some(config) = mode.into_config() {
445454
*self.tracer.get_or_insert_with(Default::default).config_mut() = config;
446455
} else {
@@ -520,7 +529,13 @@ impl InspectorStackRefMut<'_> {
520529
let result = outcome.result.result;
521530
call_inspectors!(
522531
#[ret]
523-
[&mut self.fuzzer, &mut self.tracer, &mut self.cheatcodes, &mut self.printer],
532+
[
533+
&mut self.fuzzer,
534+
&mut self.tracer,
535+
&mut self.cheatcodes,
536+
&mut self.printer,
537+
&mut self.revert_diag
538+
],
524539
|inspector| {
525540
let previous_outcome = outcome.clone();
526541
inspector.call_end(ecx, inputs, outcome);
@@ -801,7 +816,8 @@ impl Inspector<EthEvmContext<&mut dyn DatabaseExt>> for InspectorStackRefMut<'_>
801816
&mut self.coverage,
802817
&mut self.cheatcodes,
803818
&mut self.script_execution_inspector,
804-
&mut self.printer
819+
&mut self.printer,
820+
&mut self.revert_diag
805821
],
806822
|inspector| inspector.step(interpreter, ecx),
807823
);
@@ -813,7 +829,13 @@ impl Inspector<EthEvmContext<&mut dyn DatabaseExt>> for InspectorStackRefMut<'_>
813829
ecx: &mut EthEvmContext<&mut dyn DatabaseExt>,
814830
) {
815831
call_inspectors!(
816-
[&mut self.tracer, &mut self.cheatcodes, &mut self.chisel_state, &mut self.printer],
832+
[
833+
&mut self.tracer,
834+
&mut self.cheatcodes,
835+
&mut self.chisel_state,
836+
&mut self.printer,
837+
&mut self.revert_diag
838+
],
817839
|inspector| inspector.step_end(interpreter, ecx),
818840
);
819841
}
@@ -846,7 +868,13 @@ impl Inspector<EthEvmContext<&mut dyn DatabaseExt>> for InspectorStackRefMut<'_>
846868

847869
call_inspectors!(
848870
#[ret]
849-
[&mut self.fuzzer, &mut self.tracer, &mut self.log_collector, &mut self.printer],
871+
[
872+
&mut self.fuzzer,
873+
&mut self.tracer,
874+
&mut self.log_collector,
875+
&mut self.printer,
876+
&mut self.revert_diag
877+
],
850878
|inspector| {
851879
let mut out = None;
852880
if let Some(output) = inspector.call(ecx, call) {

0 commit comments

Comments
 (0)