zkSync Era - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol#L36-L43

Vulnerability details

Proof of Concept

In order to request transaction on l2 from l1 user can call Mailbox.requestL2Transaction function. As param user provides _l2GasLimit amount. This is amount of gas that user think is enough to execute his transaction on l2 and user should pay for that amount on l1.

We should know, that transaction execution on l2 consists of batch overhead gas and actually gas needed to execute call. So _l2GasLimit provided by user should cover this total gas amount. Because of that Mailbox contract does gas check.

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol#L18-L44

    function validateL1ToL2Transaction(
        IMailbox.L2CanonicalTransaction memory _transaction,
        bytes memory _encoded,
        uint256 _priorityTxMaxGasLimit
    ) internal pure {
        uint256 l2GasForTxBody = getTransactionBodyGasLimit(
            _transaction.gasLimit,
            _transaction.gasPerPubdataByteLimit,
            _encoded.length
        );


        // Ensuring that the transaction is provable
        require(l2GasForTxBody <= _priorityTxMaxGasLimit, "ui");
        // Ensuring that the transaction cannot output more pubdata than is processable
        require(l2GasForTxBody / _transaction.gasPerPubdataByteLimit <= PRIORITY_TX_MAX_PUBDATA, "uk");


        // Ensuring that the transaction covers the minimal costs for its processing:
        // hashing its content, publishing the factory dependencies, etc.
        require(
            getMinimalPriorityTransactionGasLimit(
                _encoded.length,
                _transaction.factoryDeps.length,
                _transaction.gasPerPubdataByteLimit
            ) <= _transaction.gasLimit,
            "up"
        );
    }

So this function should check that _transaction.gasLimit is high enough value in order to cover batch overhead for this transaction and gas that is needed to execute minimal l1 transaction.

getTransactionBodyGasLimit function will calculate batch overhead and return gas limit of transaction on l2 as l2GasForTxBody variable. As only this amount of gas will be sent with a call by bootloader. gasLimit of transaction will be decreased by overhead amount and part of it can be refunded after tx execution.

And later validateL1ToL2Transaction does the check that getMinimalPriorityTransactionGasLimit <= _transaction.gasLimit. This is actually the check that user has provided enough gas to execute tx on l2, which includes costs for computation on l2. The problem is that this check is incorrect as gasLimit also includes batch overhead which will not be included with a call.

As result Mailbox may allow to include tx, that will have not enough gas to be executed on l2. Such transaction will be included in the batch, but call will not be done and part of funds will be refunded for user on l2.

Also there is a way for user to use this problem in order to bridge eth cheaper. Currently if you want to bridge eth to your account on l2, then you need to initiate request on l1 with contractAddressL2 as your address and empty calldata. So when operator includes such tx in the batch, then bootloader mints eth to from address(user address on l1) and then will execute call with empty calldata to contractAddressL2. User should pay intrinsic gas for that execution.

But now, user can initiate transaction request on l1 and provide gas limit exactly enough to pay for batch overhead. After that he will have no gas, so gas limit for tx will be set to 0 and he doesn't pay intrinsic gas. Then getExecuteL1TxAndGetRefund function will not be called as user has no gas and refund will be provided, which will send whole bridged amount(excluding wasted gas) to user's refund address. So user was able to not pay intrinsic gas payment for l2 tx and bridge funds.

Impact

User can initiate l2 txs, that can't be executed on l2. User can avoid paying more fees to bridge funds.

Tools Used

VsCode

Recommended Mitigation Steps

I think that you should check that l2GasForTxBody contains enough gas to cover minimal priority tx gas cost.

require(
            getMinimalPriorityTransactionGasLimit(
                _encoded.length,
                _transaction.factoryDeps.length,
                _transaction.gasPerPubdataByteLimit
            ) <= l2GasForTxBody,
            "up"
        );

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1668

Vulnerability details

Proof of Concept

User can initiate tx from l1 or on l2. In order to run his tx user provides gas limit which should be enough to cover batch overhead and cost of tx. As bootloader executes all txs in the batch one by one, it uses near calls to provide needed amount of gas to specific tx. In such way they can avoid storage changes in case if call reverts and some state changes should be undone. All functions that should use near call should start with ZKSYNC_NEAR_CALL in function's name. One of params that this function should receive is abi, which contains gas amount that was provided to this near call. In case if near call reverts, then nearCallPanic function is called.

Now let's check nearCallPanic function. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/bootloader/bootloader.yul#L1678-L1686

            /// @dev Used to panic from the nearCall without reverting the parent frame.
            /// If you use `revert(...)`, the error will bubble up from the near call and
            /// make the bootloader to revert as well. This method allows to exit the nearCall only.
            function nearCallPanic() {
                // Here we exhaust all the gas of the current frame.
                // This will cause the execution to panic.
                // Note, that it will cause only the inner call to panic.
                precompileCall(gas())
            }

If you read these comments, then you will see that nearCallPanic will burn all frame's gas. This gas is actually all gasLimit(except batch overhead). So what this means is that in case if user's tx will revert in the beginning and nearCallPanic is called, then all his gas will be burnt. Also in case if user provided more gas then needed to execute tx and it reverts and nearCallPanic is called, then also all that gas has burnt.

I have discovered that transactions initiated on l2 and on l1 are treated not in same way by bootloader. And l1 initiated tx will burn all provided gas in case if call reverts, while l2 tx will not do that and refund rest of gas.

This is how it's done for l1 tx. Gas amount is provided, then executeL1Tx function is called. It will execute call and return result. And then if call has reverted, then nearCallPanic is called. It's done, as it's eth was minted for that call and as it failed, then it should be refunded to the refunder. This is provided as explanation in comments. So to summarize, bootloader will call nearCallPanic for reverted tx that was initiated on l1, which means that all gas provided to the call will burn and user will not receive refund for it.

Now Let's check how this is done for l2 tx. So bootloader calls l2TxExecution function, which will return result of execution. This function will call several functions, start near call and eventually executeL2Tx function which will execute call and return result. But nearCallPanic will not be called in case if execution has reverted in this case, which means that gas that has left after execution revert will be refunded to the user.

So i have shown that currently tx initiated on l1 and l2 are treated not in same way by bootloader and gas refund is processed differently.

While currently there is no such thing as mempool in zksync, in future they will have multiple operators and we don't know how they will be implemented. So it's possible that they will have mempool and we will have usual ethereum frontrunning possibilities that we love. So in such case there will be new entertainment as frontrun someone's l1 tx to make it revert and all tx's gas to be burnt.

Impact

All unused gas for tx burns in case of revert if it's initiated on l1.

Tools Used

VsCode

Recommended Mitigation Steps

I believe that protocol should find another solution to burn minted eth after l1 tx reverts on l2 instead of near call panic. So both type of txs will have same refund mechanism.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#L132 https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/Constants.sol#L35

Vulnerability details

Proof of Concept

AccountCodeStorage.getCodeSize should return code size of provided address. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#L117-L138

    function getCodeSize(uint256 _input) external view override returns (uint256 codeSize) {
        // We consider the account bytecode size of the last 20 bytes of the input, because
        // according to the spec "If EXTCODESIZE of A is X, then EXTCODESIZE of A + 2**160 is X".
        address account = address(uint160(_input));
        bytes32 codeHash = getRawCodeHash(account);


        // If the contract is a default account or is on constructor the code size is zero,
        // otherwise extract the proper value for it from the bytecode hash.
        // NOTE: zero address and precompiles are a special case, they are contracts, but we
        // want to preserve EVM invariants (see EIP-1052 specification). That's why we automatically
        // return `0` length in the following cases:
        // - `codehash(0) == 0`
        // - `account` is a precompile.
        // - `account` is currently being constructed
        if (
            uint160(account) > CURRENT_MAX_PRECOMPILE_ADDRESS &&
            codeHash != 0x00 &&
            !Utils.isContractConstructing(codeHash)
        ) {
            codeSize = Utils.bytecodeLenInBytes(codeHash);
        }
    }

As you can see in case if account address is bigger than CURRENT_MAX_PRECOMPILE_ADDRESS and it's hash isn't empty and it's not constructing, then code size will be retrieved from codeHash and returned.

Now let's check getCodeHash function. https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/AccountCodeStorage.sol#L89-L112

    function getCodeHash(uint256 _input) external view override returns (bytes32) {
        // We consider the account bytecode hash of the last 20 bytes of the input, because
        // according to the spec "If EXTCODEHASH of A is X, then EXTCODEHASH of A + 2**160 is X".
        address account = address(uint160(_input));
        if (uint160(account) <= CURRENT_MAX_PRECOMPILE_ADDRESS) {
            return EMPTY_STRING_KECCAK;
        }


        bytes32 codeHash = getRawCodeHash(account);


        // The code hash is equal to the `keccak256("")` if the account is an EOA with at least one transaction.
        // Otherwise, the account is either deployed smart contract or an empty account,
        // for both cases the code hash is equal to the raw code hash.
        if (codeHash == 0x00 && NONCE_HOLDER_SYSTEM_CONTRACT.getRawNonce(account) > 0) {
            codeHash = EMPTY_STRING_KECCAK;
        }
        // The contract is still on the constructor, which means it is not deployed yet,
        // so set `keccak256("")` as a code hash. The EVM has the same behavior.
        else if (Utils.isContractConstructing(codeHash)) {
            codeHash = EMPTY_STRING_KECCAK;
        }


        return codeHash;
    }

It also uses CURRENT_MAX_PRECOMPILE_ADDRESS and aims to show EMPTY_STRING_KECCAK as hashcode for precompiles.

Now, let's check what CURRENT_MAX_PRECOMPILE_ADDRESS is.

https://github.com/code-423n4/2023-10-zksync/blob/main/code/system-contracts/contracts/Constants.sol#L25-L35

address constant ECRECOVER_SYSTEM_CONTRACT = address(0x01);
address constant SHA256_SYSTEM_CONTRACT = address(0x02);
address constant ECADD_SYSTEM_CONTRACT = address(0x06);
address constant ECMUL_SYSTEM_CONTRACT = address(0x07);


/// @dev The current maximum deployed precompile address.
/// Note: currently only two precompiles are deployed:
/// 0x01 - ecrecover
/// 0x02 - sha256
/// Important! So the constant should be updated if more precompiles are deployed.
uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));

So currently SHA256_SYSTEM_CONTRACT address is set as latest precompile. But it's not the last one as you can see and the last one currently is ECMUL_SYSTEM_CONTRACT.

According to the eip-1052

The EXTCODEHASH of an precompiled contract is either c5d246... or 0.

So for precompiles hashcode should be empty and size should be 0. Exactly same behaviour is implemented in the zksync getCodeSize and getCodeHash implementations. But because of incorrect CURRENT_MAX_PRECOMPILE_ADDRESS, both function will provide real info about hascode and size for other precompiles instead of 0.

Impact

Some precompiles will have non zero code size and hashcode.

Tools Used

VsCode

Recommended Mitigation Steps

Make CURRENT_MAX_PRECOMPILE_ADDRESS be up to date.

Assessed type

Error

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

QA-01. L1ERC20Bridge._verifyDepositLimit doesn’t track bridged/failed amount when limitation is off for token. Description When user bridges erc20 token through the L1ERC20Bridge, then _verifyDepositLimit function is called, which should check if user doesn’t restrict limitations. https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/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;
    }
}

As you can see, depositLimitation can be switched on and off for the token. In case if it’s on, then when user bridges, his totalDepositedAmountPerUser increases and in case if deposit has failed, then it’s decreased.

As you can see, limit can be set many times. So it’s possible that limit will be working, then will be switched off and then will be switched on again.

The problem is that when limitation is switched off, then totalDepositedAmountPerUser is not changed anymore and nothing is tracked. So when it will be switched on again, then user may already use all his limit, but totalDepositedAmountPerUser will not contain that information.

Recommendation Increase and decrease totalDepositedAmountPerUser even if limitation is switched off. So when it will be switched on again, totalDepositedAmountPerUser variable will be up to date for user.

Lines of code

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

Vulnerability details

Impact

User can bridge more using the bridge. L1WethBridge limit can be flushed so noone will be able to use bridge.

Proof of Concept

When Execute.requestL2Transaction function is called, then user can provide amount that he would like to bridge to l2. Together with fees this amount is going to be checked for msg.sender to restrict it from bridging more than allowed anount. This will just increase totalDepositedAmountPerUser for msg.sender and check if he is still fine with current limit. https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L275-L281

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

So when user has reached limit, he should not be able to bridge anymore.

L1WethBridge.deposit function allows anyone to bridge weth and receive it on l2. Function will withdraw eth from weth and then will call Execute.requestL2Transaction function. As result, msg.sender in Execute.requestL2Transaction will be L1WethBridge and not deposit initiator.

As a lot of people will be using L1WethBridge contract, that means that it's deposit limit can be flushed quickly, so user's will not be able to use bridge anymore.

I understand that currently limit is not used, but just explained potential problem if it will be used.

Tools Used

VsCode

Recommended Mitigation Steps

I think, that totalDepositedAmountPerUser variable should be increased for msg.sender of bridge.

Assessed type

Error

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

QA-05. Inconsistency in different l1->l2 messages Description When user requests l1->l2 message, then he can call ContractDeployer in order to create new contract. Or he can do request without new contract creation.

When tx will be executed by bootloader on l2, then in case if it is not creating new contract, then user’s nonce will not be changed anyhow as there is no any communication with user’s account. But when new contract will be created, then user’s nonce will be increased.

As result in case if user didn’t do any tx on l2 yet, then his account is considered not active. Then if he triggers usual tx from l1, then his account is still not active. But if he deploys contract, then his account becomes active, so hashcode and code size is shown for it.

QA-01. L1ERC20Bridge._verifyDepositLimit doesn't track bridged/failed amount when limitation is off for token.

Description

When user bridges erc20 token through the L1ERC20Bridge, then _verifyDepositLimit function is called, which should check if user doesn't restrict limitations. https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/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;
        }
    }

As you can see, depositLimitation can be switched on and off for the token. In case if it's on, then when user bridges, his totalDepositedAmountPerUser increases and in case if deposit has failed, then it's decreased.

As you can see, limit can be set many times. So it's possible that limit will be working, then will be switched off and then will be switched on again.

The problem is that when limitation is switched off, then totalDepositedAmountPerUser is not changed anymore and nothing is tracked. So when it will be switched on again, then user may already use all his limit, but totalDepositedAmountPerUser will not contain that information.

Recommendation

Increase and decrease totalDepositedAmountPerUser even if limitation is switched off. So when it will be switched on again, totalDepositedAmountPerUser variable will be up to date for user.

QA-02. Executor._verifyBatchTimestamp allows timestamp of last l2 block to be in future

Description

Executor._verifyBatchTimestamp function is used to check that batch and last l2 block timestamps are both valid.

Currently, function allows l2 last block to be in future. https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/zksync/facets/Executor.sol#L94 require(lastL2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA, "h2");

This should not happen as l2 block should never have timestamp that is bigger as it creates time games, so l2 time will be bigger than l1.

Recommendation

I think restriction should be like that or even without = sign. require(lastL2BlockTimestamp <= block.timestamp, "h2");

QA-03. AccountCodeStorage.getCodeHash will return 0 for initialized account

Description

AccountCodeStorage.getCodeHash function is used to return code hash of the address. According to eip-1052:

In case ount does not have code the keccak256 hash of empty data (i.e. c5d2460186f7233cthe account does not exist or is empty (as defined by EIP-161) 0 is pushed to the stack. In case the acc927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470) is pushed to the stack.

So in case if account has non 0 balance or it's nonce is bigger than 0, then keccak256 hash of empty data should be returned. But zksync doesn't check balance of account. It only checks for nonce. As result in case if someone will transfer funds to address, or eoa will mint value from l1 tx to zksync, then this address will be considered not initialized.

As result, getCodeHash works differently on ethereum and zksync. As i haven't found this is somehow used by real contracts, i put this as qa, however it's possible to craft contract that will depend on empty keccak256 in order to check if account is initialized.

Recommendation

I believe that it should work in same way as described in eip1052.

QA-04. L1Messenger.publishPubdataAndClearState checks that numberOfL2ToL1Logs <= numberOfL2ToL1Logs

Description

L1Messenger.publishPubdataAndClearState function receives numberOfL2ToL1Logs and then it should check that provided amount is not big enough.

Because l2ToL1LogsTreeArray has L2_TO_L1_LOGS_MERKLE_TREE_LEAVES size, that means that there can be no more than L2_TO_L1_LOGS_MERKLE_TREE_LEAVES leaves to construct tree root.

So publishPubdataAndClearState should change that numberOfL2ToL1Logs <= L2_TO_L1_LOGS_MERKLE_TREE_LEAVES, but currently it do check in another way: numberOfL2ToL1Logs <= numberOfL2ToL1Logs, which is incorrect.

As i can see, there is no real impact here, as even if operator will provide more logs than L2_TO_L1_LOGS_MERKLE_TREE_LEAVES then whole batch will just revert.

Recommendation

Use this check: numberOfL2ToL1Logs <= L2_TO_L1_LOGS_MERKLE_TREE_LEAVES

QA-05. Inconsistency in different l1->l2 messages

Description

When user requests l1->l2 message, then he can call ContractDeployer in order to create new contract. Or he can do request without new contract creation.

When tx will be executed by bootloader on l2, then in case if it is not creating new contract, then user's nonce will not be changed anyhow as there is no any communication with user's account. But when new contract will be created, then user's nonce will be increased.

As result in case if user didn't do any tx on l2 yet, then his account is considered not active. Then if he triggers usual tx from l1, then his account is still not active. But if he deploys contract, then his account becomes active, so hashcode and code size is shown for it.

Recommendation

I am not sure if this is needed to be fixed, just wanted to show differences.