zkSync Era - Aymen0909's results

Lines of code

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

Vulnerability details

Impact

Due to the way the ERC20 deposit limit is implemented in L1ERC20Bridge._verifyDepositLimit it could lead to unexpected behaviours :

• If the cap is set after a given user has already deposited and his deposit tx fail he will no be able to get his tokens back as the claimFailedDeposit will be blocked (DOS) due to an underflow error in _verifyDepositLimit.

• If the cap is removed before a given user claims his failed deposit, its total deposited amount will no be updated (decreased) and thus if the cap is introduced in the future, he will have less deposit amount allowed.

Both those scenario will have a negative impact on the user especially the first one as the user token will be blocked in the bridge contract.

Proof of Concept

Each time a user deposits tokens in the L1ERC20Bridge contract the _verifyDepositLimit function will be called :

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

   ...
}

The L1ERC20Bridge._verifyDepositLimit function is illustrated below and we see that when _claiming == false there a check to apply the deposit limit on the depositor and its total deposited amount balance is incremented :

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

If a user deposit transaction has failed he is allowed to claim his token back through claimFailedDeposit function which will also call _verifyDepositLimit but this time with _claiming == true which will result in the decrease of the depositor total deposited amount balance.

Now There are two scenarios in which this deposit cap impact negatively the depositor :

• Scenario 1 :

Let's say Bob calls L1ERC20Bridge.deposit to deposit an amount amountBob of a given ERC20 token tokenBob and let's suppose this is the first time that Bob deposits this token.

Let's also suppose that the deposit cap is disabled for this token and so when _verifyDepositLimit is called by L1ERC20Bridge.deposit, Bob total deposited amount will not be incremented : totalDepositedAmountPerUser[tokenBob][Bob] = 0

Now Bob deposit transaction has failed (for some reason) and before he gets to call claimFailedDeposit to get his tokens back the deposit cap on the tokenBob token was enabled, when Bob tries to call claimFailedDeposit the _verifyDepositLimit is triggered with _claiming == true and the function will try to decrement Bob total deposited amount :

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

But because Bob balance was not incremented when he deposited (it's equal to 0) this operation will underflow and the claimFailedDeposit call will revert, Bob will not be able to get his tokens back until the cap is disabled again as for him the claimFailedDeposit function will always revert (DOS).

• Scenario 2 :

In this scenario let's take the same example of the user Bob but now let's suppose that the token deposit cap is active.

When Bob call L1ERC20Bridge.deposit, the _verifyDepositLimit function will increment Bob total deposited amount : totalDepositedAmountPerUser[tokenBob][Bob] = amountBob

In this scenario also Bob deposit transaction has failed and before he gets to call claimFailedDeposit to get his tokens back the deposit cap on the tokenBob token was disabled.

Now Bob will call claimFailedDeposit which internally calls _verifyDepositLimit with _claiming == true, this will not decrement Bob total deposit amount balance because the following check will be triggered :

if (!limitData.depositLimitation) return; // no deposit limitation is placed for this token

Thus _verifyDepositLimit will return early without decreasing Bob deposit balance, this will not have an instant impact as Bob will still get his deposited tokens back.

But if in the future the tokenBob token deposit cap is enabled again, Bob will not be able to deposit as much as he is supposed to do as his otal deposit balance will still be : totalDepositedAmountPerUser[tokenBob][Bob] = amountBob when in fact it should be zero.

In both those scenarios the user will be negatively impacted especially in the first one where his tokens will get stuck in the bridge contract

Tools Used

Manual review

Recommended Mitigation Steps

To avoid both those scenarios the _verifyDepositLimit should be modified as follows :

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 always update the user total deposited balance
    if (_claiming) {
        totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
    } else {
        if (limitData.depositLimitation){
            //@audit check only if cap is enabled
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
        }
        totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
    }
}

With this modification the user's total deposit amount is always updated and thus both scenarios mentioned above won't be possible.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L236-L273 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L275-L281

Vulnerability details

Impact

The ETH deposit limit implemented in MailboxFacet.requestL2Transaction will not work as expected, because the _verifyDepositLimit is limiting the deposit from msg.sender which is the L1 bridge instead of limiting the real depositor (the user that deposited the token amount in the L1 bridge). This issue will make it impossible to ever have an ETH deposit limit for each depositor.

Proof of Concept

The issue occurs in the requestL2Transaction function below :

function requestL2Transaction(
    address _contractL2,
    uint256 _l2Value,
    bytes calldata _calldata,
    uint256 _l2GasLimit,
    uint256 _l2GasPerPubdataByteLimit,
    bytes[] calldata _factoryDeps,
    address _refundRecipient
) external payable nonReentrant senderCanCallFunction(s.allowList) returns (bytes32 canonicalTxHash) {
    // Change the sender address if it is a smart contract to prevent address collision between L1 and L2.
    // Please note, currently zkSync address derivation is different from Ethereum one, but it may be changed in the future.
    address sender = msg.sender;
    if (sender != tx.origin) {
        sender = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
    }

    // Enforcing that `_l2GasPerPubdataByteLimit` equals to a certain constant number. This is needed
    // to ensure that users do not get used to using "exotic" numbers for _l2GasPerPubdataByteLimit, e.g. 1-2, etc.
    // VERY IMPORTANT: nobody should rely on this constant to be fixed and every contract should give their users the ability to provide the
    // ability to provide `_l2GasPerPubdataByteLimit` for each independent transaction.
    // CHANGING THIS CONSTANT SHOULD BE A CLIENT-SIDE CHANGE.
    require(_l2GasPerPubdataByteLimit == REQUIRED_L2_GAS_PRICE_PER_PUBDATA, "qp");

    // The L1 -> L2 transaction may be failed and funds will be sent to the `_refundRecipient`,
    // so we use `msg.value` instead of `_l2Value` as the bridged amount.
    //@audit msg.sender will be L1 bridge
    _verifyDepositLimit(msg.sender, msg.value);
    canonicalTxHash = _requestL2Transaction(
        sender,
        _contractL2,
        _l2Value,
        _calldata,
        _l2GasLimit,
        _l2GasPerPubdataByteLimit,
        _factoryDeps,
        false,
        _refundRecipient
    );
}

As you can see from the code above when the _verifyDepositLimit function is called, the msg.sender address is passed in the depositor field, and as both L1 bridges (L1ERC20Bridge and L1WethBridge) will make a call to requestL2Transaction when their respective deposit function are called by a user we will have msg.sender == l1Bridge.

The _verifyDepositLimit is shown below :

function _verifyDepositLimit(address _depositor, uint256 _amount) internal {
    IAllowList.Deposit memory limitData = IAllowList(s.allowList).getTokenDepositLimitData(address(0)); // address(0) denotes the ETH
    if (!limitData.depositLimitation) return; // no deposit limitation is placed for ETH

    require(s.totalDepositedAmountPerUser[_depositor] + _amount <= limitData.depositCap, "d2");
    s.totalDepositedAmountPerUser[_depositor] += _amount;
}

Because when L1 bridge calls requestL2Transaction, the depositor field is set to msg.sender == l1Bridge address, this means that all the users that send their ETH deposit transactions through the L1 bridge will share the same ETH deposit amount per user stored in s.totalDepositedAmountPerUser[address(l1Bridge)], as in this case we will always get :

s.totalDepositedAmountPerUser[address(l1Bridge)] += _amount;

Thus if the users : Alice, Bob and Lewis deposit respectively 2 ETH, 10 ETH, 5 ETH through the L1WethBridge deposit function we will end up with following deposit amount for each user :

s.totalDepositedAmountPerUser[Alice] = 0 s.totalDepositedAmountPerUser[Bob] = 0 s.totalDepositedAmountPerUser[Lewis] = 0 s.totalDepositedAmountPerUser[address(l1Bridge)] = 10+2+5 = 17

When in reality we should have :

s.totalDepositedAmountPerUser[Alice] = 2 s.totalDepositedAmountPerUser[Bob] = 10 s.totalDepositedAmountPerUser[Lewis] = 5 s.totalDepositedAmountPerUser[address(l1Bridge)] = 0

This issue will make it impossible to ever apply the ETH deposit limit correctly as the accounting will never be done for each user.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue you should give the tx.origin address as input to _verifyDepositLimit instead of msg.sender because the tx.origin address will point to the actual user that sent the deposit transaction through the bridge :

function requestL2Transaction(
    address _contractL2,
    uint256 _l2Value,
    bytes calldata _calldata,
    uint256 _l2GasLimit,
    uint256 _l2GasPerPubdataByteLimit,
    bytes[] calldata _factoryDeps,
    address _refundRecipient
) external payable nonReentrant senderCanCallFunction(s.allowList) returns (bytes32 canonicalTxHash) {
    // Change the sender address if it is a smart contract to prevent address collision between L1 and L2.
    // Please note, currently zkSync address derivation is different from Ethereum one, but it may be changed in the future.
    address sender = msg.sender;
    if (sender != tx.origin) {
        sender = AddressAliasHelper.applyL1ToL2Alias(msg.sender);
    }

    // Enforcing that `_l2GasPerPubdataByteLimit` equals to a certain constant number. This is needed
    // to ensure that users do not get used to using "exotic" numbers for _l2GasPerPubdataByteLimit, e.g. 1-2, etc.
    // VERY IMPORTANT: nobody should rely on this constant to be fixed and every contract should give their users the ability to provide the
    // ability to provide `_l2GasPerPubdataByteLimit` for each independent transaction.
    // CHANGING THIS CONSTANT SHOULD BE A CLIENT-SIDE CHANGE.
    require(_l2GasPerPubdataByteLimit == REQUIRED_L2_GAS_PRICE_PER_PUBDATA, "qp");

    // The L1 -> L2 transaction may be failed and funds will be sent to the `_refundRecipient`,
    // so we use `msg.value` instead of `_l2Value` as the bridged amount.
    //@audit use `tx.origin` address to apply limit on real depositor
    _verifyDepositLimit(tx.origin, msg.value);
    canonicalTxHash = _requestL2Transaction(
        sender,
        _contractL2,
        _l2Value,
        _calldata,
        _l2GasLimit,
        _l2GasPerPubdataByteLimit,
        _factoryDeps,
        false,
        _refundRecipient
    );
}

Assessed type

Context