zkSync Era - hals's results

Lines of code

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

Vulnerability details

Impact

• L1ERC20Bridge contract is designed to allow users to deposit ERC20 tokens from Ethereum chain to zkSync Era chain by locking funds in the contract and sending the request of processing an L2 transaction where tokens would be minted to.

• If a user wants to bridge his ERC20 tokens from L1 to L2; he invokes the L1ERC20Bridge.deposit function, then a check is made via _verifyDepositLimit function on the deposited token amount if it's within the permitted cap for each user:

      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;
          }
      }

• As can be seen; the totalDepositedAmountPerUser[_l1Token][_depositor] + _amount must be less than or equals to the limitData.depositCap of the deposited token, and this limitData.depositCap is set (and can be re-set) by the owner of the AllowList contract via AllowList.setDepositLimit function:

      function setDepositLimit(address _l1Token, bool _depositLimitation, uint256 _depositCap) external onlyOwner {
          tokenDeposit[_l1Token].depositLimitation = _depositLimitation;
          tokenDeposit[_l1Token].depositCap = _depositCap;
      }

So where exactly is the problem ?

1. first the user deposits a token and this token doesn't have a depositCap (depositLimitation is set to false), so the totalDepositedAmountPerUser[_l1Token][_depositor]=0;

2. then the depositCap of this token is updated by the AllowList contract owner; so now the depositLimitation is set to true and the depositCap of that token is supposed to be > 0.

3. then the deposit of that user is failed to be finalized/executed on L2 and now he wants to claim his deposited tokens, so he calls L1ERC20Bridge.claimFailedDeposit function.

4. by calling the L1ERC20Bridge.claimFailedDeposit function; a check is made on the claimed amount by the _verifyDepositLimit, but this function will revert due to underflow as it now tries to subtract the claimed deposited amount from totalDepositedAmountPerUser[_l1Token][_depositor] that was unintialized previously (subtracting amount from zero) when the token didn't have depositLimitation when the user first deposited.

           if (_claiming) {
               totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
           }

Proof of Concept

L1ERC20Bridge._verifyDepositLimit function

    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;
        }
    }

L1ERC20Bridge.claimFailedDeposit function/L278

 _verifyDepositLimit(_l1Token, _depositSender, amount, true);

Tools Used

Manual Testing.

Recommended Mitigation Steps

Modify L1ERC20Bridge._verifyDepositLimit function to check for the deposited amount by the user before subtraction:

    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;
+           totalDepositedAmountPerUser[_l1Token][_depositor] = totalDepositedAmountPerUser[_l1Token]       [_depositor] > 0 ? totalDepositedAmountPerUser[_l1Token][_depositor] - _amount : 0;
        } else {
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }
    }

Assessed type

Invalid Validation

Judge has assessed an item in Issue #456 as 2 risk. The relevant finding follows:

[L-01] L1ERC20Bridge._verifyDepositLimit doesn’t handle user deposits correctly if the token depositCap is changed Details Let’s see the following scenario that illustrates the issue:

first the user deposits to bridge his ERC20 tokens from L1 to L2 and the value of totalDepositedAmountPerUser[_l1Token][_depositor] will be updated. Then the owner of the AllowList removed the deposit cap of that token. The user wants to claim his L2-L1 failed deposit; but this value will not be deducted from his totalDepositedAmountPerUser; Then the admin adds a deposit cap for this token; but the totalDepositedAmountPerUser[_l1Token][_depositor] will not reflect the actual deposited amount of the user as the failed deposit hasn’t been deducted from his total amounts. So this will result in limiting the user’s deposit limit as his failed deposit hasn’t been deducted when he claimed it when the token didn’t have a depositCap.