zkSync Era - cccz's results

Lines of code

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

Vulnerability details

Impact

The owner can call AllowList.setDepositLimit() to set the user's deposit cap for tokens.

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

In L1ERC20Bridge.deposit(), each deposit increases totalDepositedAmountPerUser and then checks if it reaches the deposit cap.

    function deposit(
        address _l2Receiver,
        address _l1Token,
        uint256 _amount,
        uint256 _l2TxGasLimit,
        uint256 _l2TxGasPerPubdataByte,
        address _refundRecipient
    ) public payable nonReentrant senderCanCallFunction(allowList) returns (bytes32 l2TxHash) {
        require(_amount != 0, "2T"); // empty deposit amount
        uint256 amount = _depositFunds(msg.sender, IERC20(_l1Token), _amount);
        require(amount == _amount, "1T"); // The token has non-standard transfer logic
        // verify the deposit amount is allowed
        _verifyDepositLimit(_l1Token, msg.sender, _amount, false);
...
    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;
        }
    }

Then if the deposit fails, totalDepositedAmountPerUser is decreased in claimFailedDeposit().

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

The problem here is that if setDepositLimit() is called to set the deposit cap after the user has made a deposit, and then if the deposit fails, the user's claimFailedDeposit() call will be reverted due to overflow. Consider the following scenario.

1. Alice bridges 10000 USDC to L2, and the owner has not set a deposit cap on USDC.
2. The owner calls setDepositLimit() to set the deposit cap of USDC to 1M.
3. Alice's deposit transaction fails, and Alice calls claimFailedDeposit() to retrieve the 10000 USDC deposited, but in _verifyDepositLimit(), since totalDepositedAmountPerUser[USDC][Alice] == 0, the subtraction operation here overflows and Alice is unable to retrieve the 10000 USDC.

    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; // @auditor: overflow here

To run the POC, we need to change _verifyDepositLimit() to public first.

diff --git a/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol b/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol
index 9d134f8..827781c 100644
--- a/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol
+++ b/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol
@@ -337,7 +337,7 @@ contract L1ERC20Bridge is IL1Bridge, IL1BridgeLegacy, AllowListed, ReentrancyGua
     }
 
     /// @dev Verify the deposit limit is reached to its cap or not
-    function _verifyDepositLimit(address _l1Token, address _depositor, uint256 _amount, bool _claiming) internal {
+    function _verifyDepositLimit(address _l1Token, address _depositor, uint256 _amount, bool _claiming) public {
         IAllowList.Deposit memory limitData = IAllowList(allowList).getTokenDepositLimitData(_l1Token);
         if (!limitData.depositLimitation) return; // no deposit limitation is placed for this token

Then write the following POC that shows that calling setDepositLimit() after the user deposit, _verifyDepositLimit() in claimFailedDeposit() will be reverted due to overflow

diff --git a/code/contracts/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts b/code/contracts/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts
index ce6a634..7aab014 100644
--- a/code/contracts/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts
+++ b/code/contracts/ethereum/test/unit_tests/l1_erc20_bridge_test.spec.ts
@@ -152,6 +152,20 @@ describe(`L1ERC20Bridge tests`, function () {
         );
     });
 
+    it(`POC`, async () => {
+        const depositorAddress = await randomSigner.getAddress();
+        await depositERC20(
+            l1ERC20Bridge.connect(randomSigner),
+            zksyncContract,
+            depositorAddress,
+            testnetERC20TokenContract.address,
+            ethers.utils.parseUnits('800', 18),
+            10000000
+        );
+        allowList.setDepositLimit(testnetERC20TokenContract.address,true,ethers.utils.parseUnits('1000000', 18));
+        l1ERC20Bridge._verifyDepositLimit(testnetERC20TokenContract.address,depositorAddress,ethers.utils.parseUnits('10000', 18),true);
+    });
+
     it(`Should revert on finalizing a withdrawal with wrong message length`, async () => {
         const revertReason = await getCallRevertReason(
             l1ERC20Bridge.connect(randomSigner).finalizeWithdrawal(0, 0, 0, '0x', [])

Proof of Concept

https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L340-L350 https://github.com/code-423n4/2023-10-zksync/blob/72f5f16ed4ba94c7689fe38fcb0b7d27d2a3f135/code/contracts/ethereum/contracts/common/AllowList.sol#L129-L132

Tools Used

None

Recommended Mitigation Steps

It is recommended that the subtraction operation in claimFailedDeposit() be performed only when totalDepositedAmountPerUser is greater than _amount.

    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) {
+           if(totalDepositedAmountPerUser[_l1Token][_depositor] >= _amount)
                totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;

Assessed type

Context