fix(cheatcodes): Correct expectRevert behavior (#4945)

* chore: add repro test to pass

* chore: strictly check for the depth expectRevert was called in, instead of being able to peek at function end

* chore: tests

* chore: add more repro tests

* chore: fmt

* chore: clippy

* chore: fixup problematic tests, mark them as not working properly

* chore: forge fmt

* chore: forge fmt

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

* chore: add more info to changelog

* chore: fmt

CHANGELOG.md

@@ -27,8 +27,9 @@ To use the latest pinned nightly on your CI, modify your Foundry installation st
 
 - [expectEmit](https://github.com/foundry-rs/foundry/pull/4920) will now only work for the next call.
 - expectCall will now only work if the call(s) are made exactly after the cheatcode is invoked.
+- [expectRevert will now work if the next call does revert](https://github.com/foundry-rs/foundry/pull/4945), instead of expecting a revert during the whole test.
+  - This will very likely break your tests. Please make sure that all the calls you expect to revert are external, and if not, abstract them into a separate contract so that they can be called externally and the cheatcode can be used.
 - `-m`, the deprecated alias for `--mt` or `--match-test`, has now been removed.
-- expectRevert will now work if the next call does revert, instead of expecting a revert during the whole test.
 - [startPrank will now override the existing prank instead of erroring](https://github.com/foundry-rs/foundry/pull/4826).
 - [precompiles will not be compatible with all cheatcodes](https://github.com/foundry-rs/foundry/pull/4905).
 - The difficulty and prevrandao cheatcodes now [fail if not used with the correct EVM version](https://github.com/foundry-rs/foundry/pull/4904).

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

@@ -770,7 +770,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,
@@ -895,6 +895,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,
+                    "A `vm.expectRevert`was left dangling. Make sure that calls you expect to revert are external".encode().into(),
+                )
+            }
+
             // Check if we have any leftover expected emits
             // First, if any emits were found at the root call, then we its ok and we remove them.
             self.expected_emits.retain(|expected| !expected.found);
@@ -1052,7 +1061,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");
+    // }
 }