zkSync Era - Mohandes's results

Lines of code

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

Vulnerability details

Impact

In L1ERC20Bridge when user calls claimFailedDeposit, it can fail due to underflow in _verifyDepositLimit in certain conditions, which can mean user's funds being locked in the contract forever. This is caused by the ability to turn on an off deposit limitation for the token.

Proof of Concept

The issue lies in Line 345 of L1ERC20Bridge contract, in _verifyDepositLimit function:

            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;

Here is the full code for '_verifyDepositLimit':

    function _verifyDepositLimit(address _l1Token, address _depositor, uint256 _amount, bool _claiming) internal {
        IAllowList.Deposit memory limitData = IAllowList(allowList).getTokenDepositLimitData(_l1Token);
        if (!limitData.depositLimitation) return; // no deposit limitation is placed for this token

        if (_claiming) {
            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
        } else {
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }
    }

This function does not enforce any condition or update the calculation of totalDepositedAmountPerUser when depositLimitation is false. Not updating the total deposit is what might cause a situation where the totalDepositedAmountPerUser is less than amount, which can cause underflow error. This means the user will never be able to claim their failed deposit.

Here is the scenario:

1. There is no limitation on deposits for Token A in L1 Bridge.
2. Alice deposits 1000 Token A to L1 Bridge.
3. totalDepositedAmountPerUser[Token A][Alice] is still zero due to no deposit limit
4. Alice's transaction, fails to be accessible in L2. This might be for any valid reason that has made zkSync team develop claimFailedDeposit.
5. Admin imposes a deposit limit on Token A on their discretion, not knowing (or caring) about Alice.
6. Alice tries to claim her failed deposit by calling claimFailedDeposit and it fails due to underflow. remember , totalDepositedAmountPerUser[Token A][Alice] is still zero, so deducting amount from it will cause underflow.
7. Alice will not be able to withdraw her tokens as long as there is a deposit limit.

Because this can happen to any token, and the probability of it happening is not low, and the loss of access to funds is real, unless the user can have access to the admin which might not be the case in web3 environment, I think this should be considered as High Severity.

Tools Used

Manual Review

Recommended Mitigation Steps

Doing the total deposit accounting at all times, regardless of presence or absence of deposit limitation can fix the underflow issue, but it will mean that the logic applies on any previous deposits since the launch of the project. I believe this is even fairer, compared to the existing logic that can impose limitations on all users on the same basis regardless of their past transactions.

One thing to note is that the current logic has already got some issues other than underflow as well. For example, if a token has deposit limitations, the total deposits are updated, if the limitations set to false, the total deposits don't set to zero, so later on when the limitations set, the old total deposits are back in the effect, which seems weird.

Any solution that involves resetting the total deposits will be vulnerable to possible unknown failed deposits, which won't be possible to claim. That's why I think keeping the accounting for total deposits is the best solution.

My least favourite solution is to set the totalDepositedAmountPerUser[_l1Token][_depositor] to zero when underflow happens. This might make it harder to find future bugs.

Assessed type

Math