Skip to content

feat(script): add warning for address(this) usage #10295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
5 changes: 5 additions & 0 deletions crates/evm/evm/src/executors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,11 @@ impl Executor {
self
}

#[inline]
pub fn set_script(&mut self, script_address: Address) {
self.inspector_mut().script(script_address);
}

#[inline]
pub fn set_trace_printer(&mut self, trace_printer: bool) -> &mut Self {
self.inspector_mut().print(trace_printer);
Expand Down
3 changes: 3 additions & 0 deletions crates/evm/evm/src/inspectors/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,8 @@ pub use chisel_state::ChiselState;
mod logs;
pub use logs::LogCollector;

mod script;
pub use script::ScriptExecutionInspector;

mod stack;
pub use stack::{InspectorData, InspectorStack, InspectorStackBuilder};
35 changes: 35 additions & 0 deletions crates/evm/evm/src/inspectors/script.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
use alloy_primitives::Address;
use revm::{
interpreter::{opcode::ADDRESS, InstructionResult, Interpreter},
Database, EvmContext, Inspector,
};

/// An inspector that enforces certain rules during script execution.
///
/// Currently, it only warns if the `ADDRESS` opcode is used within the script's main contract.
#[derive(Clone, Debug, Default)]
pub struct ScriptExecutionInspector {
/// The address of the script contract being executed.
pub script_address: Address,
}

impl<DB: Database> Inspector<DB> for ScriptExecutionInspector {
#[cold]
fn step(&mut self, interpreter: &mut Interpreter, _ecx: &mut EvmContext<DB>) {
// Check for address(this) usage in the main script contract
if interpreter.current_opcode() == ADDRESS &&
interpreter.contract.target_address == self.script_address
{
// Log the reason for revert
tracing::error!(
target: "forge::script",
"Usage of `address(this)` detected in script contract. Script contracts are ephemeral and their addresses should not be relied upon."
);
// Set the instruction result to Revert to stop execution
interpreter.instruction_result = InstructionResult::Revert;
}
// Note: We don't return anything here as step returns void.
// The original check returned InstructionResult::Continue, but that's the default
// behavior.
}
}
22 changes: 19 additions & 3 deletions crates/evm/evm/src/inspectors/stack.rs

This comment was marked as spam.

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::{
Cheatcodes, CheatsConfig, ChiselState, CoverageCollector, Fuzzer, LogCollector,
TracingInspector,
ScriptExecutionInspector, TracingInspector,
};
use alloy_primitives::{map::AddressHashMap, Address, Bytes, Log, TxKind, U256};
use foundry_cheatcodes::{CheatcodesExecutor, Wallets};
Expand Down Expand Up @@ -199,6 +199,7 @@ impl InspectorStackBuilder {
if let Some(chisel_state) = chisel_state {
stack.set_chisel(chisel_state);
}

stack.collect_coverage(coverage.unwrap_or(false));
stack.collect_logs(logs.unwrap_or(true));
stack.print(print.unwrap_or(false));
Expand Down Expand Up @@ -290,6 +291,7 @@ pub struct InspectorStackInner {
pub log_collector: Option<LogCollector>,
pub printer: Option<CustomPrintTracer>,
pub tracer: Option<TracingInspector>,
pub script_execution_inspector: Option<ScriptExecutionInspector>,
pub enable_isolation: bool,
pub odyssey: bool,
pub create2_deployer: Address,
Expand Down Expand Up @@ -437,6 +439,13 @@ impl InspectorStack {
}
}

/// Set whether to enable script execution inspector.
#[inline]
pub fn script(&mut self, script_address: Address) {
self.script_execution_inspector.get_or_insert_with(Default::default).script_address =
script_address;
}

/// Collects all the data gathered during inspection into a single struct.
#[inline]
pub fn collect(self) -> InspectorData {
Expand Down Expand Up @@ -757,7 +766,13 @@ impl Inspector<&mut dyn DatabaseExt> for InspectorStackRefMut<'_> {
ecx: &mut EvmContext<&mut dyn DatabaseExt>,
) {
call_inspectors!(
[&mut self.coverage, &mut self.tracer, &mut self.cheatcodes, &mut self.printer],
[
&mut self.coverage,
&mut self.tracer,
&mut self.cheatcodes,
&mut self.script_execution_inspector,
&mut self.printer
],
|inspector| inspector.initialize_interp(interpreter, ecx),
);
}
Expand All @@ -769,7 +784,8 @@ impl Inspector<&mut dyn DatabaseExt> for InspectorStackRefMut<'_> {
&mut self.tracer,
&mut self.coverage,
&mut self.cheatcodes,
&mut self.printer,
&mut self.script_execution_inspector,
&mut self.printer
],
|inspector| inspector.step(interpreter, ecx),
);
Expand Down
29 changes: 29 additions & 0 deletions crates/forge/tests/cli/script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2645,3 +2645,32 @@ Script ran successfully.

"#]]);
});

// Tests that script reverts if it uses `address(this)`.
forgetest_init!(should_revert_on_address_opcode, |prj, cmd| {
prj.add_script(
"ScriptWithAddress.s.sol",
r#"
import {Script, console} from "forge-std/Script.sol";

contract ScriptWithAddress is Script {
function run() public view {
console.log("script address", address(this));
}
}
"#,
)
.unwrap();

cmd.arg("script").arg("ScriptWithAddress").assert_failure().stdout_eq(str![[r#"
...
[..] ERROR forge::script: Usage of `address(this)` detected in script contract. Script contracts are ephemeral and their addresses should not be relied upon.
...

"#]]).stderr_eq(str![[r#"
...
Error: script failed: <empty revert data>
...

"#]]);
});
3 changes: 3 additions & 0 deletions crates/script/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ impl ScriptRunner {
self.executor.set_nonce(self.evm_opts.sender, prev_sender_nonce)?;
}

// set script address to be used by execution inspector
self.executor.set_script(address);

traces.extend(constructor_traces.map(|traces| (TraceKind::Deployment, traces)));

// Optionally call the `setUp` function
Expand Down
Loading