023.md

0xdeadbeef

high

ETH sent from L2 to L1 using withdraw/withdrawTo will be lost if ERC20 tokens are bridged

Summary

After Bedrock migration, L2 users will use native ETH to withdraw. Therefore L2StandardBridge contains withdraw/withdrawTo functions that are payable. These functions can be used to bridge ETH/ERC20 tokens to L1.

Because of missing input validation, funds can be frozen in the bridge contract.

Vulnerability Detail

The vulnerability will be triggered if a used calls the L2StandardBridge withdraw/withdrawTo functions with valid ERC20 token parameters AND an ETH value (msg.value).

A user might do this by accidentally setting the amount of ERC20 as msg.value and parameter or by thinking the functions can handle bridging both in the same transaction as the comments only say "Initiates a withdrawal from L2 to L1."

Impact

If the above conditions are met, all user funds sent to the transaction will be permanently locked.

Code Snippet

Both functions withdraw and withdrawTo call _initiateWithdrawal: https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L86 https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L110

The bug is in _initiateWithdrawal: https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L162-L178

The else clause should include a check that msg.value == 0

Foundry POC:

Add the following test to L2StandardBridge.t.sol:

    function test_withdraw_funds_locked() external {
        uint256 withdrawal_amount = 100 ether;
        // Alice has 100 L2Token and 100 ether
        deal(address(L2Token), alice, withdrawal_amount, true);
        vm.deal(address(alice), withdrawal_amount);
        // validate
        assertEq(address(alice).balance, withdrawal_amount);
        assertEq(L2Token.balanceOf(address(alice)), withdrawal_amount);

        // Capture L2Bridge balance before withdraw (should be zero anyway)
        uint256 balanceBefore = address(L2Bridge).balance;

        // Withdraw both ERC20 and ETH
        vm.prank(alice, alice);
        L2Bridge.withdraw{value: withdrawal_amount}(address(L2Token), withdrawal_amount, 1000, hex"");

        // Validate both balances have zeroed 
        assertEq(address(alice).balance, 0 ether);
        assertEq(L2Token.balanceOf(address(alice)), 0);

        // Prove that the L2Bridge now holds the frozen ETH (instead of passing it to messanger)
        assertEq(address(L2Bridge).balance - balanceBefore, withdrawal_amount);
    }

Tool used

Manual Review, Foundry

Recommendation

Add the following require in the else clause of _initiateWithdraw(https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L2/L2StandardBridge.sol#L173):

} else {
        require(msg.value == 0, "Value should not be passed to ERC20 withdrawals");
        _initiateBridgeERC20(_l2Token, l1Token, _from, _to, _amount, _minGasLimit, _extraData);
}