User might not be able to withdraw WETH/ETH on L1 with legacy l2Batchnumber, resulting in ETH permanently locked in L1SharedBridge.sol

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L443-L447

Vulnerability details

Impact

A user might not be able to withdraw WETH/ETH on L1 with legacy l2Batchnumber, resulting in ETH permanently locked in L1SharedBridge.sol.

The new Mailbox is not always backward compatible.

Proof of Concept

New Mailbox and L1SharedBridge are intended to be backward compatible. But the current implementation of Mailbox.sol and L1SharedBridge.sol' s withdrawal flow might cause certain legacy WETH deposits to be locked when withdrawn.

Suppose a user deposited WETH before the upgrade through old L1WETHBridge.sol. Now user initiated the withdrawal of WETH from old L2WETHBridge.sol before ERA upgrade.

(1) Since upgrade hasn't started or is about to start, user would still interact with the old L2WETHBridge.sol withdraw(), which burns user's L2Weth for ETH and send a ETH withdraw through L2_ETH_ADDRESS.withdrawWithMessage{value: _amount}(l1Bridge, wethMessage);. Note that this set l1Bridge as the l1 receiver (old L1WETHbridge.sol). The user designated _l1Receiver is part of wethMessage.

//L2WethBridge.sol (legacy, for reference only)
    function withdraw(
        address _l1Receiver,
        address _l2Token,
        uint256 _amount
    ) external override {
...
        // WETH withdrawal message.
        bytes memory wethMessage = abi.encodePacked(_l1Receiver);

        // Withdraw ETH to L1 bridge.
        //@audit-info note: legacy L2WethBridge will put L1WethBridge address as l1Reciver. Actual user address(`_l1Receiver`) will be encoded in message.
|>       L2_ETH_ADDRESS.withdrawWithMessage{value: _amount}(l1Bridge, wethMessage);

(2) Now if L1 ERA upgrade hasn't started yet, the user will still call L1WethBrdige.sol's finalizeWithdrawal() to withdraw WETH. However, if L1 ERA-upgrade settles first before the user's finalizeWithdrawal() call. L1WethBridge.sol's finalizeWithdrawal() tx will call the new mailbox's finalizeEthWithdrawal() function.

The problem occurs here: Although the new Mailbox.sol maintains finalizeEthWithdrawal(), this won't be backward compatible with a legacy WETH withdrawal flow.

The new finalizeEthWithdrawal() will call L1SharedBridge's finalizeWithdrawal(). L1SharedBridge will send ETH to l1Reciver(L1WethBridge address, Not the user's address). However, the ETH transfer will revert due to L1WethBridge.sol will not recognize L1SharedBridge.sol's address and revert in the receive() fallback.

//code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol
    function finalizeEthWithdrawal(
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes calldata _message,
        bytes32[] calldata _merkleProof
    ) external nonReentrant {
...
          //@audit-info note: in a legacy Weth withdrawal flow, new Mailbox will pass control flow to L1SharedBridge
|>        IL1SharedBridge(s.baseTokenBridge).finalizeWithdrawal(
            ERA_CHAIN_ID,
            _l2BatchNumber,
            _l2MessageIndex,
            _l2TxNumberInBatch,
            _message,
            _merkleProof
        );

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L186)

//code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol
    function _finalizeWithdrawal(
        uint256 _chainId,
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes calldata _message,
        bytes32[] calldata _merkleProof
    )
...
          //@audit-info note: when a user finalize a legacy weth withdrawal, this l1Receiver will be legacy L1WethBrdige address, not the actual user withdrawal receiver
|>        (l1Receiver, l1Token, amount) = _checkWithdrawal(_chainId, messageParams, _message, _merkleProof);
...
        if (l1Token == ETH_TOKEN_ADDRESS) {
            bool callSuccess;
            // Low-level assembly call, to avoid any memory copying (save gas)
            assembly {
                  //@audit-info note: in the flow of a user finalize a legacy weth withdrawal, this ETH transfer will revert at L1WethBridge.sol receive fallback.
|>                callSuccess := call(gas(), l1Receiver, amount, 0, 0, 0, 0)
            }

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L443-L447)

//L1WethBridge.sol (legacy, for reference only)
    receive() external payable {
        // Expected to receive ether in two cases:
        // 1. l1 WETH sends ether on `withdraw`
        // 2. zkSync contract withdraw funds in `finalizeEthWithdrawal`
          //@audit-info note: a legacy Weth finalize withdrawal will revert, because msg.sender will now be L1SharedBridge.sol
 |>       require(msg.sender == l1WethAddress || msg.sender == address(zkSync), "pn");
        emit EthReceived(msg.value);
    }

Note: if the user calls finalizeEthWithdrawal() directly on the new Mailbox.sol, this will also revert due to the same reason above.

Because the old L2WethBridge.sol will code L1Wethbridge.sol as L1receiver, any attempts of Weth withdrawal on L2 close to or during ERA upgrade might result in ETH permanently locked on L1SharedBridge.sol. User will lose funds.

Tools Used

Manual

Recommended Mitigation Steps

since the new Mailbox's intended to be backward compatible and there will be legacy Weth withdrawal tx, it needs to be ensured that on-going users' legacy weth withdrawal tx will not lock funds on L1SharedBridge.

Consider refactoring Mailbox's finalizeEthWithdrawal() to be compatible with the legacy Weth withdrawal flow.

Assessed type

Other

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

This is indeed a good finding, but we consider this out of scope since l1 weth bridge is not deployed on mainnet.

[alex-ppg]

The Warden details a misbehavior that might arise as part of a migration of the legacy L1WethBridge, however, the Sponsor has specified this contract has not been deployed.

I could not find any online evidence that the L1WethBridge was ever deployed or in operation, and several breaking changes have been cataloged in the contest's changelog alongside the deprecation of the L1WethBridge (incorrectly spelled as L1WethBridge).

In addition to this, the official list of main-net deployments that was available at the time the contest initiated did not list L1WethBridge as deployed based on the publicly available GitHub repository: https://github.com/matter-labs/zksync-web-era-docs/blob/b6fef08381e92c2b8b5adea6beb3ed5431c54f79/docs/build/quick-start/useful-address.md

I appreciate the Warden's thoroughness and unique approach demonstrated in this finding, but I cannot accept it as a valid vulnerability as it is practically inapplicable.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid