diff --git a/crates/edr_napi/index.d.ts b/crates/edr_napi/index.d.ts index ef4cc51b3..806525216 100644 --- a/crates/edr_napi/index.d.ts +++ b/crates/edr_napi/index.d.ts @@ -515,8 +515,8 @@ export interface SolidityTestRunnerConfigArgs { } /** Fuzz testing configuration */ export interface FuzzConfigArgs { - /** Path where fuzz failures are recorded and replayed. */ - failurePersistDir: string + /** Path where fuzz failures are recorded and replayed if set. */ + failurePersistDir?: string /** Name of the file to record fuzz failures, defaults to `failures`. */ failurePersistFile?: string /** @@ -560,7 +560,7 @@ export interface FuzzConfigArgs { } /** Invariant testing configuration. */ export interface InvariantConfigArgs { - /** Path where invariant failures are recorded and replayed. */ + /** Path where invariant failures are recorded and replayed if set. */ failurePersistDir?: string /** * The number of runs that must execute for each invariant test group. diff --git a/crates/edr_napi/src/solidity_tests/config.rs b/crates/edr_napi/src/solidity_tests/config.rs index fd720bc10..7f7ebfd36 100644 --- a/crates/edr_napi/src/solidity_tests/config.rs +++ b/crates/edr_napi/src/solidity_tests/config.rs @@ -182,7 +182,8 @@ impl TryFrom for SolidityTestRunnerConfig { let invariant: InvariantConfig = fuzz .as_ref() - .map(|f| invariant.unwrap_or_default().defaults_from_fuzz(f)) + .map(|f| invariant.clone().unwrap_or_default().defaults_from_fuzz(f)) + .or(invariant) .map(TryFrom::try_from) .transpose()? .unwrap_or_default(); @@ -301,8 +302,8 @@ impl TryFrom for SolidityTestRunnerConfig { #[napi(object)] #[derive(Clone, Default, Debug)] pub struct FuzzConfigArgs { - /// Path where fuzz failures are recorded and replayed. - pub failure_persist_dir: String, + /// Path where fuzz failures are recorded and replayed if set. + pub failure_persist_dir: Option, /// Name of the file to record fuzz failures, defaults to `failures`. pub failure_persist_file: Option, /// The amount of fuzz runs to perform for each fuzz test case. Higher @@ -348,8 +349,8 @@ impl TryFrom for FuzzConfig { include_push_bytes, } = value; - let failure_persist_dir = Some(failure_persist_dir.into()); - let failure_persist_file = Some(failure_persist_file.unwrap_or("failures".to_string())); + let failure_persist_dir = failure_persist_dir.map(PathBuf::from); + let failure_persist_file = failure_persist_file.unwrap_or("failures".to_string()); let seed = seed .map(|s| { s.parse().map_err(|_err| { @@ -362,6 +363,8 @@ impl TryFrom for FuzzConfig { seed, failure_persist_dir, failure_persist_file, + // TODO https://github.com/NomicFoundation/edr/issues/657 + gas_report_samples: 0, ..FuzzConfig::default() }; @@ -393,7 +396,7 @@ impl TryFrom for FuzzConfig { #[napi(object)] #[derive(Clone, Default, Debug)] pub struct InvariantConfigArgs { - /// Path where invariant failures are recorded and replayed. + /// Path where invariant failures are recorded and replayed if set. pub failure_persist_dir: Option, /// The number of runs that must execute for each invariant test group. /// Defaults to 256. @@ -442,7 +445,7 @@ impl InvariantConfigArgs { } = fuzz; if self.failure_persist_dir.is_none() { - self.failure_persist_dir = Some(failure_persist_dir.clone()); + self.failure_persist_dir.clone_from(failure_persist_dir); } if self.runs.is_none() { @@ -483,6 +486,8 @@ impl From for InvariantConfig { let mut invariant = InvariantConfig { failure_persist_dir, + // TODO https://github.com/NomicFoundation/edr/issues/657 + gas_report_samples: 0, ..InvariantConfig::default() }; diff --git a/crates/foundry/config/src/fuzz.rs b/crates/foundry/config/src/fuzz.rs index 366a0e835..1f17717cc 100644 --- a/crates/foundry/config/src/fuzz.rs +++ b/crates/foundry/config/src/fuzz.rs @@ -24,7 +24,7 @@ pub struct FuzzConfig { /// Path where fuzz failures are recorded and replayed. pub failure_persist_dir: Option, /// Name of the file to record fuzz failures, defaults to `failures`. - pub failure_persist_file: Option, + pub failure_persist_file: String, } impl Default for FuzzConfig { @@ -36,7 +36,7 @@ impl Default for FuzzConfig { dictionary: FuzzDictionaryConfig::default(), gas_report_samples: 0, failure_persist_dir: None, - failure_persist_file: None, + failure_persist_file: "failures".into(), } } } @@ -46,13 +46,8 @@ impl FuzzConfig { /// `{PROJECT_ROOT}/cache/fuzz` dir. pub fn new(cache_dir: PathBuf) -> Self { FuzzConfig { - runs: 256, - max_test_rejects: 65536, - seed: None, - dictionary: FuzzDictionaryConfig::default(), - gas_report_samples: 0, failure_persist_dir: Some(cache_dir), - failure_persist_file: Some("failures".to_string()), + ..FuzzConfig::default() } } } diff --git a/crates/foundry/config/src/invariant.rs b/crates/foundry/config/src/invariant.rs index 4ec907731..13806f9c6 100644 --- a/crates/foundry/config/src/invariant.rs +++ b/crates/foundry/config/src/invariant.rs @@ -69,10 +69,16 @@ impl InvariantConfig { } /// Returns path to failure dir of given invariant test contract. - pub fn failure_dir(self, contract_name: &str) -> PathBuf { + pub fn failure_dir(&self, contract_name: &str) -> Option { self.failure_persist_dir - .unwrap() - .join("failures") - .join(contract_name.split(':').last().unwrap()) + .as_ref() + .map(|failure_persist_dir| { + failure_persist_dir.join("failures").join( + contract_name + .split(':') + .last() + .expect("contract name should have solc version suffix"), + ) + }) } } diff --git a/crates/foundry/forge/src/lib.rs b/crates/foundry/forge/src/lib.rs index 935c7230f..5bd52a668 100644 --- a/crates/foundry/forge/src/lib.rs +++ b/crates/foundry/forge/src/lib.rs @@ -1,6 +1,12 @@ #[macro_use] extern crate tracing; +use std::{ + collections::HashSet, + fmt::Debug, + sync::{OnceLock, RwLock}, +}; + use foundry_config::{FuzzConfig, InvariantConfig}; use proptest::test_runner::{ FailurePersistence, FileFailurePersistence, RngAlgorithm, TestRng, TestRunner, @@ -25,6 +31,8 @@ pub mod result; pub use foundry_common::traits::TestFilter; pub use foundry_evm::*; +static FAILURE_PATHS: OnceLock>> = OnceLock::new(); + /// Metadata on how to run fuzz/invariant tests #[derive(Clone, Debug, Default)] pub struct TestOptions { @@ -38,19 +46,46 @@ impl TestOptions { /// Returns a "fuzz" test runner instance. pub fn fuzz_runner(&self) -> TestRunner { let fuzz_config = self.fuzz_config().clone(); - let failure_persist_path = fuzz_config - .failure_persist_dir - .unwrap() - .join(fuzz_config.failure_persist_file.unwrap()) - .into_os_string() - .into_string() - .unwrap(); - self.fuzzer_with_cases( - fuzz_config.runs, - Some(Box::new(FileFailurePersistence::Direct( - failure_persist_path.leak(), - ))), - ) + + if let Some(failure_persist_dir) = fuzz_config.failure_persist_dir { + let failure_persist_path = failure_persist_dir + .join(fuzz_config.failure_persist_file) + .into_os_string() + .into_string() + .expect("path should be valid UTF-8"); + + // HACK: We need to leak the path as + // `proptest::test_runner::FileFailurePersistence` requires a + // `&'static str`. We mitigate this by making sure that one particular path + // is only leaked once. + let failure_paths = FAILURE_PATHS.get_or_init(RwLock::default); + // Need to be in a block to ensure that the read lock is dropped before we try + // to insert. + { + let failure_paths_guard = failure_paths.read().expect("lock is not poisoned"); + if let Some(static_path) = failure_paths_guard.get(&*failure_persist_path) { + return self.fuzzer_with_cases( + fuzz_config.runs, + Some(Box::new(FileFailurePersistence::Direct(static_path))), + ); + } + } + // Write block + { + let mut failure_paths_guard = failure_paths.write().expect("lock is not poisoned"); + failure_paths_guard.insert(failure_persist_path.clone().leak()); + let static_path = failure_paths_guard + .get(&*failure_persist_path) + .expect("must exist since we just inserted it"); + + self.fuzzer_with_cases( + fuzz_config.runs, + Some(Box::new(FileFailurePersistence::Direct(static_path))), + ) + } + } else { + self.fuzzer_with_cases(fuzz_config.runs, None) + } } /// Returns an "invariant" test runner instance. diff --git a/crates/foundry/forge/src/runner.rs b/crates/foundry/forge/src/runner.rs index 8e7fecb82..3f4e51468 100644 --- a/crates/foundry/forge/src/runner.rs +++ b/crates/foundry/forge/src/runner.rs @@ -3,13 +3,14 @@ use std::{ borrow::Cow, cmp::min, collections::{BTreeMap, HashMap}, + path::Path, sync::Arc, time::Instant, }; use alloy_dyn_abi::DynSolValue; use alloy_json_abi::Function; -use alloy_primitives::{Address, U256}; +use alloy_primitives::{Address, Log, U256}; use eyre::Result; use foundry_common::{ contracts::{ContractsByAddress, ContractsByArtifact}, @@ -42,6 +43,7 @@ use crate::{ fuzz::{invariant::BasicTxDetails, BaseCounterExample}, multi_runner::{is_matching_test, TestContract}, result::{SuiteResult, TestKind, TestResult, TestSetup, TestStatus}, + traces::Traces, TestFilter, TestOptions, }; @@ -669,75 +671,23 @@ impl<'a> ContractRunner<'a> { let mut coverage = coverage.clone(); let failure_dir = invariant_config.clone().failure_dir(self.name); - let failure_file = failure_dir.join(invariant_contract.invariant_function.clone().name); + let failure_file = failure_dir + .as_ref() + .map(|failure_dir| failure_dir.join(&invariant_contract.invariant_function.name)); - // Try to replay recorded failure if any. - if let Ok(call_sequence) = - foundry_common::fs::read_json_file::>(failure_file.as_path()) - { - // Create calls from failed sequence and check if invariant still broken. - let txes = call_sequence - .clone() - .into_iter() - .map(|seq| BasicTxDetails { - sender: seq.sender.unwrap_or_default(), - call_details: CallDetails { - target: seq.addr.unwrap_or_default(), - calldata: seq.calldata, - }, - }) - .collect::>(); - if let Ok((success, replayed_entirely)) = check_sequence( - self.executor.clone(), - &txes, - (0..min(txes.len(), invariant_config.depth as usize)).collect(), - invariant_contract.address, - invariant_contract - .invariant_function - .selector() - .to_vec() - .into(), - invariant_config.fail_on_revert, - ) { - if !success { - // If sequence still fails then replay error to collect traces and - // exit without executing new runs. - let _ = replay_run( - &invariant_contract, - self.executor.clone(), - known_contracts, - identified_contracts.clone(), - &mut logs, - &mut traces, - &mut coverage, - txes, - ); - return TestResult { - status: TestStatus::Failure, - reason: if replayed_entirely { - Some(format!( - "{} replay failure", - invariant_contract.invariant_function.name - )) - } else { - Some(format!( - "{} persisted failure revert", - invariant_contract.invariant_function.name - )) - }, - decoded_logs: decode_console_logs(&logs), - traces, - coverage, - counterexample: Some(CounterExample::Sequence(call_sequence)), - kind: TestKind::Invariant { - runs: 1, - calls: 1, - reverts: 1, - }, - duration: start.elapsed(), - ..Default::default() - }; - } + if let Some(failure_file) = failure_file.as_ref() { + if let Some(result) = self.try_to_replay_recorded_failures(ReplayArgs { + failure_file, + invariant_config: &invariant_config, + known_contracts, + identified_contracts, + invariant_contract: &invariant_contract, + logs: &mut logs, + traces: &mut traces, + coverage: &mut coverage, + start: &start, + }) { + return result; } } @@ -795,13 +745,19 @@ impl<'a> ContractRunner<'a> { Ok(call_sequence) => { if !call_sequence.is_empty() { // Persist error in invariant failure dir. - if let Err(err) = foundry_common::fs::create_dir_all(failure_dir) { - error!(%err, "Failed to create invariant failure dir"); - } else if let Err(err) = foundry_common::fs::write_json_file( - failure_file.as_path(), - &call_sequence, - ) { - error!(%err, "Failed to record call sequence"); + if let Some(failure_dir) = failure_dir { + let failure_file = failure_file + .expect("failure file must be some if failure_dir is some"); + if let Err(err) = + foundry_common::fs::create_dir_all(failure_dir) + { + error!(%err, "Failed to create invariant failure dir"); + } else if let Err(err) = foundry_common::fs::write_json_file( + failure_file.as_path(), + &call_sequence, + ) { + error!(%err, "Failed to record call sequence"); + } } counterexample = Some(CounterExample::Sequence(call_sequence)); } @@ -1000,6 +956,102 @@ impl<'a> ContractRunner<'a> { .collect(), } } + + fn try_to_replay_recorded_failures(&'a self, args: ReplayArgs<'a>) -> Option { + let ReplayArgs { + failure_file, + invariant_config, + known_contracts, + identified_contracts, + invariant_contract, + logs, + traces, + coverage, + start, + } = args; + + if let Ok(call_sequence) = + foundry_common::fs::read_json_file::>(failure_file) + { + // Create calls from failed sequence and check if invariant still broken. + let txes = call_sequence + .clone() + .into_iter() + .map(|seq| BasicTxDetails { + sender: seq.sender.unwrap_or_default(), + call_details: CallDetails { + target: seq.addr.unwrap_or_default(), + calldata: seq.calldata, + }, + }) + .collect::>(); + if let Ok((success, replayed_entirely)) = check_sequence( + self.executor.clone(), + &txes, + (0..min(txes.len(), invariant_config.depth as usize)).collect(), + invariant_contract.address, + invariant_contract + .invariant_function + .selector() + .to_vec() + .into(), + invariant_config.fail_on_revert, + ) { + if !success { + // If sequence still fails then replay error to collect traces and + // exit without executing new runs. + let _ = replay_run( + invariant_contract, + self.executor.clone(), + known_contracts, + identified_contracts.clone(), + logs, + traces, + coverage, + txes, + ); + return Some(TestResult { + status: TestStatus::Failure, + reason: if replayed_entirely { + Some(format!( + "{} replay failure", + invariant_contract.invariant_function.name + )) + } else { + Some(format!( + "{} persisted failure revert", + invariant_contract.invariant_function.name + )) + }, + decoded_logs: decode_console_logs(logs), + traces: traces.clone(), + coverage: coverage.clone(), + counterexample: Some(CounterExample::Sequence(call_sequence)), + kind: TestKind::Invariant { + runs: 1, + calls: 1, + reverts: 1, + }, + duration: start.elapsed(), + ..Default::default() + }); + } + } + } + None + } +} + +struct ReplayArgs<'a> { + failure_file: &'a Path, + invariant_config: &'a InvariantConfig, + known_contracts: &'a ContractsByArtifact, + identified_contracts: &'a ContractsByAddress, + invariant_contract: &'a InvariantContract<'a>, + logs: &'a mut Vec, + traces: &'a mut Traces, + coverage: &'a mut Option, + start: &'a Instant, } /// Utility function to merge coverage options diff --git a/crates/foundry/forge/tests/it/fuzz.rs b/crates/foundry/forge/tests/it/fuzz.rs index cfb2cdfc8..e061c05b6 100644 --- a/crates/foundry/forge/tests/it/fuzz.rs +++ b/crates/foundry/forge/tests/it/fuzz.rs @@ -170,7 +170,7 @@ async fn test_persist_fuzz_failure() { } // write new failure in different file, but keep the same directory - fuzz_config.failure_persist_file = Some("failure1".to_string()); + fuzz_config.failure_persist_file = "failure1".to_string(); let runner = TEST_DATA_DEFAULT.runner_with_fuzz_config(fuzz_config).await; let new_calldata = match get_failure_result!(runner) { Some(CounterExample::Single(counterexample)) => counterexample.calldata, diff --git a/crates/foundry/forge/tests/it/test_helpers.rs b/crates/foundry/forge/tests/it/test_helpers.rs index 870b46319..3696a0c69 100644 --- a/crates/foundry/forge/tests/it/test_helpers.rs +++ b/crates/foundry/forge/tests/it/test_helpers.rs @@ -132,7 +132,7 @@ pub struct TestFuzzConfig { pub dictionary: TestFuzzDictionaryConfig, pub gas_report_samples: u32, pub failure_persist_dir: Option, - pub failure_persist_file: Option, + pub failure_persist_file: String, } impl Default for TestFuzzConfig { @@ -144,7 +144,7 @@ impl Default for TestFuzzConfig { dictionary: TestFuzzDictionaryConfig::default(), gas_report_samples: 256, failure_persist_dir: Some(tempfile::tempdir().unwrap().into_path()), - failure_persist_file: Some("testfailure".to_string()), + failure_persist_file: "testfailure".into(), } } } diff --git a/js/integration-tests/solidity-tests/contracts/ForkCheatcode.t.sol b/js/integration-tests/solidity-tests/contracts/ForkCheatcode.t.sol index b219bfa13..643e74a6a 100644 --- a/js/integration-tests/solidity-tests/contracts/ForkCheatcode.t.sol +++ b/js/integration-tests/solidity-tests/contracts/ForkCheatcode.t.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.24; - import "./test.sol"; import "./Vm.sol"; diff --git a/js/integration-tests/solidity-tests/contracts/Invariant.t.sol b/js/integration-tests/solidity-tests/contracts/Invariant.t.sol new file mode 100644 index 000000000..e2babc6c0 --- /dev/null +++ b/js/integration-tests/solidity-tests/contracts/Invariant.t.sol @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "./test.sol"; +import "./Vm.sol"; + +contract StochasticWrongContract { + uint256 public a; + uint256 public b; + uint256 public both; + + function addToA(uint256 amount) external { + // This is an intentional bug in the contract to trigger invariant failure. + // If the conditional is removed, the invariant will pass. + if (amount % 13 != 0) { + a += amount; + } + both += amount; + } + + function addToB(uint256 amount) external { + b += amount; + both += amount; + } +} + +// Test that the invariant testing works correctly by catching a bug in the contract. +contract FailingInvariantTest is DSTest { + StochasticWrongContract wrongContract; + + function setUp() external { + wrongContract = new StochasticWrongContract(); + } + + function invariant() external { + assertEq(wrongContract.a() + wrongContract.b(), wrongContract.both()); + } +} + diff --git a/js/integration-tests/solidity-tests/contracts/Overflow.t.sol b/js/integration-tests/solidity-tests/contracts/Overflow.t.sol new file mode 100644 index 000000000..910682a91 --- /dev/null +++ b/js/integration-tests/solidity-tests/contracts/Overflow.t.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.24; + +import "./test.sol"; +import "./Vm.sol"; + +// Contract to be tested with overflow vulnerability +contract MyContract { + function addWithOverflow(uint256 a, uint256 b) public pure returns (uint256) { + return a + b; + } +} + +// Test that the fuzzing catches overflows +contract OverflowTest is DSTest { + Vm constant vm = Vm(HEVM_ADDRESS); + + MyContract public myContract; + + function setUp() public { + myContract = new MyContract(); + } + + function testFuzzAddWithOverflow(uint256 a, uint256 b) public { + uint256 result = myContract.addWithOverflow(a, b); + assertEq(result, a + b); + } +} diff --git a/js/integration-tests/solidity-tests/test/fuzz.ts b/js/integration-tests/solidity-tests/test/fuzz.ts new file mode 100644 index 000000000..d12bc88f5 --- /dev/null +++ b/js/integration-tests/solidity-tests/test/fuzz.ts @@ -0,0 +1,113 @@ +import chai, { assert, expect } from "chai"; +import { TestContext } from "./testContext"; +import fs from "node:fs/promises"; +import { existsSync } from "node:fs"; +import { FuzzTestKind } from "@nomicfoundation/edr"; +import { runAllSolidityTests } from "@nomicfoundation/edr-helpers"; + +describe("Fuzz and invariant testing", function () { + let testContext: TestContext; + + before(async function () { + testContext = await TestContext.setup(); + }); + + it("FailingFuzz", async function () { + const failureDir = testContext.fuzzFailuresPersistDir; + + // Remove invariant config failure directory to make sure it's created fresh. + await fs.rm(failureDir, { + recursive: true, + force: true, + }); + + const result1 = await testContext.runTestsWithStats("OverflowTest"); + assert.equal(result1.failedTests, 1); + assert.equal(result1.totalTests, 1); + + // The fuzz failure directory should not be created if we don't set the directory + assert.isFalse(existsSync(failureDir)); + + const result2 = await testContext.runTestsWithStats("OverflowTest", { + fuzz: { + failurePersistDir: failureDir, + }, + }); + assert.equal(result2.failedTests, 1); + assert.equal(result2.totalTests, 1); + + // The fuzz failure directory should now be created + assert.isTrue(existsSync(failureDir)); + }); + + // One test as steps should be sequential + it("FailingInvariant", async function () { + const failureDir = testContext.invariantFailuresPersistDir; + const invariantConfig = { + runs: 256, + depth: 15, + // This is false by default, we just specify it here to make it obvious to the reader. + failOnRevert: false, + }; + + // Remove invariant config failure directory to make sure it's created fresh. + await fs.rm(failureDir, { + recursive: true, + force: true, + }); + + const result1 = await testContext.runTestsWithStats( + "FailingInvariantTest", + { + invariant: invariantConfig, + } + ); + assert.equal(result1.failedTests, 1); + assert.equal(result1.totalTests, 1); + + // The invariant failure directory should not be created if we don't set the directory + assert.isFalse(existsSync(failureDir)); + + const results2 = await runAllSolidityTests( + testContext.artifacts, + testContext.matchingTest("FailingInvariantTest"), + { + ...testContext.defaultConfig(), + invariant: { + ...invariantConfig, + failurePersistDir: failureDir, + }, + } + ); + assert.equal(results2.length, 1); + assert.equal(results2[0].testResults.length, 1); + assert.equal(results2[0].testResults[0].status, "Failure"); + const fuzzTestResult2 = results2[0].testResults[0].kind as FuzzTestKind; + // More than one run should be needed on a fresh invariant test. + assert.isTrue(fuzzTestResult2.runs > 1n); + + // The invariant failure directory should now be created + assert.isTrue(existsSync(failureDir)); + + const results3 = await runAllSolidityTests( + testContext.artifacts, + testContext.matchingTest("FailingInvariantTest"), + { + ...testContext.defaultConfig(), + invariant: { + ...invariantConfig, + failurePersistDir: failureDir, + }, + } + ); + + // The invariant failure directory should still be there + assert.isTrue(existsSync(failureDir)); + assert.equal(results3.length, 1); + assert.equal(results3[0].testResults.length, 1); + assert.equal(results3[0].testResults[0].status, "Failure"); + const fuzzTestResult3 = results3[0].testResults[0].kind as FuzzTestKind; + // The second time only one run should be needed, because the persisted failure is used. + assert.equal(fuzzTestResult3.runs, 1n); + }); +}); diff --git a/js/integration-tests/solidity-tests/test/testContext.ts b/js/integration-tests/solidity-tests/test/testContext.ts index 2773751e3..ab42fe2a6 100644 --- a/js/integration-tests/solidity-tests/test/testContext.ts +++ b/js/integration-tests/solidity-tests/test/testContext.ts @@ -36,24 +36,6 @@ export class TestContext { }; } - fuzzConfig( - config?: Omit - ): FuzzConfigArgs { - return { - failurePersistDir: this.fuzzFailuresPersistDir, - ...config, - }; - } - - invariantConfig( - config?: Omit - ): InvariantConfigArgs { - return { - failurePersistDir: this.invariantFailuresPersistDir, - ...config, - }; - } - async runTestsWithStats( contractName: string, config?: Omit