Skip to content
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

feat(cheatcodes): additional cheatcodes to aid in symbolic testing #8807

Merged
merged 7 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions crates/cheatcodes/assets/cheatcodes.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions crates/cheatcodes/spec/src/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,15 @@ interface Vm {
function mockCallRevert(address callee, uint256 msgValue, bytes calldata data, bytes calldata revertData)
external;

/// Whenever a call is made to `callee` with calldata `data`, this cheatcode instead calls
/// `target` with the same calldata. This functionality is similar to a delegate call made to
/// `target` contract from `callee`.
/// Can be used to substitute a call to a function with another implementation that captures
/// the primary logic of the original function but is easier to reason about.
/// If calldata is not a strict match then partial match by selector is attempted.
#[cheatcode(group = Evm, safety = Unsafe)]
function mockFunction(address callee, address target, bytes calldata data) external;

// --- Impersonation (pranks) ---

/// Sets the *next* call's `msg.sender` to be the input address.
Expand Down Expand Up @@ -2303,6 +2312,14 @@ interface Vm {
/// Unpauses collection of call traces.
#[cheatcode(group = Utilities)]
function resumeTracing() external view;

/// Utility cheatcode to copy storage of `from` contract to another `to` contract.
#[cheatcode(group = Utilities)]
function copyStorage(address from, address to) external;

/// Utility cheatcode to set arbitrary storage for given target address.
#[cheatcode(group = Utilities)]
function setArbitraryStorage(address target) external;
}
}

Expand Down
11 changes: 9 additions & 2 deletions crates/cheatcodes/src/evm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ use foundry_evm_core::{
backend::{DatabaseExt, RevertSnapshotAction},
constants::{CALLER, CHEATCODE_ADDRESS, HARDHAT_CONSOLE_ADDRESS, TEST_CONTRACT_ADDRESS},
};
use rand::Rng;
use revm::{
primitives::{Account, Bytecode, SpecId, KECCAK_EMPTY},
primitives::{Account, Bytecode, EvmStorageSlot, SpecId, KECCAK_EMPTY},
InnerEvmContext,
};
use std::{
Expand Down Expand Up @@ -89,7 +90,13 @@ impl Cheatcode for loadCall {
let Self { target, slot } = *self;
ensure_not_precompile!(&target, ccx);
ccx.ecx.load_account(target)?;
let val = ccx.ecx.sload(target, slot.into())?;
let mut val = ccx.ecx.sload(target, slot.into())?;
// Generate random value if target should have arbitrary storage and storage slot untouched.
if ccx.state.arbitrary_storage.contains(&target) && val.is_cold && val.data.is_zero() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's considered "untouched" here? eg in a single test calling the same contract two times from test scope would result in slot being cold on first call, and warm on second due to how our EVM works. is this expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, that's the expected behavior, subsequent loads of same slot should return same value

// Load slot again and make sure we get same value.

unless explicitly rewritten

// This should remain 0 if explicitly set.

val.data = ccx.state.rng().gen();
let mut account = ccx.ecx.load_account(target)?;
account.storage.insert(slot.into(), EvmStorageSlot::new(val.data));
}
Ok(val.abi_encode())
}
}
Expand Down
9 changes: 9 additions & 0 deletions crates/cheatcodes/src/evm/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,15 @@ impl Cheatcode for mockCallRevert_1Call {
}
}

impl Cheatcode for mockFunctionCall {
fn apply(&self, state: &mut Cheatcodes) -> Result {
let Self { callee, target, data } = self;
state.mocked_functions.entry(*callee).or_default().insert(data.clone(), *target);

Ok(Default::default())
}
}

#[allow(clippy::ptr_arg)] // Not public API, doesn't matter
fn mock_call(
state: &mut Cheatcodes,
Expand Down
37 changes: 35 additions & 2 deletions crates/cheatcodes/src/inspector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use revm::{
EOFCreateInputs, EOFCreateKind, Gas, InstructionResult, Interpreter, InterpreterAction,
InterpreterResult,
},
primitives::{BlockEnv, CreateScheme, EVMError, SpecId, EOF_MAGIC_BYTES},
primitives::{BlockEnv, CreateScheme, EVMError, EvmStorageSlot, SpecId, EOF_MAGIC_BYTES},
EvmContext, InnerEvmContext, Inspector,
};
use rustc_hash::FxHashMap;
Expand Down Expand Up @@ -320,6 +320,9 @@ pub struct Cheatcodes {
// **Note**: inner must a BTreeMap because of special `Ord` impl for `MockCallDataContext`
pub mocked_calls: HashMap<Address, BTreeMap<MockCallDataContext, MockCallReturnData>>,

/// Mocked functions. Maps target address to be mocked to pair of (calldata, mock address).
pub mocked_functions: HashMap<Address, HashMap<Bytes, Address>>,

/// Expected calls
pub expected_calls: ExpectedCallTracker,
/// Expected emits
Expand Down Expand Up @@ -368,6 +371,10 @@ pub struct Cheatcodes {

/// Ignored traces.
pub ignored_traces: IgnoredTraces,

/// Addresses that should have arbitrary storage generated (SLOADs return random value if
/// storage slot wasn't accessed).
pub arbitrary_storage: Vec<Address>,
}

// This is not derived because calling this in `fn new` with `..Default::default()` creates a second
Expand Down Expand Up @@ -396,6 +403,7 @@ impl Cheatcodes {
recorded_account_diffs_stack: Default::default(),
recorded_logs: Default::default(),
mocked_calls: Default::default(),
mocked_functions: Default::default(),
expected_calls: Default::default(),
expected_emits: Default::default(),
allowed_mem_writes: Default::default(),
Expand All @@ -410,6 +418,7 @@ impl Cheatcodes {
breakpoints: Default::default(),
rng: Default::default(),
ignored_traces: Default::default(),
arbitrary_storage: Default::default(),
}
}

Expand Down Expand Up @@ -1045,14 +1054,18 @@ impl<DB: DatabaseExt> Inspector<DB> for Cheatcodes {
}

#[inline]
fn step_end(&mut self, interpreter: &mut Interpreter, _ecx: &mut EvmContext<DB>) {
fn step_end(&mut self, interpreter: &mut Interpreter, ecx: &mut EvmContext<DB>) {
if self.gas_metering.paused {
self.meter_gas_end(interpreter);
}

if self.gas_metering.touched {
self.meter_gas_check(interpreter);
}

if self.arbitrary_storage.contains(&interpreter.contract().target_address) {
self.ensure_arbitrary_storage(interpreter, ecx);
}
}

fn log(&mut self, interpreter: &mut Interpreter, _ecx: &mut EvmContext<DB>, log: &Log) {
Expand Down Expand Up @@ -1465,6 +1478,26 @@ impl Cheatcodes {
}
}

/// Generates arbitrary values for storage slots.
#[cold]
fn ensure_arbitrary_storage<DB: DatabaseExt>(
&mut self,
interpreter: &mut Interpreter,
ecx: &mut EvmContext<DB>,
) {
if interpreter.current_opcode() == op::SLOAD {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should move the SLOAD check outside into step_end, and make it call one arbitrary_storage_end to avoid callsite bloat

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in 7e69723

let key = try_or_return!(interpreter.stack().peek(0));
let target_address = interpreter.contract().target_address;
if let Ok(value) = ecx.sload(target_address, key) {
if value.is_cold && value.data.is_zero() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I think this would never be cold because revm marks slot as warm during execution of opcode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though it looks like tests are passing? would need to look closer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be because we're reading the value instead of a key in interpreter.stack().peek(0). on step_end stack already contains read value

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's my take too, the opcode is not yet executed / slot marked as warm at this point

Copy link
Member

@klkvr klkvr Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah interpreter.current_opcode() returns next opcode in step_end, so it's fine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's my take too, the opcode is not yet executed / slot marked as warm at this point

I was thinking that we're entering ensure_arbitrary_storage after revm executed the opcode, so it's my bad

though can we document what's happening here anyway? especially that this would happen before sload even though we are in _end hook

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will add comment to make this clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klkvr please check 16f068f Had to accommodate copy from arbitrary storage scenario pointed here #8807 (comment) too, so a little bit bigger change, ptal. thanks!

if let Ok(mut target_account) = ecx.load_account(target_address) {
target_account.storage.insert(key, EvmStorageSlot::new(self.rng().gen()));
}
}
}
}
}

/// Records storage slots reads and writes.
#[cold]
fn record_accesses(&mut self, interpreter: &mut Interpreter) {
Expand Down
24 changes: 23 additions & 1 deletion crates/cheatcodes/src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Implementations of [`Utilities`](spec::Group::Utilities) cheatcodes.

use crate::{Cheatcode, Cheatcodes, Result, Vm::*};
use crate::{Cheatcode, Cheatcodes, CheatsCtxt, Result, Vm::*};
use alloy_primitives::{Address, U256};
use alloy_sol_types::SolValue;
use foundry_common::ens::namehash;
Expand Down Expand Up @@ -149,3 +149,25 @@ impl Cheatcode for resumeTracingCall {
Ok(Default::default())
}
}
impl Cheatcode for setArbitraryStorageCall {
grandizzy marked this conversation as resolved.
Show resolved Hide resolved
fn apply_stateful<DB: DatabaseExt>(&self, ccx: &mut CheatsCtxt<DB>) -> Result {
let Self { target } = self;
ccx.state.arbitrary_storage.push(*target);

Ok(Default::default())
}
}

impl Cheatcode for copyStorageCall {
fn apply_stateful<DB: DatabaseExt>(&self, ccx: &mut CheatsCtxt<DB>) -> Result {
let Self { from, to } = self;
if let Ok(from_account) = ccx.load_account(*from) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this won't work for forking but Ig it's out of scope?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, wasn't discussed to work for forking but could be added later if needed

let from_storage = from_account.storage.clone();
if let Ok(mut to_account) = ccx.load_account(*to) {
to_account.storage = from_storage;
}
}

Ok(Default::default())
}
}
12 changes: 12 additions & 0 deletions crates/evm/evm/src/inspectors/stack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,18 @@ impl<'a, DB: DatabaseExt> Inspector<DB> for InspectorStackRefMut<'a> {

ecx.journaled_state.depth += self.in_inner_context as usize;
if let Some(cheatcodes) = self.cheatcodes.as_deref_mut() {
// Handle mocked functions, replace bytecode address with mock if matched.
if let Some(mocks) = cheatcodes.mocked_functions.get(&call.target_address) {
if let Some(target) = mocks.get(&call.input) {
call.bytecode_address = *target;
} else {
// Check if we have a catch-all mock set for selector.
if let Some(target) = mocks.get(&call.input.slice(..4)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can collapse into one get().or_else(|| ), also this panics if call.input.len() < 4, use call.input.get(..4)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in 7e69723

call.bytecode_address = *target;
}
}
}

if let Some(output) = cheatcodes.call_with_executor(ecx, call, self.inner) {
if output.result.result != InstructionResult::Continue {
ecx.journaled_state.depth -= self.in_inner_context as usize;
Expand Down
2 changes: 2 additions & 0 deletions crates/forge/tests/it/cheats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::{
TEST_DATA_MULTI_VERSION,
},
};
use alloy_primitives::U256;
use foundry_config::{fs_permissions::PathPermission, FsPermissions};
use foundry_test_utils::Filter;

Expand All @@ -26,6 +27,7 @@ async fn test_cheats_local(test_data: &ForgeTestData) {
}

let mut config = test_data.config.clone();
config.fuzz.seed = Some(U256::from(100));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be set in test_data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, not sure we should be adding a new TEST_DATA_WITH_SEED (we don't have one for isolate either) but default tests shouldn't run with seed - since there's no option to set it inline I've added a test that runs contracts matching WithSeed (excluded from defaults), hope this makes sense
pls check 7e69723

config.fs_permissions = FsPermissions::new(vec![PathPermission::read_write("./")]);
let runner = test_data.runner_with_config(config);

Expand Down
3 changes: 3 additions & 0 deletions testdata/cheats/Vm.sol

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading