zkSync Era - shealtielanz'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

Users call to claimFailedDeposit() will always revert if the token didn't have a deposit limit when they initiated a deposit to L2, but has a deposit limit when they try to claim their tokens back on L1 if the deposit failed, thereby losing their tokens forever.

Function Supposed FLow

When Users call deposit() on L1ERC20Bridge.sol, The deposit is Initiated to be finalized on L2 where it locks the tokens on L1 and sends the request of processing an L2 transaction to L2 where the tokens will be minted.

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

        bytes memory l2TxCalldata = _getDepositL2Calldata(msg.sender, _l2Receiver, _l1Token, amount);
        // If the refund recipient is not specified, the refund will be sent to the sender of the transaction.
        // Otherwise, the refund will be sent to the specified address.
        // If the recipient is a contract on L1, the address alias will be applied.
        address refundRecipient = _refundRecipient;
        if (_refundRecipient == address(0)) {
            refundRecipient = msg.sender != tx.origin ? AddressAliasHelper.applyL1ToL2Alias(msg.sender) : msg.sender;
        }
        l2TxHash = zkSync.requestL2Transaction{value: msg.value}(
            l2Bridge,
            0, // L2 msg.value
            l2TxCalldata,
            _l2TxGasLimit,
            _l2TxGasPerPubdataByte,
            new bytes[](0),
            refundRecipient
        );

        // Save the deposited amount to claim funds on L1 if the deposit failed on L2
        depositAmount[msg.sender][_l1Token][l2TxHash] = amount;

        emit DepositInitiated(l2TxHash, msg.sender, _l2Receiver, _l1Token, amount);
    }

In the function, before it calls zkSync.requestL2Transaction, it first verifies that the deposit limit of the msg.sender to the supposed token has not been reached for the requested amount via _verifyDepositLimit(_l1Token, msg.sender, _amount, false) Link here

   function _verifyDepositLimit(address _l1Token, address _depositor, uint256 _amount, bool _claiming) internal {
    //@audit here it gets the deposit limit data of the token
        IAllowList.Deposit memory limitData = IAllowList(allowList).getTokenDepositLimitData(_l1Token);
    //@audit here it checks if a deposit limit has been set for the token, if a deposit limit has not been set
    //It skips the if_else statement below by returning.
        if (!limitData.depositLimitation) return; // no deposit limitation is placed for this token
    //@audit When depositing `_claiming` is set to false, but when claiming a failed deposit, `_claiming` is set to True.
        if (_claiming) {
    //@audit when claiming it deducts the amount requested from the mapping below 
            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
        } else {
    //@audit When depositing, it checks if the user hasn't passed their limit and it adds the amount to their mapping
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }
    }

so when a deposit is initiated on L1, the function checks that the depositor has not crossed their deposit limit for that particular token and if the user has not it adds the amount to the totalDepositedAmountPerUser[_l1Token][_depositor] mapping of the depositor, in a case where the finalization of the deposit fails on L2, the protocol has a claimFailedDeposit() function to help the users claim their failed deposit

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

when claiming the function also verifies the amount in order to update the depositors limit for that token by deducting it from the depositors totalDepositedAmountPerUser[_l1Token][_depositor] mapping to ensure that the deposits limit is still accurate as the deposit to L2 failed.

Assumed Flow:

• Depositor Bob Intializes a deposit on L2
  • If the token has a limit placed on it, it checks that Bob hasn't crossed his limit for the token via totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap and if Bob is a good boy meaning that he hasn't reached his limit for that token, it adds the amount to Bob's mapping here totalDepositedAmountPerUser[_l1Token][_depositor] += _amount to track the amount he has brigded.
• The depsoit to L2 fails and in order to claim back his token.
  • Bob calls claimFailedDeposit() where it verifies that Bob Transaction to L2 failed, it then gets the amount of Bob's tokens that was locked via uint256 amount = depositAmount[_depositSender][_l1Token][_l2TxHash] it then updates Bobs Limit since the transaction failed by calling _verifyDepositLimit(_l1Token, _depositSender, amount, true) then it transfers to Bob back his locked tokens.
  • Bob is happy ;), where he can come back another day to deposit on L2.

The Issue: An assumption that the desired function logic is executed without any issues is made here but remember if the token Bob had chosen to bridge to L2 did not have a limit placed on it, when Bob Initiates a deposit to L2 the and _verifyDepositLimit(_l1Token, msg.sender, _amount, false) is called the check if (!limitData.depositLimitation) causes the function to return without any state change made to totalDepositedAmountPerUser of Bob.

    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

So if the transaction fails, when Bobs calls claimFailedDeposit() the check to verify via _verifyDepositLimit(_l1Token, _depositSender, amount, true) doesn't make any state changes to Bobs totalDepositedAmountPerUser.

Breaking Point Now consider the fact that the owner of the AllowList.sol can set the Deposit Limit of any token accepted by zkSync for bridging, via setDepositLimit() supposed at any time.

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

In a scenario where Bob Initiates a deposit to L2, consider that the token Bob wants to bridge has no deposit limit placed on it, when _verifyDepositLimit(_l1Token, msg.sender, _amount, false) is called since no limit is placed on the token, the check if (!limitData.depositLimitation) return; will cause _verifyDepositLimit() to return without Bobs totalDepositedAmountPerUser being updated with the amount the wishes to bridge, if the transaction fails to be finalized on L2, the only way Bob can claim back his locked tokens is to call claimFailedDeposit(), The Issue here is that if the owner sets a deposit limit on the token after Bob Initializes the deposit, if his transaction doesn't finalize on L2, when he tries to claim his tokens that are locked on L1, his transaction would always revert as, now the line

if (!limitData.depositLimitation) return;

will be skipped and the function will try to execute the following line.

    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
      //@audit When claiming the `if-statement` subtracts the `amount` from `0`, causing it to revert, `0 - amount` will throw since `totalDepositedAmountPerUser` is mapping to a `uint256` 
        if (_claiming) {
            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
        } else {
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }
    }

since the token didn't have a limit when Bob Initialized the transaction, his mapping to the token limit is left at the default value 0, when Bob tries to claim his token back(Now the token has a limit since the owner of Allowlist.sol set it) the line subtracting the amount from his totalDepositedAmountPerUser mapping

totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;` will always revert.

will always revert.

Proof of Concept

• Bob wants to bridge 10,000 USDC Tokens L2(with USDC Tokens having no deposit limit as of when he initializes the deposit), His Tokens are locked on L1 with a request sent to L2.
• since the token doesn't have a deposit limit when he called the transaction.

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

  Bob's totalDepositedAmountPerUser is never updated.
• Bob's Transaction failed to be finalized on L2.
• The Owner of Allowlist.sol decides to set a limit on USDC Tokens for security reasons or knowingly in order to grief Bob, being aware of the bug.

•     /// @dev Set deposit limit data for a token
    /// @param _l1Token The address of L1 token
    /// @param _depositLimitation deposit limitation is active or not
    /// @param _depositCap The maximum amount that can be deposited.
    function setDepositLimit(address _l1Token, bool _depositLimitation, uint256 _depositCap) external onlyOwner {
        tokenDeposit[_l1Token].depositLimitation = _depositLimitation;
        tokenDeposit[_l1Token].depositCap = _depositCap;
    }

• Bob becomes aware that his transaction failed after a while and calls claimFailedDeposit()

•    function claimFailedDeposit(
    // --- Input Params ---
    ) external nonReentrant senderCanCallFunction(allowList) {
      //  --- Some code to prove tranction is valid ---
        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);
    }

  Now that the token has a limit the if (!limitData.depositLimitation) return; will be skipped, where

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

  _claiming is True, the function tries to subtract the amount from Bob's totalDepositedAmountPerUser when it is set to default as zero, you can see the state variable totalDepositedAmountPerUser is a mapping to a uint256

     /// @dev The accumulated deposited amount per user.
    /// @dev A mapping L1 token address => user address => the total deposited amount by the user
    mapping(address => mapping(address => uint256)) public totalDepositedAmountPerUser;

  The line will throw an error, causing the transaction to revert.

No matter what Bob does his tokens are lost forever, as the function will keep on reverting, There are many pathways to exploit this issue but due to time constriant, ill leave it here.

Impact here is high as there's Loss of funds but the pathway is actually kind of unique but very possible I'll leave it as Medium Severity, allowing the Judge to have the final say on the severity.

Tools Used

God, Manual Analysis + Weird Thinking, Trust fluidity for glory report

Assessed type

Invalid Validation