From dcaf6676d634fc1a8ef667182e2b176fd7b86016 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Mon, 12 Jun 2023 12:57:23 -0400 Subject: [PATCH 1/3] feat(cheatcodes): disallow usage of expectCall/Emit with expectRevert --- .../executor/inspector/cheatcodes/expect.rs | 61 +++++++++---------- evm/src/executor/inspector/cheatcodes/mod.rs | 9 +++ 2 files changed, 38 insertions(+), 32 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/expect.rs b/evm/src/executor/inspector/cheatcodes/expect.rs index 2b0e04059fdb..4dbdf5697edc 100644 --- a/evm/src/executor/inspector/cheatcodes/expect.rs +++ b/evm/src/executor/inspector/cheatcodes/expect.rs @@ -45,6 +45,16 @@ fn expect_revert(state: &mut Cheatcodes, reason: Option, depth: u64) -> R Ok(Bytes::new()) } +fn expect_emit( + state: &mut Cheatcodes, + address: Option, + depth: u64, + checks: [bool; 4], +) -> Result { + state.expected_emits.push_back(ExpectedEmit { depth, address, checks, ..Default::default() }); + Ok(Bytes::new()) +} + #[instrument(skip_all, fields(expected_revert, status, retdata = hex::encode(&retdata)))] pub fn handle_expect_revert( is_create: bool, @@ -347,39 +357,26 @@ pub fn apply( expect_revert(state, Some(inner.0.into()), data.journaled_state.depth()) } HEVMCalls::ExpectEmit0(_) => { - state.expected_emits.push_back(ExpectedEmit { - depth: data.journaled_state.depth(), - checks: [true, true, true, true], - ..Default::default() - }); - Ok(Bytes::new()) - } - HEVMCalls::ExpectEmit1(inner) => { - state.expected_emits.push_back(ExpectedEmit { - depth: data.journaled_state.depth(), - checks: [true, true, true, true], - address: Some(inner.0), - ..Default::default() - }); - Ok(Bytes::new()) - } - HEVMCalls::ExpectEmit2(inner) => { - state.expected_emits.push_back(ExpectedEmit { - depth: data.journaled_state.depth(), - checks: [inner.0, inner.1, inner.2, inner.3], - ..Default::default() - }); - Ok(Bytes::new()) - } - HEVMCalls::ExpectEmit3(inner) => { - state.expected_emits.push_back(ExpectedEmit { - depth: data.journaled_state.depth(), - checks: [inner.0, inner.1, inner.2, inner.3], - address: Some(inner.4), - ..Default::default() - }); - Ok(Bytes::new()) + expect_emit(state, None, data.journaled_state.depth(), [true, true, true, true]) } + HEVMCalls::ExpectEmit1(inner) => expect_emit( + state, + Some(inner.0), + data.journaled_state.depth(), + [true, true, true, true], + ), + HEVMCalls::ExpectEmit2(inner) => expect_emit( + state, + None, + data.journaled_state.depth(), + [inner.0, inner.1, inner.2, inner.3], + ), + HEVMCalls::ExpectEmit3(inner) => expect_emit( + state, + Some(inner.4), + data.journaled_state.depth(), + [inner.0, inner.1, inner.2, inner.3], + ), HEVMCalls::ExpectCall0(inner) => expect_call( state, inner.0, diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index f305219d8df0..f96494122c29 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -770,6 +770,15 @@ where // Handle expected reverts if let Some(expected_revert) = &self.expected_revert { + // Irrespective of whether a revert will be matched or not, disallow having expected reverts + // alongside expected emits or calls. + if !self.expected_calls.is_empty() || !self.expected_emits.is_empty() { + return ( + InstructionResult::Revert, + remaining_gas, + "Cannot expect a function to revert while trying to match expected calls or events.".to_string().encode().into(), + ) + } if data.journaled_state.depth() == expected_revert.depth { let expected_revert = std::mem::take(&mut self.expected_revert).unwrap(); return match handle_expect_revert( From b6aadac31c27c9c4b3acc672c0dbc00635c58f01 Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Mon, 12 Jun 2023 12:57:39 -0400 Subject: [PATCH 2/3] chore: add tests --- testdata/cheats/ExpectCall.t.sol | 8 ++++++++ testdata/cheats/ExpectEmit.t.sol | 9 +++++++++ 2 files changed, 17 insertions(+) diff --git a/testdata/cheats/ExpectCall.t.sol b/testdata/cheats/ExpectCall.t.sol index fef0dd4aafda..b480968a41a7 100644 --- a/testdata/cheats/ExpectCall.t.sol +++ b/testdata/cheats/ExpectCall.t.sol @@ -254,6 +254,14 @@ contract ExpectCallTest is DSTest { cheats.expectCallMinGas(address(inner), 0, 50_001, abi.encodeWithSelector(inner.add.selector, 1, 1)); this.exposed_addHardGasLimit(target); } + + /// Ensure that you cannot use expectCall with an expectRevert. + function testFailExpectCallWithRevertDisallowed() public { + Contract target = new Contract(); + cheats.expectRevert(); + cheats.expectCall(address(target), abi.encodeWithSelector(target.add.selector)); + this.exposed_callTargetNTimes(target, 5, 5, 1); + } } contract ExpectCallCountTest is DSTest { diff --git a/testdata/cheats/ExpectEmit.t.sol b/testdata/cheats/ExpectEmit.t.sol index b9c6d54998e5..78c5ade0ebfc 100644 --- a/testdata/cheats/ExpectEmit.t.sol +++ b/testdata/cheats/ExpectEmit.t.sol @@ -569,6 +569,15 @@ contract ExpectEmitTest is DSTest { emitter.emitWindowAndOnTest(this); } + /// We should not be able to expect emits if we're expecting the function reverts, no matter + /// if the function reverts or not. + function testFailEmitWindowWithRevertDisallowed() public { + cheats.expectRevert(); + cheats.expectEmit(true, false, false, true); + emit A(1); + emitter.emitWindow(); + } + /// This test will fail if we check that all expected logs were emitted /// after every call from the same depth as the call that invoked the cheatcode. /// From 103ff281eddea014fe5b9ae25203396b8248b2ed Mon Sep 17 00:00:00 2001 From: Enrique Ortiz Date: Mon, 12 Jun 2023 12:57:52 -0400 Subject: [PATCH 3/3] chore: fmt --- evm/src/executor/inspector/cheatcodes/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/evm/src/executor/inspector/cheatcodes/mod.rs b/evm/src/executor/inspector/cheatcodes/mod.rs index f96494122c29..8aa453c89be2 100644 --- a/evm/src/executor/inspector/cheatcodes/mod.rs +++ b/evm/src/executor/inspector/cheatcodes/mod.rs @@ -770,8 +770,8 @@ where // Handle expected reverts if let Some(expected_revert) = &self.expected_revert { - // Irrespective of whether a revert will be matched or not, disallow having expected reverts - // alongside expected emits or calls. + // Irrespective of whether a revert will be matched or not, disallow having expected + // reverts alongside expected emits or calls. if !self.expected_calls.is_empty() || !self.expected_emits.is_empty() { return ( InstructionResult::Revert,