finalizing withdrawals and claiming failed deposits in L1 bridges for proven and executed batches won't be possible when zkSync is frozen

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L49-L109

Vulnerability details

Impact

functions proveL2MessageInclusion(), proveL2LogInclusion() and proveL1ToL2TransactionStatus() in MailBox contract have view modifier and they only verify a message inclusion in proven and executed batch commitment. but because they are inside MailBox facet if the zkSync diamond was frozen then it won't be possible to call them. the side effect is that some functions of L1ERC20Bridge and L1WethBridge won't be callable like claimFiledDeposits() and finializeWithdraw() for proven and executed batches. the users funds won't be accessible while the zkSync diamond is frozen and this is in contradiction to how zk-rollups designed. when a batch commitment is proved and executed in the zkSync then it is final and those data should be available and verifiable by anyone. even so freezing the MailBox is only possible by Governance contract, I believe Governance shouldn't be able to block users access to their funds that legitimately has been proved and executed in the L2 network (zkSync contracts). so IMO this has Medium Severity because Governance can block users access to their funds. when a batch executes in the zkSync then it is final and even governance shouldn't be able to deny others on-chain contract access to those verified batches.

Proof of Concept

This is proveL2MessageInclusion(), proveL2LogInclusion() and proveL1ToL2TransactionStatus() codes:

    function proveL2MessageInclusion(
        uint256 _batchNumber,
        uint256 _index,
        L2Message memory _message,
        bytes32[] calldata _proof
    ) public view returns (bool) {
        return _proveL2LogInclusion(_batchNumber, _index, _L2MessageToLog(_message), _proof);
    }

    function proveL2LogInclusion(
        uint256 _batchNumber,
        uint256 _index,
        L2Log memory _log,
        bytes32[] calldata _proof
    ) external view returns (bool) {
        return _proveL2LogInclusion(_batchNumber, _index, _log, _proof);
    }

    function proveL1ToL2TransactionStatus(
        bytes32 _l2TxHash,
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes32[] calldata _merkleProof,
        TxStatus _status
    ) public view override returns (bool) {
        // - txNumberInBatch = number of transaction in the batch
        L2Log memory l2Log = L2Log({
            l2ShardId: 0,
            isService: true,
            txNumberInBatch: _l2TxNumberInBatch,
            sender: L2_BOOTLOADER_ADDRESS,
            key: _l2TxHash,
            value: bytes32(uint256(_status))
        });
        return _proveL2LogInclusion(_l2BatchNumber, _l2MessageIndex, l2Log, _merkleProof);
    }

As you can see they are all view functions and they just check for inclusion of the message, log or transaction status in proved and executed batches. denying access to these functions is like blocking access to old verified blocks information for on-chain contracts.

This is claimFailedDeposit() and finalizeWithdrawal() code in L1ERC20Bridge:

    function claimFailedDeposit(
        address _depositSender,
        address _l1Token,
        bytes32 _l2TxHash,
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes32[] calldata _merkleProof
    ) external nonReentrant senderCanCallFunction(allowList) {
        bool proofValid = zkSync.proveL1ToL2TransactionStatus(
            _l2TxHash,
            _l2BatchNumber,
            _l2MessageIndex,
            _l2TxNumberInBatch,
            _merkleProof,
            TxStatus.Failure
        );
        require(proofValid, "yn");

        uint256 amount = depositAmount[_depositSender][_l1Token][_l2TxHash];
        require(amount > 0, "y1");

        // Change the total deposited amount by the user
        _verifyDepositLimit(_l1Token, _depositSender, amount, true);

        delete depositAmount[_depositSender][_l1Token][_l2TxHash];
        // Withdraw funds
        IERC20(_l1Token).safeTransfer(_depositSender, amount);

        emit ClaimedFailedDeposit(_depositSender, _l1Token, amount);
    }

    function finalizeWithdrawal(
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes calldata _message,
        bytes32[] calldata _merkleProof
    ) external nonReentrant senderCanCallFunction(allowList) {
        require(!isWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex], "pw");

        L2Message memory l2ToL1Message = L2Message({
            txNumberInBatch: _l2TxNumberInBatch,
            sender: l2Bridge,
            data: _message
        });

        (address l1Receiver, address l1Token, uint256 amount) = _parseL2WithdrawalMessage(l2ToL1Message.data);
        // Preventing the stack too deep error
        {
            bool success = zkSync.proveL2MessageInclusion(_l2BatchNumber, _l2MessageIndex, l2ToL1Message, _merkleProof);
            require(success, "nq");
        }

        isWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex] = true;
        // Withdraw funds
        IERC20(l1Token).safeTransfer(l1Receiver, amount);

        emit WithdrawalFinalized(l1Receiver, l1Token, amount);
    }

As you can see they call view function proveL2MessageInclusion() and proveL1ToL2TransactionStatus() to verify information about zkSync L2 blocks and if the zkSync was frozen then these logics won't work and users can't withdraw and claim their funds.

Tools Used

VIM

Recommended Mitigation Steps

move those 3 functions to Getter Facet that shouldn't be freezable.

note:

I don't have backstage access and I can't comment during Post-Judging QA duration. so if you as judge or sponsor have any question regarding the issue please contact me directly.

Assessed type

Access Control

[c4-pre-sort]

bytes032 marked the issue as low quality report

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Part of the current setup, potentially OOS as known

[miladpiri]

Although it is by design, it can be QA.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-sponsor]

miladpiri (sponsor) acknowledged

[0xunforgiven]

Hi @GalloDaSballo, Thank you for reviewing this submission.

Zero Knowledge rollups are designed to provide users with the assurance that once a transaction is finalized, no one can prevent them from accessing their funds. This is a fundamental principle of zk-rollups, and zkSync claims to adhere to this principle.

The zkSync documentation clearly affirms this promise (link1, link2):

After that, the withdrawal action will be available to be finalized by anyone in the L1 bridge (by proving the inclusion of the L2 -> L1 message, which is done when calling the finalizeWithdrawal method on the L1 bridge contract)

Finalized: This indicates that the SNARK validity proof for the transaction has been submitted and verified by the smart contract. After this step, the transaction is considered to be final.

This issue, however, demonstrates for L2 blocks that despite being verified and finalized on L1, zkSync's availability can be compromised, preventing users from accessing their funds. This contradicts the assurances given in the project documentation.

Essentially, governance has the ability to block access to zkSync's history on L1, rendering the implementation inconsistent with the documentation's promises. This risk remains unaddressed, and users should be informed of this potential for disruption. This issue is not documented as a known risk or potential issue for users in contest page or documentation.

The ability to block access to finalized transactions undermines the core principles of zk-rollups and raises serious concerns about the security and usability of zkSync.

Furthermore, my report shows the same concerns raised in this Optimism report regarding finalized blocks (in optimistic rollups txs finalized after 7 days):

if finalization period is crossed then no matter what, transaction should be considered confirmed and challenger shouldn't be able to compromise them

[miladpiri]

thanks @0xunforgiven It can be a decentralization risk and better to be put in analysis section. The freezing mechanism can be triggerred by the governor and is helpful in case of critical security situations, so when the protocol is frozen it means something unexpected has happened. So, it is safer to not allow users to withdraw. This is by design, so QA.

[GalloDaSballo]

I believe this is already acknowledged through documentation as well as the known issues, QA is appropriate