Merge pull request #5017 from ethereum-optimism/clabby/ctb/safecall-min-gas

fix(ctb): `finalizeWithdrawalTransaction` gas limit dos (v2)

packages/contracts-bedrock/.gas-snapshot

@@ -264,13 +264,13 @@ 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: 203388)
+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: 244483)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245634)
+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: 235047)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163866)
+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)
 OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidWithdrawalProof_reverts() (gas: 137350)
@@ -403,6 +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: 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

@@ -43,11 +43,6 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
      */
     uint64 internal constant RECEIVE_DEFAULT_GAS_LIMIT = 100_000;
 
-    /**
-     * @notice Additional gas reserved for clean up after finalizing a transaction withdrawal.
-     */
-    uint256 internal constant FINALIZE_GAS_BUFFER = 20_000;
-
     /**
      * @notice Address of the L2OutputOracle.
      */
@@ -363,26 +358,19 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         // Mark the withdrawal as finalized so it can't be replayed.
         finalizedWithdrawals[withdrawalHash] = true;
 
-        // We 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. We can do this by
-        // enforcing that, at this point in time, we still have gaslimit + buffer gas available.
-        require(
-            gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER,
-            "OptimismPortal: insufficient gas to finalize withdrawal"
-        );
-
         // 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.
-        bool success = SafeCall.call(
-            _tx.target,
-            gasleft() - FINALIZE_GAS_BUFFER,
-            _tx.value,
-            _tx.data
-        );
+        // 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.
         l2Sender = Constants.DEFAULT_L2_SENDER;

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
@@ -34,4 +34,71 @@ library SafeCall {
         }
         return _success;
     }
+
+    /**
+     * @notice Perform a low level call without copying any returndata. This function
+     *         will revert if the call cannot be performed with the specified minimum
+     *         gas.
+     *
+     * @param _target   Address to call
+     * @param _minGas   The minimum amount of gas that may be passed to the call
+     * @param _value    Amount of value to pass to the call
+     * @param _calldata Calldata to pass to the call
+     */
+    function callWithMinGas(
+        address _target,
+        uint256 _minGas,
+        uint256 _value,
+        bytes memory _calldata
+    ) internal returns (bool) {
+        bool _success;
+        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)) {
+                // Store the "Error(string)" selector in scratch space.
+                mstore(0, 0x08c379a0)
+                // Store the pointer to the string length in scratch space.
+                mstore(32, 32)
+                // Store the string.
+                //
+                // SAFETY:
+                // - We pad the beginning of the string with two zero bytes as well as the
+                // length (24) to ensure that we override the free memory pointer at offset
+                // 0x40. This is necessary because the free memory pointer is likely to
+                // be greater than 1 byte when this function is called, but it is incredibly
+                // 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(88, 0x0000185361666543616c6c3a204e6f7420656e6f75676820676173)
+
+                // Revert with 'Error("SafeCall: Not enough gas")'
+                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`.
+            _success := call(
+                gas(), // gas
+                _target, // recipient
+                _value, // ether value
+                add(_calldata, 32), // inloc
+                mload(_calldata), // inlen
+                0x00, // outloc
+                0x00 // outlen
+            )
+        }
+        return _success;
+    }
 }

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

@@ -905,7 +905,7 @@ contract OptimismPortal_FinalizeWithdrawal_Test is Portal_Initializer {
         );
 
         vm.warp(block.timestamp + oracle.FINALIZATION_PERIOD_SECONDS() + 1);
-        vm.expectRevert("OptimismPortal: insufficient gas to finalize withdrawal");
+        vm.expectRevert("SafeCall: Not enough gas");
         op.finalizeWithdrawalTransaction{ gas: gasLimit }(insufficientGasTx);
     }
 

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,20 +23,140 @@ 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(
+        address from,
+        address to,
+        uint64 minGas,
+        uint64 value,
+        bytes memory data
+    ) external {
+        vm.assume(from.balance == 0);
+        vm.assume(to.balance == 0);
+        // no precompiles (mainnet)
+        assumeNoPrecompiles(to, 1);
+        // don't call the vm
+        vm.assume(to != address(vm));
+        vm.assume(from != address(vm));
+        // don't call the console
+        vm.assume(to != address(0x000000000000000000636F6e736F6c652e6c6f67));
+        // don't call the create2 deployer
+        vm.assume(to != address(0x4e59b44847b379578588920cA78FbF26c0B4956C));
+
+        assertEq(from.balance, 0, "from balance is 0");
+        vm.deal(from, value);
+        assertEq(from.balance, value, "from balance not dealt");
+
+        // 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);
+
+        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 < 50_000; i++) {
+            uint256 snapshot = vm.snapshot();
+
+            // 26,071 is the exact amount of gas required to make the safe call
+            // successfully.
+            if (i < 26_071) {
+                assertFalse(caller.makeSafeCall(i, 25_000));
+            } else {
+                vm.expectCallMinGas(
+                    address(caller),
+                    0,
+                    25_000,
+                    abi.encodeWithSelector(caller.setA.selector, 1)
+                );
+                assertTrue(caller.makeSafeCall(i, 25_000));
+            }
+
+            assertTrue(vm.revertTo(snapshot));
+        }
+    }
+
+    function test_callWithMinGas_noLeakageHigh_succeeds() external {
+        SimpleSafeCaller caller = new SimpleSafeCaller();
+
+        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_769) {
+                assertFalse(caller.makeSafeCall(i, 15_000_000));
+            } else {
+                vm.expectCallMinGas(
+                    address(caller),
+                    0,
+                    15_000_000,
+                    abi.encodeWithSelector(caller.setA.selector, 1)
+                );
+                assertTrue(caller.makeSafeCall(i, 15_000_000));
+            }
+
+            assertTrue(vm.revertTo(snapshot));
+        }
+    }
+}
+
+contract SimpleSafeCaller {
+    uint256 public a;
+
+    function makeSafeCall(uint64 gas, uint64 minGas) external returns (bool) {
+        return
+            SafeCall.call(
+                address(this),
+                gas,
+                0,
+                abi.encodeWithSelector(this.makeSafeCallMinGas.selector, minGas)
+            );
+    }
+
+    function makeSafeCallMinGas(uint64 minGas) external returns (bool) {
+        return
+            SafeCall.callWithMinGas(
+                address(this),
+                minGas,
+                0,
+                abi.encodeWithSelector(this.setA.selector, 1)
+            );
+    }
+
+    function setA(uint256 _a) external {
+        a = _a;
     }
 }

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

+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.15;
+
+import { Test } from "forge-std/Test.sol";
+import { StdUtils } from "forge-std/StdUtils.sol";
+import { Vm } from "forge-std/Vm.sol";
+import { SafeCall } from "../../libraries/SafeCall.sol";
+
+contract SafeCall_Succeeds_Invariants is Test {
+    SafeCaller_Actor actor;
+
+    function setUp() public {
+        // Create a new safe caller actor.
+        actor = new SafeCaller_Actor(vm, false);
+
+        // Set the caller to this contract
+        targetSender(address(this));
+
+        // Target the safe caller actor.
+        targetContract(address(actor));
+    }
+
+    /**
+     * @custom:invariant If `callWithMinGas` performs a call, then it must always
+     * provide at least the specified minimum gas limit to the subcontext.
+     *
+     * If the check for remaining gas in `SafeCall.callWithMinGas` passes, the
+     * subcontext of the call below it must be provided at least `minGas` gas.
+     */
+    function invariant_callWithMinGas_alwaysForwardsMinGas_succeeds() public {
+        assertEq(actor.numCalls(), 0, "no failed calls allowed");
+    }
+
+    function performSafeCallMinGas(uint64 minGas) external {
+        SafeCall.callWithMinGas(address(0), minGas, 0, hex"");
+    }
+}
+
+contract SafeCall_Fails_Invariants is Test {
+    SafeCaller_Actor actor;
+
+    function setUp() public {
+        // Create a new safe caller actor.
+        actor = new SafeCaller_Actor(vm, true);
+
+        // Set the caller to this contract
+        targetSender(address(this));
+
+        // Target the safe caller actor.
+        targetContract(address(actor));
+    }
+
+    /**
+     * @custom:invariant `callWithMinGas` reverts if there is not enough gas to pass
+     * to the subcontext.
+     *
+     * 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.
+     */
+    function invariant_callWithMinGas_neverForwardsMinGas_reverts() public {
+        assertEq(actor.numCalls(), 0, "no successful calls allowed");
+    }
+
+    function performSafeCallMinGas(uint64 minGas) external {
+        SafeCall.callWithMinGas(address(0), minGas, 0, hex"");
+    }
+}
+
+contract SafeCaller_Actor is StdUtils {
+    bool internal immutable FAILS;
+
+    Vm internal vm;
+    uint256 public numCalls;
+
+    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(0x2ae57a41, minGas)
+        );
+
+        if (success && FAILS) numCalls++;
+        if (!FAILS && !success) numCalls++;
+    }
+}

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

@@ -14,6 +14,7 @@ This directory contains documentation for all defined invariant tests within `co
 - [L2OutputOracle](./L2OutputOracle.md)
 - [OptimismPortal](./OptimismPortal.md)
 - [ResourceMetering](./ResourceMetering.md)
+- [SafeCall](./SafeCall.md)
 - [SystemConfig](./SystemConfig.md)
 <!-- END autoTOC -->
 

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)
+
+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)
+
+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.