fix(cheatcodes): Correct expectRevert behavior

evm/src/executor/inspector/cheatcodes/mod.rs

@@ -768,7 +768,7 @@ where
 
         // Handle expected reverts
         if let Some(expected_revert) = &self.expected_revert {
-            if data.journaled_state.depth() <= expected_revert.depth {
+            if data.journaled_state.depth() == expected_revert.depth {
                 let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
                 return match handle_expect_revert(
                     false,
@@ -821,6 +821,15 @@ where
 
         // If the depth is 0, then this is the root call terminating
         if data.journaled_state.depth() == 0 {
+            // See if there's a dangling expectRevert that should've been matched.
+            if self.expected_revert.is_some() {
+                return (
+                    InstructionResult::Revert,
+                    remaining_gas,
+                    "Expected revert was left dangling".encode().into(),
+                )
+            }
+
             // Match expected calls
             for (address, calldatas) in &self.expected_calls {
                 // Loop over each address, and for each address, loop over each calldata it expects.
@@ -1043,7 +1052,7 @@ where
 
         // Handle expected reverts
         if let Some(expected_revert) = &self.expected_revert {
-            if data.journaled_state.depth() <= expected_revert.depth {
+            if data.journaled_state.depth() == expected_revert.depth {
                 let expected_revert = std::mem::take(&mut self.expected_revert).unwrap();
                 return match handle_expect_revert(
                     true,

forge/tests/it/repros.rs

@@ -149,6 +149,12 @@ fn test_issue_3221() {
     test_repro!("Issue3221");
 }
 
+// <https://github.com/foundry-rs/foundry/issues/3437>
+#[test]
+fn test_issue_3437() {
+    test_repro!("Issue3437");
+}
+
 // <https://github.com/foundry-rs/foundry/issues/3708>
 #[test]
 fn test_issue_3708() {
@@ -238,6 +244,12 @@ fn test_issue_3703() {
     test_repro!("Issue3703");
 }
 
+// <https://github.com/foundry-rs/foundry/issues/3723>
+#[test]
+fn test_issue_3723() {
+    test_repro!("Issue3723");
+}
+
 // <https://github.com/foundry-rs/foundry/issues/3753>
 #[test]
 fn test_issue_3753() {
@@ -255,3 +267,9 @@ fn test_issue_4630() {
 fn test_issue_4586() {
     test_repro!("Issue4586");
 }
+
+// https://github.com/foundry-rs/foundry/issues/4832
+#[test]
+fn test_issue_4832() {
+    test_repro!("Issue4832");
+}

testdata/cheats/Etch.t.sol

@@ -14,12 +14,12 @@ contract EtchTest is DSTest {
         assertEq(string(code), string(target.code));
     }
 
-    function testEtchNotAvailableOnPrecompiles() public {
-        address target = address(1);
-        bytes memory code = hex"1010";
-        cheats.expectRevert(
-            bytes("Etch cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead")
-        );
-        cheats.etch(target, code);
-    }
+    // function testEtchNotAvailableOnPrecompiles() public {
+    //     address target = address(1);
+    //     bytes memory code = hex"1010";
+    //     cheats.expectRevert(
+    //         bytes("Etch cannot be used on precompile addresses (N < 10). Please use an address bigger than 10 instead")
+    //     );
+    //     cheats.etch(target, code);
+    // }
 }

testdata/cheats/ExpectRevert.t.sol

@@ -70,12 +70,31 @@ contract Dummy {
 contract ExpectRevertTest is DSTest {
     Cheats constant cheats = Cheats(HEVM_ADDRESS);
 
+    function shouldRevert() internal {
+        revert();
+    }
+
     function testExpectRevertString() public {
         Reverter reverter = new Reverter();
         cheats.expectRevert("revert");
         reverter.revertWithMessage("revert");
     }
 
+    function testFailRevertNotOnImmediateNextCall() public {
+        Reverter reverter = new Reverter();
+        // expectRevert should only work for the next call. However,
+        // we do not inmediately revert, so,
+        // we fail.
+        cheats.expectRevert("revert");
+        reverter.doNotRevert();
+        reverter.revertWithMessage("revert");
+    }
+
+    function testFailDanglingOnInternalCall() public {
+        cheats.expectRevert();
+        shouldRevert();
+    }
+
     function testExpectRevertConstructor() public {
         cheats.expectRevert("constructor revert");
         new ConstructorReverter("constructor revert");
@@ -167,10 +186,17 @@ contract ExpectRevertTest is DSTest {
         cheats.expectRevert("dangling");
     }
 
-    function testExpectRevertInvalidEnv() public {
-        cheats.expectRevert(
-            "Failed to get environment variable `_testExpectRevertInvalidEnv` as type `string`: environment variable not found"
-        );
-        string memory val = cheats.envString("_testExpectRevertInvalidEnv");
-    }
+    // This is now illegal behavior for expectRevert.
+    // This test would've previously passed as expectRevert
+    // would also check reverts at the test level, not only
+    // at the immediate next call.
+    // This allowed cheatcodes to be checked for reverts,
+    // which actually shouldn't have been possible as cheatcodes
+    // are supposed to be bypassed for all expect* checks.
+    // function testExpectRevertInvalidEnv() public {
+    //     cheats.expectRevert(
+    //         "Failed to get environment variable `_testExpectRevertInvalidEnv` as type `string`: environment variable not found"
+    //     );
+    //     string memory val = cheats.envString("_testExpectRevertInvalidEnv");
+    // }
 }