Ro - Users can lose bridged funds due to insufficient gas validation

[sherlock-admin]

Ro

medium

Users can lose bridged funds due to insufficient gas validation

Summary

The CrossDomainMessenger doesn't validate that a message has enough gas in order to be successful.

Vulnerability Detail

When bridging assets (both ways) the flow is as follows:

L1 to L2

L1 side: User ==> L1StandardBridge ==> L1CrossDomainMessenger ==> OptimismPortal
L2 side: sender ==> L2CrossDomainMessenger ==> L2StandardBridge ...

L2 to L1
L2 side: User ==> L2StandardBridge ==> L2CrossDomainMessenger ==> L2toL1MessagePasser
L1 side: Relayer or user ==> OptimismPortal ==> L1CrossDomainMessenger ==> L1StandardBridge ...

In critical steps along the way, there is always a gas validation mechanism (click here for a past sherlock issue):

When sending a cross-chain message, the first crucial gas validation comes from the "baseGas" function in the CrossDomainMessenger:

function baseGas(bytes calldata _message, uint32 _minGasLimit) public pure returns (uint64) {
        // We peform the following math on uint64s to avoid overflow errors. Multiplying the
        // by MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR would otherwise limit the _minGasLimit to
        // type(uint32).max / MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR ~= 4.2m.
        return
            // Dynamic overhead
            ((uint64(_minGasLimit) * MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR) /
                MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR) +
            // Calldata overhead
            (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD) +
            // Constant overhead
            MIN_GAS_CONSTANT_OVERHEAD;
    }

This ensures that the first message call cross chain will have enough gas.

The problem comes from not validating the _minGasLimit sent to the other cross chain relayMessage:

_sendMessage(
            OTHER_MESSENGER,
            baseGas(_message, _minGasLimit),
            msg.value,
            abi.encodeWithSelector(
                this.relayMessage.selector,
                messageNonce(),
                msg.sender,
                _target,
                msg.value,
                _minGasLimit,
                _message
            )
        );

The _minGasLimit is sent "as is" without a minimum validation (needs to be greater than 0 + call opcode + x..).

This parameter is even more important than the first gas limit, because in the OptimismPortal the first call will fail with the new callWithMinGas function:

bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

But if the second call to the Cross Chain Messenger reverts due to out of gas, the Portal won't allow for a replay and the transaction hash will be set to finalised therefore all the funds will be lost.

The transaction is also completely lost because when (let's suppose) sending assets from l2 to l1, the eth or tokens are burned on l2, therefore it is impossible to replay the transaction with higher gas input.

Impact

If a User mistakenly pass 0 or a low amount of gas as "_minGasLimit" the transaction will revert in the other chain and the user's funds will be lost, there is no safety check for this.

Optimism thoroughly validates user inputs throughout the contract, except this crucial step.

As relevant example, Arbitrum's token has $220,000 usd stuck in its contract (users mistakenly sent the tokens to arbitrum's address:) https://arbiscan.io/address/0x912ce59144191c1204e64559fe8253a0e49e6548

For Optimism's case you can say that it would be the user's fault, but Optimism validates gas inputs everywhere else.

Code Snippet

function sendMessage(
        address _target,
        bytes calldata _message,
        uint32 _minGasLimit
    ) external payable {
        // Triggers a message to the other messenger. Note that the amount of gas provided to the
        // message is the amount of gas requested by the user PLUS the base gas value. We want to
        // guarantee the property that the call to the target contract will always have at least
        // the minimum gas limit specified by the user.
        _sendMessage(
            OTHER_MESSENGER,
            baseGas(_message, _minGasLimit),
            msg.value,
            abi.encodeWithSelector(
                this.relayMessage.selector,
                messageNonce(),
                msg.sender,
                _target,
                msg.value,
                _minGasLimit,
                _message
            )
        );

        emit SentMessage(_target, msg.sender, _message, messageNonce(), _minGasLimit);
        emit SentMessageExtension1(msg.sender, msg.value);

        unchecked {
            ++msgNonce;
        }
    }

Tool used

Manual Review

Recommendation

Add a minimum gas check similar to how it is done with the first message call "baseGas".

 function sendMessage(
        address _target,
        bytes calldata _message,
        uint32 _minGasLimit
    ) external payable {
       checkGas(_minGasLimit) // <-- add good enough checks
        // Triggers a message to the other messenger. Note that the amount of gas provided to the
        // message is the amount of gas requested by the user PLUS the base gas value. We want to
        // guarantee the property that the call to the target contract will always have at least
        // the minimum gas limit specified by the user.
        _sendMessage(
            OTHER_MESSENGER,
            baseGas(_message, _minGasLimit),
            msg.value,
            abi.encodeWithSelector(
                this.relayMessage.selector,
                messageNonce(),
                msg.sender,
                _target,
                msg.value,
                _minGasLimit,
                _message
            )
        );

[GalloDaSballo]

Am thinking this is missing proof and should be escalated with a POC

There is no reasonable way to predict if the xChainGas will be sufficient, you'd need to have a oracle perform that, which is not consistent with the current architecture

[maurelian]

This is valid, but it's worth noting that:
We view this as similar to #40, except that it lacks a POC and doesn’t call out the actual attack vector as 40 does. There's a possibility we would not have accepted this if 40 didn't put the work in to demonstrate it.

[GalloDaSballo]

I'm thinking this can be douped with #37 then as they both do not explain the underlying issue in the same way, but their POCs lead to the same result

[GalloDaSballo]

Recommend:
Make dup of #37

[GalloDaSballo]

The finding tried to hit on multiple ideas, but doesn't demonstrate any, I think this should have been better developed and believe it would be unfair and incorrect to allow to escalate it, closing as Low Severity