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

Vulnerability details

In L1ERC20Bridge.sol, if the deposit() transaction for L1 -> L2 fails we can retrieve the token we deposited via the claimFailedDeposit() method. This prevents the token from being lost

claimFailedDeposit() mainly performs the following logic

1. call zkSync.proveL1ToL2TransactionStatus() to check if the task has really failed
2. call _verifyDepositLimit(_claiming=true) to reduce the limit totalDepositedAmountPerUser[_l1Token][_depositor]-= _amount
3. clear the deposit flag and return the token to the user

The second step is used to limit the maximum number of tokens that can be deposited by the user, the logic is to increase the amount when depositing and decrease the amount when failing to retrieve, if the amount exceeds the limit, it can not be deposited again.

The main code is as follows L1ERC20Bridge.sol#L340-L350

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

From the above code we know that If _l1token is a non-restricted token, then we can just return it without adding or subtracting. This will be a problem in one scenario: when the user executes deposit(), and then the administrator changes the original not need to limit to need to limit, so that in the execution of the retrieval of the token totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount will overflow.

Example. Suppose now _l1token.depositLimitation = false

1. Alice deposits 100 token, then totalDepositedAmountPerUser[_l1Token][alice] = 0
2. Administrator modifies _l1token.depositLimitation = true
3. L2 failed to execute Alice's transaction
4. Alice executes this method to retrieve the token. claimFailedDeposit() -> _verifyDepositLimit() -> totalDepositedAmountPerUser[_l1Token][alice] -=100 this will underflow totalDepositedAmountPerUser[_l1Token][alice] = 0 - 100

Alice will not be able to retrieve the token.

Impact

If the administrator turns the limit on after the user deposits, due to the above issue _verifyDepositLimit() will end up revert The user will not be able to retrieve the deposited token

Recommended Mitigation

if _amount>= totalDepositedAmountPerUser[_l1Token][_depositor] then set to 0

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

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L918

Vulnerability details

Vulnerability details

in bootloader.yul For each transaction, we need to calculate how much eth should be refunded to the user, regardless of success or failure.

There are a few main steps that we perform to calculate the refund.

1. first calculate the current gasLimitForTx and reservedGas by getGasLimitForTx(). When user specified l2GasLimit>MAX_GAS_PER_TRANSACTION, then reservedGas > 0.
2. Execute the transaction and get potentialRefund: potentialRefund, success := getExecuteL1TxAndGetRefund().
3. Operator can specify a amount to increase this refundGas (refundGas := max(getOperatorRefundForTx(transactionIndex), potentialRefund))
4. the number of refunds added to the reservedGas from the first step . refundGas := add(refundGas, reservedGas) .
5. perform a refund to the user in the final refundGas quantity

Where 2,3,4 are implemented in the following code.

            function processL1Tx(
                txDataOffset,
                resultPtr,
                transactionIndex,
                gasPerPubdata,
                isPriorityOp
            ) {
...
                let gasLimitForTx, reservedGas := getGasLimitForTx(
                    innerTxDataOffset, 
                    transactionIndex, 
                    gasPerPubdata, 
                    L1_TX_INTRINSIC_L2_GAS(), 
                    L1_TX_INTRINSIC_PUBDATA()
                )

                
                if gt(gasLimitForTx, gasUsedOnPreparation) {
                    let potentialRefund := 0

                    potentialRefund, success := getExecuteL1TxAndGetRefund(txDataOffset, sub(gasLimitForTx, gasUsedOnPreparation))

                    // Asking the operator for refund
                    askOperatorForRefund(potentialRefund)

                    // In case the operator provided smaller refund than the one calculated
                    // by the bootloader, we return the refund calculated by the bootloader.
                    refundGas := max(getOperatorRefundForTx(transactionIndex), potentialRefund)
                }
@>              refundGas := add(refundGas, reservedGas)

The problem is in step 4 refundGas := add(refundGas, reservedGas). Due to the use of add(), which does not prevent overflow, and the fact that refundGas can come from getOperatorRefundForTx(transactionIndex) specified by the Operator.

This way operator can maliciously specify a very large value, causing add() to overflow and refundGas to become 0 thus stealing the user's refund

Impact

refundGas overflow to 0 due to use of add() method Malicious operator can steal refunds from users

Proof of Concept

The following code assumes that reservedGas>0 A malicious operator could specify getOperatorRefundForTx() via add() overflow Changing refundGas from 10000 to 0

$ chisel

➜ uint256 reservedGas = 100
➜ uint256 potentialRefund = 10000
➜ uint256 refundGas = potentialRefund
➜ uint256 getOperatorRefundForTx = type(uint256).max - 99
➜ assembly{
	refundGas := getOperatorRefundForTx // max(getOperatorRefundForTx,potentialRefund)
	refundGas := add(refundGas, reservedGas)
}
➜ refundGas
Type: uint
├ Hex: 0x0
└ Decimal: 0
➜ 

Recommended Mitigation

use safeAdd

            function processL1Tx(
                txDataOffset,
                resultPtr,
                transactionIndex,
                gasPerPubdata,
                isPriorityOp
            ) {
...
-              refundGas := add(refundGas, reservedGas)
+              refundGas := safeAdd(refundGas, reservedGas)

                if gt(refundGas, gasLimit) {
                    assertionError("L1: refundGas > gasLimit")
                }

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L261

Vulnerability details

Vulnerability details

currently all L1 -> L2 transactions are initiated by Mailbox.requestL2Transaction() This method can transport eth from L1 to L2 (via the L2Value parameter) To be on the safe side, we can enable a limit on the maximum number of eth transferred per user

This is done mainly by accumulating s.totalDepositedAmountPerUser[_depositor]

The corresponding logic code is as follows.

contract MailboxFacet is Base, IMailbox {

    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) {
...
@>      _verifyDepositLimit(msg.sender, msg.value);
....
    }

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

When the limit is turned on, just calling zksync.requestL2Transaction() will accumulate s.totalDepositedAmountPerUser[msg.sender].

Currently L1WethBridge.sol is used to facilitate the transfer of weth, also calling zksync.requestL2Transaction() for the transfer of eth.

contract L1WethBridge is IL1Bridge, AllowListed, ReentrancyGuard {

    function deposit(
        address _l2Receiver,
        address _l1Token,
        uint256 _amount,
        uint256 _l2TxGasLimit,
        uint256 _l2TxGasPerPubdataByte,
        address _refundRecipient
    ) external payable nonReentrant senderCanCallFunction(allowList) returns (bytes32 txHash) {
...
        txHash = zkSync.requestL2Transaction{value: _amount + msg.value}(
            l2Bridge,
            _amount,
            l2TxCalldata,
            _l2TxGasLimit,
            _l2TxGasPerPubdataByte,
            new bytes[](0),
            refundRecipient
        );

        emit DepositInitiated(txHash, msg.sender, _l2Receiver, _l1Token, _amount);
    }

This causes a problem, all users of L1WethBridge are using the same eth depositLimitation, because the msg.sender in Mailbox.requestL2Transaction() are all L1WethBridge address s.totalDepositedAmountPerUser[L1WethBridge] += _amount;

This leads to a scenario Suppose we now need to limit the maximum transfer per user for eth limitData[address(0)].depositLimitation= 1000 eth

At this point, L1WethBridge will use the same amount of 1000 eth, which is totally insufficient and can easily exceed the limit, resulting in L1WethBridge being equivalent to a failure. If we increase the limit to a large amount, then the user can skip L1WethBridge and call `Mailbox.requestL2Transaction()``directly to use this large amount.

L1ERC20Bridge.sol has a similar problem, only less obvious, because GAS is harder to exceed the limit.

Impact

If the eth limit is turned on, all users share the same eth limit resulting in L1WethBridge.sol quickly exceeding the limit, making it essentially unusable

Recommended Mitigation

1. The depositLimitation of the same token should be able to be set separately for different _depositor.
2. L1WethBridge limits the amount of WETH by itself, similar to L1ERC20Bridge.sol.

-    function getTokenDepositLimitData(address _l1Token) external view returns (Deposit memory) {
+    function getTokenDepositLimitData(address _l1Token,address deposit) external view returns (Deposit memory) {
....

Assessed type

Context