obront - `seigManager` on `L2NativeToken` can cause withdrawals to revert, losing funds

[sherlock-admin4]

obront

High

seigManager on L2NativeToken can cause withdrawals to revert, losing funds

Summary

The minGasLimit set on L2 for an L1 withdrawal is a crucial value. Finalization of withdrawals is permissionless, so any attacker can choose the amount of gas that is sent with the transaction. In many cases, if the transaction reverts, we lose replayability and the withdrawer loses their funds.

On the L2NativeToken implementation that is planned to be used, there is a seigManager address that receives callbacks on all transfers. Any reverts that can be caused by the seigManager (most likely, gas usage causing out of gas failures) will cause the withdrawal to be lost.

Root Cause

When native tokens are sent through the bridge, each layer of the bridge calls approve() on the native token, and the next layer is responsible for calling transferFrom() to pull the tokens along.

Most importantly, we can think about the Portal calling approve() while the L1 Cross Domain Messenger calls:

if (_value > 0) {
    IERC20(_nativeTokenAddress).safeTransferFrom(address(portal), address(this), _value);
}

It is important to understand that all calls to the CrossDomainMessenger from the Portal must either succeed or set the failedMessages mapping. If a call reverts without setting failedMessages, the withdrawal will be lost, because calls from the Portal can only be played once.

In order to make this guarantee, some different gas limits are maintained: 1) The minGasLimit set by the user is used for the call from the Cross Domain Messenger, and there are checks to ensure that this much gas will be available. 2) A padded version with additional gas added is validated to be used from the Portal, to ensure that a malicious actor cannot call finalizeWithdrawalTransaction() with insufficnet gas that the call reverts.

However, these values to do not take into account the additional gas usage from the safeTransferFrom() call above. Specifically, this call happens outside of the gas checks that verify the above logic, and therefore will not save the message in failedMessages if it reverts.

This means that anything that pushes the safeTransferFrom() gas usage high enough that it could revert before getting to the failedMessages mapping update would cause the withdrawal to be bricked and lost.

There are many reasons this could happen with an arbitrary token, but with the current L2NativeToken implementation, we can see the seigManager address. When callbacks are enabled, the seigManager is called every single transfer due to this overridden _transfer() function:

function _transfer(address sender, address recipient, uint256 amount) internal override {
    super._transfer(sender, recipient, amount);
    if (callbackEnabled && address(seigManager) != address(0)) {
        require(seigManager.onTransfer(sender, recipient, amount));
    }
}

As a result, any seigManager that uses sufficient gas in that callback can be abused by an attacker to process the withdrawal transaction with too little gas, causing an early revert and loss of funds.

(Note that the same risk exists in the calls on the L1StandardBridge, which performs two transfers, but by the time the call has reached the bridge it should always be replayable, so it is a less significant issue.)

Internal Preconditions

1. seigManager is set, and the callback uses sufficient gas that it is possible to run out of gas before the failedMessages mapping is set.

External Preconditions

None.

Attack Path

1. seigManager is set to a value that uses sufficient gas for us to run out of gas before the failedMessages mapping is set.
2. An attacker sees a withdrawal transaction with a low minGasLimit.
3. The attacker calls finalizeWithdrawalTransaction() with just the amount of gas to pass the OptimismPortal2 checks, which ensure that there would be enough gas to pass the minGasLimit to the call from the Cross Domain Messenger if none of the extra functionality was added.
4. Instead, the call out to nativeToken.safeTransferFrom() uses up enough of the gas that the failedMessages mapping is not set before the function reverts.
5. The withdrawal is lost and cannot be replayed.

Impact

If the seigManager is set in such a way that it uses most of the gas that was allotted to the minGasLimit for the call (or the safeTransferFrom() call can be made to revert for some other reason), attackers can brick innocent user's withdrawals.

PoC

N/A

Mitigation

Use a low level call for the call to transferFrom() and set the failedMessages mapping before reverting in the event of a failure. This will ensure that the withdrawal can always be replayed in the event of a failure.

Additionally, it would be useful to adjust the constants around gas usage to reflect the realities of the upgraded contracts.