Merge pull request #5470 from ethereum-optimism/clabby/ctb/call-with-min-gas-fix

feat(ctb): Account for worst-case `CALL` gas in `SafeCall`

packages/contracts-bedrock/.gas-snapshot

@@ -7,14 +7,14 @@ Bytes_toNibbles_Test:test_toNibbles_expectedResult5Bytes_works() (gas: 6132)
 Bytes_toNibbles_Test:test_toNibbles_zeroLengthInput_works() (gas: 944)
 CrossDomainMessenger_BaseGas_Test:test_baseGas_succeeds() (gas: 20097)
 CrossDomainOwnable2_Test:test_onlyOwner_notMessenger_reverts() (gas: 8416)
-CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 57254)
+CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 57323)
 CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16566)
-CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 73282)
+CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 73351)
 CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10554)
 CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notMessenger_reverts() (gas: 28334)
-CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 73730)
+CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 73799)
 CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner_reverts() (gas: 31978)
-CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 91287)
+CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 91356)
 CrossDomainOwnable3_Test:test_localOnlyOwner_notOwner_reverts() (gas: 13193)
 CrossDomainOwnable3_Test:test_localOnlyOwner_succeeds() (gas: 35220)
 CrossDomainOwnable3_Test:test_localTransferOwnership_succeeds() (gas: 52128)
@@ -70,18 +70,18 @@ L1BlockTest:test_timestamp_succeeds() (gas: 7640)
 L1BlockTest:test_updateValues_succeeds() (gas: 60482)
 L1CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 24715)
 L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 49394)
-L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 209286)
-L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203184)
-L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 123784)
-L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77098)
-L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197091)
-L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 74034)
+L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 209424)
+L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203322)
+L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 123853)
+L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77167)
+L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197229)
+L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 74103)
 L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 56540)
 L1CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 12365)
 L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 31063)
 L1CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 304740)
 L1CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 1496124)
-L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84563)
+L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84632)
 L1CrossDomainMessenger_Test:test_xDomainSender_notSet_reverts() (gas: 24296)
 L1ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 52707)
 L1ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 27310)
@@ -118,13 +118,13 @@ L1StandardBridge_Getter_Test:test_getters_succeeds() (gas: 32173)
 L1StandardBridge_Initialize_Test:test_initialize_succeeds() (gas: 22050)
 L1StandardBridge_Receive_Test:test_receive_succeeds() (gas: 525438)
 L2CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 8411)
-L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 163159)
-L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 48640)
+L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 163297)
+L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 48709)
 L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 29021)
 L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 11711)
 L2CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 122508)
 L2CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 134826)
-L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 48139)
+L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 48208)
 L2CrossDomainMessenger_Test:test_xDomainSender_senderNotSet_reverts() (gas: 10590)
 L2ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 26431)
 L2ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 21814)
@@ -264,13 +264,13 @@ OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutp
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 207497)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 41753)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 199441)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 205795)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 205862)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 180206)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 243812)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245528)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 243881)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245597)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_paused_reverts() (gas: 53576)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 234941)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163864)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 235010)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163867)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 197019)
 OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 85690)
 OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidWithdrawalProof_reverts() (gas: 137350)
@@ -404,8 +404,8 @@ ResourceMetering_Test:test_meter_updateTenEmptyBlocks_succeeds() (gas: 23747)
 ResourceMetering_Test:test_meter_updateTwoEmptyBlocks_succeeds() (gas: 23703)
 ResourceMetering_Test:test_meter_useMax_succeeds() (gas: 20020816)
 ResourceMetering_Test:test_meter_useMoreThanMax_reverts() (gas: 19549)
-SafeCall_call_Test:test_callWithMinGas_noLeakageHigh_succeeds() (gas: 2075873614)
-SafeCall_call_Test:test_callWithMinGas_noLeakageLow_succeeds() (gas: 753665282)
+SafeCall_Test:test_callWithMinGas_noLeakageHigh_succeeds() (gas: 1020974534)
+SafeCall_Test:test_callWithMinGas_noLeakageLow_succeeds() (gas: 1094812728)
 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

@@ -140,7 +140,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
     }
 
     /**
-     * @custom:semver 1.3.1
+     * @custom:semver 1.4.0
      *
      * @param _l2Oracle                  Address of the L2OutputOracle contract.
      * @param _guardian                  Address that can pause deposits and withdrawals.
@@ -152,7 +152,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         address _guardian,
         bool _paused,
         SystemConfig _config
-    ) Semver(1, 3, 1) {
+    ) Semver(1, 4, 0) {
         L2_ORACLE = _l2Oracle;
         GUARDIAN = _guardian;
         SYSTEM_CONFIG = _config;
@@ -388,11 +388,9 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         // 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.
+        //   2. The amount of gas provided to the execution context of the target is at least the
+        //      gas limit specified by the user. If there is not enough gas in the current context
+        //      to accomplish this, `callWithMinGas` 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

@@ -35,6 +35,41 @@ library SafeCall {
         return _success;
     }
 
+    /**
+     * @notice Helper function to determine if there is sufficient gas remaining within the context
+     *         to guarantee that the minimum gas requirement for a call will be met as well as
+     *         optionally reserving a specified amount of gas for after the call has concluded.
+     * @param _minGas      The minimum amount of gas that may be passed to the target context.
+     * @param _reservedGas Optional amount of gas to reserve for the caller after the execution
+     *                     of the target context.
+     * @return `true` if there is enough gas remaining to safely supply `_minGas` to the target
+     *         context as well as reserve `_reservedGas` for the caller after the execution of
+     *         the target context.
+     * @dev !!!!! FOOTGUN ALERT !!!!!
+     *      1.) The 40_000 base buffer is to account for the worst case of the dynamic cost of the
+     *          `CALL` opcode's `address_access_cost`, `positive_value_cost`, and
+     *          `value_to_empty_account_cost` factors with an added buffer of 5,700 gas. It is
+     *          still possible to self-rekt by initiating a withdrawal with a minimum gas limit
+     *          that does not account for the `memory_expansion_cost` & `code_execution_cost`
+     *          factors of the dynamic cost of the `CALL` opcode.
+     *      2.) This function should *directly* precede the external call if possible. There is an
+     *          added buffer to account for gas consumed between this check and the call, but it
+     *          is only 5,700 gas.
+     *      3.) 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.
+     *      4.) Use wisely. This function is not a silver bullet.
+     */
+    function hasMinGas(uint256 _minGas, uint256 _reservedGas) internal view returns (bool) {
+        bool _hasMinGas;
+        assembly {
+            _hasMinGas := iszero(
+                lt(gas(), add(div(mul(_minGas, 64), 63), add(40000, _reservedGas)))
+            )
+        }
+        return _hasMinGas;
+    }
+
     /**
      * @notice Perform a low level call without copying any returndata. This function
      *         will revert if the call cannot be performed with the specified minimum
@@ -52,16 +87,10 @@ library SafeCall {
         bytes memory _calldata
     ) internal returns (bool) {
         bool _success;
+        bool _hasMinGas = hasMinGas(_minGas, 0);
         assembly {
-            // Assertion: gasleft() >= ((_minGas + 200) * 64) / 63
-            //
-            // 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(mul(64, add(_minGas, 200)), 63)) {
+            // Assertion: gasleft() >= (_minGas * 64) / 63 + 40_000
+            if iszero(_hasMinGas) {
                 // Store the "Error(string)" selector in scratch space.
                 mstore(0, 0x08c379a0)
                 // Store the pointer to the string length in scratch space.
@@ -82,13 +111,11 @@ library SafeCall {
                 revert(28, 100)
             }
 
-            // The call will be supplied at least (((_minGas + 200) * 64) / 63) - 49 gas due to the
-            // above assertion. This ensures that, in all circumstances, the call will
-            // receive at least the minimum amount of gas specified.
-            // We can prove this property by solving the inequalities:
-            // ((((_minGas + 200) * 64) / 63) - 49) >= _minGas
-            // ((((_minGas + 200) * 64) / 63) - 51) * (63 / 64) >= _minGas
-            // Both inequalities hold true for all possible values of `_minGas`.
+            // The call will be supplied at least ((_minGas * 64) / 63) gas due to the
+            // above assertion. This ensures that, in all circumstances (except for when the
+            // `_minGas` does not account for the `memory_expansion_cost` and `code_execution_cost`
+            // factors of the dynamic cost of the `CALL` opcode), the call will receive at least
+            // the minimum amount of gas specified.
             _success := call(
                 gas(), // gas
                 _target, // recipient

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

@@ -4,7 +4,7 @@ pragma solidity 0.8.15;
 import { CommonTest } from "./CommonTest.t.sol";
 import { SafeCall } from "../libraries/SafeCall.sol";
 
-contract SafeCall_call_Test is CommonTest {
+contract SafeCall_Test is CommonTest {
     function testFuzz_call_succeeds(
         address from,
         address to,
@@ -63,6 +63,8 @@ contract SafeCall_call_Test is CommonTest {
         vm.assume(to != address(0x000000000000000000636F6e736F6c652e6c6f67));
         // don't call the create2 deployer
         vm.assume(to != address(0x4e59b44847b379578588920cA78FbF26c0B4956C));
+        // don't call the FFIInterface
+        vm.assume(to != address(0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f));
 
         assertEq(from.balance, 0, "from balance is 0");
         vm.deal(from, value);
@@ -89,12 +91,12 @@ contract SafeCall_call_Test is CommonTest {
     function test_callWithMinGas_noLeakageLow_succeeds() external {
         SimpleSafeCaller caller = new SimpleSafeCaller();
 
-        for (uint64 i = 5000; i < 50_000; i++) {
+        for (uint64 i = 40_000; i < 100_000; i++) {
             uint256 snapshot = vm.snapshot();
 
-            // 26,071 is the exact amount of gas required to make the safe call
+            // 65_903 is the exact amount of gas required to make the safe call
             // successfully.
-            if (i < 26_071) {
+            if (i < 65_903) {
                 assertFalse(caller.makeSafeCall(i, 25_000));
             } else {
                 vm.expectCallMinGas(
@@ -116,9 +118,9 @@ contract SafeCall_call_Test is CommonTest {
         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
+            // 15_278_602 is the exact amount of gas required to make the safe call
             // successfully.
-            if (i < 15_238_769) {
+            if (i < 15_278_602) {
                 assertFalse(caller.makeSafeCall(i, 15_000_000));
             } else {
                 vm.expectCallMinGas(

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

@@ -18,6 +18,9 @@ contract SafeCall_Succeeds_Invariants is Test {
 
         // Target the safe caller actor.
         targetContract(address(actor));
+
+        // Give the actor some ETH to work with
+        vm.deal(address(actor), type(uint128).max);
     }
 
     /**
@@ -31,8 +34,8 @@ contract SafeCall_Succeeds_Invariants is Test {
         assertEq(actor.numCalls(), 0, "no failed calls allowed");
     }
 
-    function performSafeCallMinGas(uint64 minGas) external {
-        SafeCall.callWithMinGas(address(0), minGas, 0, hex"");
+    function performSafeCallMinGas(address to, uint64 minGas) external payable {
+        SafeCall.callWithMinGas(to, minGas, msg.value, hex"");
     }
 }
 
@@ -48,6 +51,9 @@ contract SafeCall_Fails_Invariants is Test {
 
         // Target the safe caller actor.
         targetContract(address(actor));
+
+        // Give the actor some ETH to work with
+        vm.deal(address(actor), type(uint128).max);
     }
 
     /**
@@ -62,8 +68,8 @@ contract SafeCall_Fails_Invariants is Test {
         assertEq(actor.numCalls(), 0, "no successful calls allowed");
     }
 
-    function performSafeCallMinGas(uint64 minGas) external {
-        SafeCall.callWithMinGas(address(0), minGas, 0, hex"");
+    function performSafeCallMinGas(address to, uint64 minGas) external payable {
+        SafeCall.callWithMinGas(to, minGas, msg.value, hex"");
     }
 }
 
@@ -78,25 +84,39 @@ contract SafeCaller_Actor is StdUtils {
         FAILS = _fails;
     }
 
-    function performSafeCallMinGas(uint64 gas, uint64 minGas) external {
-        if (FAILS) {
+    function performSafeCallMinGas(
+        uint64 gas,
+        uint64 minGas,
+        address to,
+        uint8 value
+    ) external {
+        // Only send to EOAs - we exclude the console as it has no code but reverts when called
+        // with a selector that doesn't exist due to the foundry hook.
+        vm.assume(to.code.length == 0 && to != 0x000000000000000000636F6e736F6c652e6c6f67);
+
         // 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)));
+        if (FAILS) {
+            // Bound the gas passed to [minGas, ((minGas * 64) / 63)]
+            gas = uint64(bound(gas, minGas, (minGas * 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));
+            // Bound the gas passed to
+            // [((minGas * 64) / 63) + 40_000 + 1000, type(uint64).max]
+            // The extra 1000 gas is to account for the gas used by the `SafeCall.call` call
+            // itself.
+            gas = uint64(bound(gas, ((minGas * 64) / 63) + 40_000 + 1000, type(uint64).max));
         }
 
-        vm.expectCallMinGas(address(0x00), 0, minGas, hex"");
+        vm.expectCallMinGas(to, value, minGas, hex"");
         bool success = SafeCall.call(
             msg.sender,
             gas,
-            0,
-            abi.encodeWithSelector(0x2ae57a41, minGas)
+            value,
+            abi.encodeWithSelector(
+                SafeCall_Succeeds_Invariants.performSafeCallMinGas.selector,
+                to,
+                minGas
+            )
         );
 
         if (success && FAILS) numCalls++;

packages/contracts-bedrock/invariant-docs/SafeCall.md

 # `SafeCall` Invariants
 
 ## If `callWithMinGas` performs a call, then it must always provide at least the specified minimum gas limit to the subcontext.
-**Test:** [`SafeCall.t.sol#L30`](../contracts/test/invariants/SafeCall.t.sol#L30)
+**Test:** [`SafeCall.t.sol#L33`](../contracts/test/invariants/SafeCall.t.sol#L33)
 
 If the check for remaining gas in `SafeCall.callWithMinGas` passes, the subcontext of the call below it must be provided at least `minGas` gas. 
 
 
 ## `callWithMinGas` reverts if there is not enough gas to pass to the subcontext.
-**Test:** [`SafeCall.t.sol#L61`](../contracts/test/invariants/SafeCall.t.sol#L61)
+**Test:** [`SafeCall.t.sol#L67`](../contracts/test/invariants/SafeCall.t.sol#L67)
 
 If there is not enough gas in the callframe to ensure that `callWithMinGas` can provide the specified minimum gas limit to the subcontext of the call, then `callWithMinGas` must revert.