A user's funds could be unclaimable and stuck in the Bridge for an unknown amount of time, potentially forever

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L255-L285

Vulnerability details

Impact

User's funds being stuck in the bridge due to the inablity to claim their failed deposits

Proof of Concept

Take a look at L1ERC20Bridge.sol#L255-L285

    /// @dev Withdraw funds from the initiated deposit, that failed when finalizing on L2
    /// @param _depositSender The address of the deposit initiator
    /// @param _l1Token The address of the deposited L1 ERC20 token
    /// @param _l2TxHash The L2 transaction hash of the failed deposit finalization
    /// @param _l2BatchNumber The L2 batch number where the deposit finalization was processed
    /// @param _l2MessageIndex The position in the L2 logs Merkle tree of the l2Log that was sent with the message
    /// @param _l2TxNumberInBatch The L2 transaction number in a batch, in which the log was sent
    /// @param _merkleProof The Merkle proof of the processing L1 -> L2 transaction with deposit finalization
    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
        //@audit
        IERC20(_l1Token).safeTransfer(_depositSender, amount);

        emit ClaimedFailedDeposit(_depositSender, _l1Token, amount);
    }

As seen, this function is used by a user to withdraw their funds that might have failed when depositing on the L2, do note that there are no time restrictions on this and why this is being stored in the mapping mapping(address => mapping(address => mapping(bytes32 => uint256))) internal depositAmount;

That is, whenever a user decides to withdraw their funds they can just call the claimFailedDeposit() function.

So while user waits to do this, since there is no time restriction, depending on the type of token they potentially could get blocklisted for whatever reasons, do note that tokens that could blocklist users exist and two even have massive adoption of over 100 billion US dollars, namely USDC and USDT.

To find out more about tokens that could blocklist users, check this: https://github.com/d-xo/weird-erc20#tokens-with-blocklists

Issue is that, while trying to claim this failed deposit a hardcoded address _ _depositSender_ is used as the receiver, and if said receiver is now blocklisted most transactions on that address in regards to the user would fail to execute, including transfers/approvals, etc... essentially making this line revert

IERC20(_l1Token).safeTransfer(_depositSender, amount);

Since this line reverts, then the user funds are effectively stuck in the bridge.

Tool used

Manual Review

Recommended Mitigation Steps

A popular approach of fixing this is to allow _depositSender to provide a fresh address while trying to claim their failed deposits, If that's not an option that then another approach is to clearly document this possibility and require that claiming failed deposits should be done in a short time boxed manner, that way the chances of this happening have been massively reduced... the former is a better approach though since the latter never really solves the issue

Assessed type

Token-Transfer

[c4-pre-sort]

bytes032 marked the issue as low quality report

[miladpiri]

At most QA.

[c4-sponsor]

miladpiri marked the issue as disagree with severity

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[Bauchibred]

Hi @GalloDaSballo, thanks for judging, I'd like to indicate that I disagree with sponsors claim that this is "at most QA", Code4rena guidelines are there for issues like this, so I'd like to provide more points peding you re-evaluation on this.

This is similar to other classic ‘hardcoded section of protocol’ issues in other protocols that have been validated and awarded as H/Mfindings in the past with this case being the ‘hardcoded receiver addresses’, note that in some areas, this bug case leads to a complete loss of the tokens say in the case where protocol is a multi chain project and hardcodes the address to receive a token to msg.sender while making interchain transfers, or say a DOS for a classic case like when a protocol hardcodes the fee for every uniswap pool expecting all tokens to have a pool existing (or even enough liquidity) with that fee, where as the aforementioned cases is not in direct relation to the above report, it proves that hardcoding the receiver address should be considered a bug within a protocol.

Additionally, based on Code4rena's severity categorization, quoting "the function of the protocol or its availability could be impacted", that's in this case the fact that the L1ERC20Bridge#claimFailedDeposit() function would be completely unavailable to some users & since this has a hypothetical path to the loss/stuckage, one can argue with these two facts, rightfully so, that this is a medium severity finding.

As a final note to support my argument do note that this is exactly the same bug case as was reported in the Maia contest from Code4rena late Q3/early Q4 2023 which was validated as a medium, coupling this with the fact that the market share of these type of tokens is even getting larger (with USDC & USDT being at the forefront since they are the most adopted stablecoins in the market) and the crypto bull run somewhat kicking in, with zkSync being a way larger protocol compared to Maia adoption wise, so I believe there are valid grounds for this to be in fact a medium severity finding, lastly if we want to consider this a self-rekt, where as one can disagree on this being the case, other findings in this same contest that are somewhat "self-rekt" based have also been validated as H/M findings... all these points together I assume holding same severity here as medium only seems fair and upholds the C4 judging consistency and yours on this contest.

Thank you for your time.

[GalloDaSballo]

I disagree with the interpretation in this case and would also have downgraded to QA in Maia