cmichel - Cannot bridge native L2 tokens to L1 using withdraw/withdrawTo functions

[github-actions]

cmichel

low

Cannot bridge native L2 tokens to L1 using withdraw/withdrawTo functions

Summary

The StandardBridges are supposed to bridge ERC20 tokens from L1 to L2 but also from L2 to L1.

In the case that an ERC20 token is native to L2, it will be escrowed within this contract. - L2StandardBridge

The L2 -> L1 withdrawals for native L2 tokens do not work when using the legacy L2StandardBridge.withdraw/withdrawTo functions.

Vulnerability Detail

Bridging tokes requires one native token (standard ERC20) and one OptimismMintableERC20 token on the remote chain. (Pairing two OptimismMintableERC20 does not make sense because the minting rights are at the bridges, and no tokens could ever be minted in this case.) Therefore, the native token is a simple token implementing the ERC20 interface and not created by the OptimismMintableERC20 factory.
However, the L2StandardBridge.withdraw/withdrawTo functions assume that the local token is always an OptimismMintableERC20 and it calls OptimismMintableERC20(_l2Token).l1Token() on it:

function withdrawTo(
    address _l2Token,
    address _to,
    uint256 _amount,
    uint32 _minGasLimit,
    bytes calldata _extraData
) external payable virtual {
    _initiateWithdrawal(_l2Token, msg.sender, _to, _amount, _minGasLimit, _extraData);
}

function _initiateWithdrawal(
    address _l2Token,
    address _from,
    address _to,
    uint256 _amount,
    uint32 _minGasLimit,
    bytes calldata _extraData
) internal {
    // @audit _l2Token might be a native, standard ERC20, token and does not implement the `OptimismMintableERC20.l1Token()` interface. This call will revert
    address l1Token = OptimismMintableERC20(_l2Token).l1Token();
    if (_l2Token == Predeploys.LEGACY_ERC20_ETH) {
        _initiateBridgeETH(_from, _to, _amount, _minGasLimit, _extraData);
    } else {
        _initiateBridgeERC20(_l2Token, l1Token, _from, _to, _amount, _minGasLimit, _extraData);
    }

    emit WithdrawalInitiated(l1Token, _l2Token, _from, _to, _amount, _extraData);
}

For native L2 tokens, the OptimismMintableERC20(_l2Token).l1Token() will revert as l1Token is not part of the standard ERC20 interface.

Impact

Native L2 tokens, like the OP token, cannot be bridged using these legacy functions. The functions bridgeERC20/bridgeERC20To of the StandardBridge super class must be used instead.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L170

Tool used

Manual Review

Recommendation

This might be intended to keep functionality with the legacy system the same that also couldn't do this. However, it's misleading that one function can bridge native tokens and the other cannot. Consider documenting this difference for users of the StandardBridge.

[rcstanciu]

Comment from Optimism

---

Description: Cannot bridge native L2 tokens to L1 using / functions

Reason: This is not clearly in the scope of the contest, but IMO can be called "stuck funds". The watson called this low, I'm upgrading to medium.

Action: Need to fix this in the L2 Bridge.

[trust1995]

Escalate for 250 USDC

This issue should be downgraded to a Low.

Optimism's official response is that:

This is not clearly in the scope of the contest, but IMO can be called "stuck funds". The watson called this low, I'm upgrading to medium.

However, I believe this is a misunderstanding of the issue. There are no possible stuck funds. The issue is simply pointing out that the old legacy functions will revert in some cases, so the new ones need to be used. This appears to be intended behavior, and at best should be a low to document it.

cmichel is an extremely savvy and experienced auditor. We can be sure that if there is any justification for this to be a Medium, he would have submitted it as such. It's only a misunderstanding on Optimism's part that led to this being upgraded.

[zobront]

Escalate for 250 USDC

This issue should be downgraded to a Low.

Optimism's official response is that:

This is not clearly in the scope of the contest, but IMO can be called "stuck funds". The watson called this low, I'm upgrading to medium.

However, I believe this is a misunderstanding of the issue. There are no possible stuck funds. The issue is simply pointing out that the old legacy functions will revert in some cases, so the new ones need to be used. This appears to be intended behavior, and at best should be a low to document it.

cmichel is an extremely savvy and experienced auditor. We can be sure that if there is any justification for this to be a Medium, he would have submitted it as such. It's only a misunderstanding on Optimism's part that led to this being upgraded.

[sherlock-admin]

Escalate for 250 USDC

This issue should be downgraded to a Low.

Optimism's official response is that:

This is not clearly in the scope of the contest, but IMO can be called "stuck funds". The watson called this low, I'm upgrading to medium.

However, I believe this is a misunderstanding of the issue. There are no possible stuck funds. The issue is simply pointing out that the old legacy functions will revert in some cases, so the new ones need to be used. This appears to be intended behavior, and at best should be a low to document it.

cmichel is an extremely savvy and experienced auditor. We can be sure that if there is any justification for this to be a Medium, he would have submitted it as such. It's only a misunderstanding on Optimism's part that led to this being upgraded.

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Evert0x]

Escalation accepted

Downgrading to intended low severity as funds don't get stuck like the protocol team was interpreting

[sherlock-admin]

Escalation accepted

Downgrading to intended low severity as funds don't get stuck like the protocol team was interpreting

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.