Update tests

John's nits pt. 3

packages/contracts-bedrock/.gas-snapshot

@@ -264,12 +264,12 @@ OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutp
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 207520)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 41753)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 199464)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 206358)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 206360)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 180229)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 244375)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245526)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 244377)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245528)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_paused_reverts() (gas: 53555)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 234939)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 234941)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163864)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 197042)
 OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 85690)
@@ -403,8 +403,8 @@ ResourceMetering_Test:test_meter_updateTenEmptyBlocks_succeeds() (gas: 21161)
 ResourceMetering_Test:test_meter_updateTwoEmptyBlocks_succeeds() (gas: 21117)
 ResourceMetering_Test:test_meter_useMax_succeeds() (gas: 8017416)
 ResourceMetering_Test:test_meter_useMoreThanMax_reverts() (gas: 16045)
-SafeCall_call_Test:test_callWithMinGas_noLeakageHigh_succeeds() (gas: 388610034)
-SafeCall_call_Test:test_callWithMinGas_noLeakageLow_succeeds() (gas: 142093281)
+SafeCall_call_Test:test_callWithMinGas_noLeakageHigh_succeeds() (gas: 2075873614)
+SafeCall_call_Test:test_callWithMinGas_noLeakageLow_succeeds() (gas: 753665282)
 Semver_Test:test_behindProxy_succeeds() (gas: 506748)
 Semver_Test:test_version_succeeds() (gas: 9418)
 SequencerFeeVault_Test:test_constructor_succeeds() (gas: 5526)

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

@@ -361,12 +361,15 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         // Set the l2Sender so contracts know who triggered this withdrawal on L2.
         l2Sender = _tx.sender;
 
-        // Trigger the call to the target contract. We use SafeCall because we don't
-        // care about the returndata and we don't want target contracts to be able to force this
-        // call to run out of gas via a returndata bomb. We also want to maintain the property
-        // that the amount of gas supplied to the call to the target contract is at least the gas
-        // limit specified by the user. If there is not enough gas in the callframe to accomplish
-        // this, `callWithMinGas` will revert.
+        // Trigger the call to the target contract. We use a custom low level method
+        // SafeCall.callWithMinGas to ensure two key properties
+        //   1. Target contracts cannot force this call to run out of gas by returning a very large
+        //      amount of data (and this is OK because we don't care about the returndata here).
+        //   2. The amount of gas provided to the call to the target contract is at least the gas
+        //      limit specified by the user. If there is not enough gas in the callframe to
+        //      accomplish this, `callWithMinGas` will revert.
+        // Additionally, if there is not enough gas remaining to complete the execution after the
+        // call returns, this function will revert.
         bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);
 
         // Reset the l2Sender back to the default value.

packages/contracts-bedrock/contracts/libraries/SafeCall.sol

@@ -26,7 +26,7 @@ library SafeCall {
                 _gas, // gas
                 _target, // recipient
                 _value, // ether value
-                add(_calldata, 0x20), // inloc
+                add(_calldata, 32), // inloc
                 mload(_calldata), // inlen
                 0, // outloc
                 0 // outlen
@@ -55,17 +55,17 @@ library SafeCall {
         assembly {
             // Assertion: gasleft() >= ((_minGas + 200) * 64) / 63
             //
-            // Because EIP-150 ensures that, at max, 64/64ths of the remaining gas in the call
+            // Because EIP-150 ensures that, a maximum of 63/64ths of the remaining gas in the call
             // frame may be passed to a subcontext, we need to ensure that the gas will not be
             // truncated to hold this function's invariant: "If a call is performed by
             // `callWithMinGas`, it must receive at least the specified minimum gas limit." In
             // addition, exactly 51 gas is consumed between the below `GAS` opcode and the `CALL`
             // opcode, so it is factored in with some extra room for error.
-            if lt(gas(), div(shl(0x06, add(_minGas, 0xC8)), 0x3F)) {
+            if lt(gas(), div(mul(64, add(_minGas, 200)), 63)) {
                 // Store the "Error(string)" selector in scratch space.
-                mstore(0x00, 0x08c379a0)
+                mstore(0, 0x08c379a0)
                 // Store the pointer to the string length in scratch space.
-                mstore(0x20, 0x20)
+                mstore(32, 32)
                 // Store the string.
                 //
                 // SAFETY:
@@ -76,10 +76,10 @@ library SafeCall {
                 // unlikely that it will be greater than 3 bytes. As for the data within
                 // 0x60, it is ensured that it is 0 due to 0x60 being the zero offset.
                 // - It's fine to clobber the free memory pointer, we're reverting.
-                mstore(0x58, 0x0000185361666543616c6c3a204e6f7420656e6f75676820676173)
+                mstore(88, 0x0000185361666543616c6c3a204e6f7420656e6f75676820676173)
 
                 // Revert with 'Error("SafeCall: Not enough gas")'
-                revert(0x1c, 0x64)
+                revert(28, 100)
             }
 
             // The call will be supplied at least (((_minGas + 200) * 64) / 63) - 49 gas due to the
@@ -93,7 +93,7 @@ library SafeCall {
                 gas(), // gas
                 _target, // recipient
                 _value, // ether value
-                add(_calldata, 0x20), // inloc
+                add(_calldata, 32), // inloc
                 mload(_calldata), // inlen
                 0x00, // outloc
                 0x00 // outlen

packages/contracts-bedrock/contracts/test/SafeCall.t.sol

@@ -14,8 +14,8 @@ contract SafeCall_call_Test is CommonTest {
     ) external {
         vm.assume(from.balance == 0);
         vm.assume(to.balance == 0);
-        // no precompiles
-        vm.assume(uint160(to) > 10);
+        // no precompiles (mainnet)
+        assumeNoPrecompiles(to, 1);
         // don't call the vm
         vm.assume(to != address(vm));
         vm.assume(from != address(vm));
@@ -23,21 +23,24 @@ contract SafeCall_call_Test is CommonTest {
         vm.assume(to != address(0x000000000000000000636F6e736F6c652e6c6f67));
         // don't call the create2 deployer
         vm.assume(to != address(0x4e59b44847b379578588920cA78FbF26c0B4956C));
-        // don't send funds to self
-        vm.assume(from != to);
 
         assertEq(from.balance, 0, "from balance is 0");
         vm.deal(from, value);
         assertEq(from.balance, value, "from balance not dealt");
 
-        vm.expectCall(to, value, data);
+        uint256[2] memory balancesBefore = [from.balance, to.balance];
 
+        vm.expectCall(to, value, data);
         vm.prank(from);
         bool success = SafeCall.call(to, gas, value, data);
 
-        assertEq(success, true, "call not successful");
-        assertEq(to.balance, value, "to balance received");
-        assertEq(from.balance, 0, "from balance not drained");
+        assertTrue(success, "call not successful");
+        if (from == to) {
+            assertEq(from.balance, balancesBefore[0], "Self-send did not change balance");
+        } else {
+            assertEq(from.balance, balancesBefore[0] - value, "from balance not drained");
+            assertEq(to.balance, balancesBefore[1] + value, "to balance received");
+        }
     }
 
     function testFuzz_callWithMinGas_hasEnough_succeeds(
@@ -49,8 +52,8 @@ contract SafeCall_call_Test is CommonTest {
     ) external {
         vm.assume(from.balance == 0);
         vm.assume(to.balance == 0);
-        // no precompiles
-        vm.assume(uint160(to) > 10);
+        // no precompiles (mainnet)
+        assumeNoPrecompiles(to, 1);
         // don't call the vm
         vm.assume(to != address(vm));
         vm.assume(from != address(vm));
@@ -58,8 +61,6 @@ contract SafeCall_call_Test is CommonTest {
         vm.assume(to != address(0x000000000000000000636F6e736F6c652e6c6f67));
         // don't call the create2 deployer
         vm.assume(to != address(0x4e59b44847b379578588920cA78FbF26c0B4956C));
-        // don't send funds to self
-        vm.assume(from != to);
 
         assertEq(from.balance, 0, "from balance is 0");
         vm.deal(from, value);
@@ -68,43 +69,55 @@ contract SafeCall_call_Test is CommonTest {
         // Bound minGas to [0, l1_block_gas_limit]
         minGas = uint64(bound(minGas, 0, 30_000_000));
 
+        uint256[2] memory balancesBefore = [from.balance, to.balance];
+
         vm.expectCallMinGas(to, value, minGas, data);
         vm.prank(from);
         bool success = SafeCall.callWithMinGas(to, minGas, value, data);
 
-        assertEq(success, true, "call not successful");
-        assertEq(to.balance, value, "to balance received");
-        assertEq(from.balance, 0, "from balance not drained");
+        assertTrue(success, "call not successful");
+        if (from == to) {
+            assertEq(from.balance, balancesBefore[0], "Self-send did not change balance");
+        } else {
+            assertEq(from.balance, balancesBefore[0] - value, "from balance not drained");
+            assertEq(to.balance, balancesBefore[1] + value, "to balance received");
+        }
     }
 
     function test_callWithMinGas_noLeakageLow_succeeds() external {
         SimpleSafeCaller caller = new SimpleSafeCaller();
 
-        for (uint64 i = 5000; i < 50000; i++) {
-            // 26,069 is the exact amount of gas required to make the safe call
+        for (uint64 i = 5000; i < 50_000; i++) {
+            uint256 snapshot = vm.snapshot();
+
+            // 26,071 is the exact amount of gas required to make the safe call
             // successfully.
-            if (i < 26069) {
-                assertFalse(caller.makeSafeCall(i, 25000));
+            if (i < 26_071) {
+                assertFalse(caller.makeSafeCall(i, 25_000));
             } else {
                 vm.expectCallMinGas(
                     address(caller),
                     0,
-                    25000,
+                    25_000,
                     abi.encodeWithSelector(caller.setA.selector, 1)
                 );
-                assertTrue(caller.makeSafeCall(i, 25000));
+                assertTrue(caller.makeSafeCall(i, 25_000));
             }
+
+            assertTrue(vm.revertTo(snapshot));
         }
     }
 
     function test_callWithMinGas_noLeakageHigh_succeeds() external {
         SimpleSafeCaller caller = new SimpleSafeCaller();
 
-        for (uint256 i = 15_200_000; i < 15_300_000; i++) {
-            // 15,238,767 is the exact amount of gas required to make the safe call
+        for (uint64 i = 15_200_000; i < 15_300_000; i++) {
+            uint256 snapshot = vm.snapshot();
+
+            // 15,238,769 is the exact amount of gas required to make the safe call
             // successfully.
-            if (i < 15_238_767) {
-                assertFalse(caller.makeSafeCall(uint64(i), 15_000_000));
+            if (i < 15_238_769) {
+                assertFalse(caller.makeSafeCall(i, 15_000_000));
             } else {
                 vm.expectCallMinGas(
                     address(caller),
@@ -112,8 +125,10 @@ contract SafeCall_call_Test is CommonTest {
                     15_000_000,
                     abi.encodeWithSelector(caller.setA.selector, 1)
                 );
-                assertTrue(caller.makeSafeCall(uint64(i), 15_000_000));
+                assertTrue(caller.makeSafeCall(i, 15_000_000));
             }
+
+            assertTrue(vm.revertTo(snapshot));
         }
     }
 }

packages/contracts-bedrock/contracts/test/invariants/SafeCall.t.sol

@@ -7,11 +7,11 @@ import { Vm } from "forge-std/Vm.sol";
 import { SafeCall } from "../../libraries/SafeCall.sol";
 
 contract SafeCall_Succeeds_Invariants is Test {
-    SafeCaller_Succeeds_Actor actor;
+    SafeCaller_Actor actor;
 
     function setUp() public {
         // Create a new safe caller actor.
-        actor = new SafeCaller_Succeeds_Actor(vm);
+        actor = new SafeCaller_Actor(vm, false);
 
         // Set the caller to this contract
         targetSender(address(this));
@@ -28,7 +28,7 @@ contract SafeCall_Succeeds_Invariants is Test {
      * subcontext of the call below it must be provided at least `minGas` gas.
      */
     function invariant_callWithMinGas_alwaysForwardsMinGas_succeeds() public {
-        assertEq(actor.numFailed(), 0, "no failed calls allowed");
+        assertEq(actor.numCalls(), 0, "no failed calls allowed");
     }
 
     function performSafeCallMinGas(uint64 minGas) external {
@@ -37,11 +37,11 @@ contract SafeCall_Succeeds_Invariants is Test {
 }
 
 contract SafeCall_Fails_Invariants is Test {
-    SafeCaller_Fails_Actor actor;
+    SafeCaller_Actor actor;
 
     function setUp() public {
         // Create a new safe caller actor.
-        actor = new SafeCaller_Fails_Actor(vm);
+        actor = new SafeCaller_Actor(vm, true);
 
         // Set the caller to this contract
         targetSender(address(this));
@@ -59,7 +59,7 @@ contract SafeCall_Fails_Invariants is Test {
      * then `callWithMinGas` must revert.
      */
     function invariant_callWithMinGas_neverForwardsMinGas_reverts() public {
-        assertEq(actor.numSuccessful(), 0, "no successful calls allowed");
+        assertEq(actor.numCalls(), 0, "no successful calls allowed");
     }
 
     function performSafeCallMinGas(uint64 minGas) external {
@@ -67,57 +67,39 @@ contract SafeCall_Fails_Invariants is Test {
     }
 }
 
-contract SafeCaller_Succeeds_Actor is StdUtils {
-    Vm internal vm;
-    uint256 public numFailed;
-
-    constructor(Vm _vm) {
-        vm = _vm;
-    }
+contract SafeCaller_Actor is StdUtils {
+    bool internal immutable FAILS;
 
-    function performSafeCallMinGas(uint64 gas, uint64 minGas) external {
-        // Bound the minimum gas amount to [2500, type(uint48).max]
-        minGas = uint64(bound(minGas, 2500, type(uint48).max));
-        // Bound the gas passed to [(((minGas + 200) * 64) / 63) + 500, type(uint64).max]
-        gas = uint64(bound(gas, (((minGas + 200) * 64) / 63) + 500, type(uint64).max));
-
-        vm.expectCallMinGas(address(0x00), 0, minGas, hex"");
-        bool success = SafeCall.call(
-            msg.sender,
-            gas,
-            0,
-            abi.encodeWithSelector(
-                SafeCall_Succeeds_Invariants.performSafeCallMinGas.selector,
-                minGas
-            )
-        );
-
-        if (!success) numFailed++;
-    }
-}
-
-contract SafeCaller_Fails_Actor is StdUtils {
     Vm internal vm;
-    uint256 public numSuccessful;
+    uint256 public numCalls;
 
-    constructor(Vm _vm) {
+    constructor(Vm _vm, bool _fails) {
         vm = _vm;
+        FAILS = _fails;
     }
 
     function performSafeCallMinGas(uint64 gas, uint64 minGas) external {
+        if (FAILS) {
             // Bound the minimum gas amount to [2500, type(uint48).max]
             minGas = uint64(bound(minGas, 2500, type(uint48).max));
             // Bound the gas passed to [minGas, (((minGas + 200) * 64) / 63)]
             gas = uint64(bound(gas, minGas, (((minGas + 200) * 64) / 63)));
+        } else {
+            // Bound the minimum gas amount to [2500, type(uint48).max]
+            minGas = uint64(bound(minGas, 2500, type(uint48).max));
+            // Bound the gas passed to [(((minGas + 200) * 64) / 63) + 500, type(uint64).max]
+            gas = uint64(bound(gas, (((minGas + 200) * 64) / 63) + 500, type(uint64).max));
+        }
 
         vm.expectCallMinGas(address(0x00), 0, minGas, hex"");
         bool success = SafeCall.call(
             msg.sender,
             gas,
             0,
-            abi.encodeWithSelector(SafeCall_Fails_Invariants.performSafeCallMinGas.selector, minGas)
+            abi.encodeWithSelector(0x2ae57a41, minGas)
         );
 
-        if (success) numSuccessful++;
+        if (success && FAILS) numCalls++;
+        if (!FAILS && !success) numCalls++;
     }
 }