L1SharedBridge's restriction on Weth deposit can be bypassed

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L194 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L206

Vulnerability details

Impact

L1SharedBridge's restriction on Weth deposit can be bypassed. Deprecated L1WethBridge.sol can still interact with Mailbox to deposit WETH on L2.

Proof of Concept

At current hyperchain upgrade, L1WethBridge and L2WethBrdige will be deprecated.

L1SharedBridge.sol has checks to ensure that weth deposit is not allowed in bridgehubDeposit() (_l1Token != l1WethAddress).

This check can be bypassed. Notably the new Mailbox.sol still maintains legacy requestL2Transaction() for ERA chain, but it doesn't check whether the sender is the L1Wethbridge address. As a result, users can still call L1Wethbridge deposit() which calls the new Mailbox's requestL2Transaction to deposit Weth. And tx will go through because the lack of checks on Weth bridge addresses on Mailbox.

//code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol
    function requestL2Transaction(
        address _contractL2,
        uint256 _l2Value,
        bytes calldata _calldata,
        uint256 _l2GasLimit,
        uint256 _l2GasPerPubdataByteLimit,
        bytes[] calldata _factoryDeps,
        address _refundRecipient
    ) external payable returns (bytes32 canonicalTxHash) {
        //@audit-info note: this only ensure chain is ERA, but doesn't check whether msg.sender is L1Wethbridge address. L1Wethbridge can still requestL2Transaction through Mailbox
|>      require(
            s.chainId == ERA_CHAIN_ID,
            "legacy interface only available for era token"
        );
        canonicalTxHash = _requestL2TransactionSender(
            BridgehubL2TransactionRequest({
                sender: msg.sender,
                contractL2: _contractL2,
                mintValue: msg.value,
                l2Value: _l2Value,
                l2GasLimit: _l2GasLimit,
                l2Calldata: _calldata,
                l2GasPerPubdataByteLimit: _l2GasPerPubdataByteLimit,
                factoryDeps: _factoryDeps,
                refundRecipient: _refundRecipient
            })
        );
        IL1SharedBridge(s.baseTokenBridge).bridgehubDepositBaseToken{
            value: msg.value
        }(s.chainId, msg.sender, ETH_TOKEN_ADDRESS, msg.value);

(https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/chain-deps/facets/Mailbox.sol#L206) Note that _requestL2TransactionSender() will simply put msg.sender which is L1Wethbrdige in this case as l2sender and write priority op in _requestL2Transaction(). Then, L1SharedBridge.bridgehubDepositBaseToken will be called to deposit ETH for baseCost. The _l1Token != l1WethAddress check in bridgehubDeposit() is bypassed completely.

Tools Used

Manual

Recommended Mitigation Steps

In Mailbox, consider disallow deprecated L1WethBrdige deposit through requestL2Transaction by checking msg.sender is not L1WethBridge.

Assessed type

Other

[c4-sponsor]

saxenism (sponsor) disputed

[saxenism]

This is invalid, because this mechanism is by design and anyone can create their own token. Also we wanted to prevent footguns.

[alex-ppg]

The Warden specifies that misbehavior may arise from the legacy L1WethBridge contract interacting with the Mailbox. Per the Sponsor's comments in #35, the L1WethBridge has not been deployed on the main-net and thus is not considered an active threat to the protocol in a production environment.

Regardless of this, I do not believe the outlined behavior constitutes a vulnerability as the security check of the L1SharedBridge is a rudimentary security measure meant to protect against misuse rather than the integrity or security of the contract. A user going through the hoops described to interact with the L1SharedBridge in an insecure manner for themselves cannot be considered a vulnerability.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid