zkSync Era - lsaudit's results

Lines of code

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

Vulnerability details

Impact

The CURRENT_MAX_PRECOMPILE_ADDRESS is defined as current maximum deployed precompile address. According to its NatSpec: Important! So the constant should be updated if more precompiles are deployed.

CURRENT_MAX_PRECOMPILE_ADDRESS is being utilized in system-contracts/contracts/AccountCodeStorage.sol to simulate the behavior of the extcodehash and extcodesize EVM opcode (functions getCodeHash() and getCodeSize(). Whenever CURRENT_MAX_PRECOMPILE_ADDRESS is not correctly defined, those function won't behave as expected and the whole AccountCodeStorage won't return expected results.

Proof of Concept

When we look at the CURRENT_MAX_PRECOMPILE_ADDRESS, it's defined as: uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT));. It assumes that there are only two precompiled contracts and the SHA256_SYSTEM_CONTRACT is the last one. However, there are 4 precompiled contracts, the last one is ECMUL_SYSTEM_CONTRACT:

File: system-contracts/contracts/Constants.sol

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

This implies, that CURRENT_MAX_PRECOMPILE_ADDRESS wasn't updated when ECADD_SYSTEM_CONTRACT and ECMUL_SYSTEM_CONTRACT were added, thus its value is incorrect. It ignores two additional precompiled contracts.

Tools Used

Manual code review

Recommended Mitigation Steps

Change uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(SHA256_SYSTEM_CONTRACT)); to uint256 constant CURRENT_MAX_PRECOMPILE_ADDRESS = uint256(uint160(ECMUL_SYSTEM_CONTRACT));

Assessed type

Other

[QA-1] Unsafe addition in unchecked block may overflow in NonceHolder.sol

File: system-contracts/contracts/NonceHolder.sol

 unchecked {
            rawNonces[addressAsKey] = (oldRawNonce + _value);
        }

It's important to notice that parameter _value is passed by the user. This means, that calculating oldRawNonce + _value in unchecked block is not safe - since this value may overflow the new nonce. The function is public, but has onlySystemCall modifier - thus the severity had been decreased to QA only. Nonetheless, performing calculations in unchecked blocks, where one of the argument is taken from the function's parameters is bad security practice and should be avoided.

The only protection from overflow is at line 66: require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high"); This requires, that _value cannot exceed MAXIMAL_MIN_NONCE_INCREMENT = 2 ** 32. However, this check does not stop from calling increaseMinNonce(MAXIMAL_MIN_NONCE_INCREMENT) multiple of times, until it will actually overflow. It's recommended to add a check for that overflow.

[QA-2] Missing checks for address(0) when updating address state variables

This instance was not detected during bot-race File: ethereum/contracts/governance/Governance.sol

 function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {
        emit ChangeSecurityCouncil(securityCouncil, _newSecurityCouncil);
        securityCouncil = _newSecurityCouncil;
    }

There's no verification for address(0), thus securityCouncil can be set to address(0).

[QA-3] Consider adding address verifyingContract to EIP721_DOMAIN_TYPEHASH in TransactionHelper.sol

According to EIP-712 documentation:

https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator

address verifyingContract the address of the contract that will verify the signature. The user-agent may do contract specific phishing prevention.

Having verifyingContract in EIP721_DOMAIN_TYPEHASH - is considered as a protection from phishing attacks.

File: system-contracts/contracts/libraries/TransactionHelper.sol

bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId)");

can be changed to:

bytes32 constant EIP712_DOMAIN_TYPEHASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

Changing EIP712_DOMAIN_TYPEHASH means that additional modification of code-base has to be done.

In most cases, verifyingContract is just address(this). This means, that below code:

File: system-contracts/contracts/libraries/TransactionHelper.so

        bytes32 domainSeparator = keccak256(
            abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256("zkSync"), keccak256("2"), block.chainid)
        );

needs to be changed to:

        bytes32 domainSeparator = keccak256(
            abi.encode(EIP712_DOMAIN_TYPEHASH, keccak256("zkSync"), keccak256("2"), block.chainid, address(this))
        );

[QA-4] Violation of EIP-712 in TransactionHelper.sol

According to EIP-712, the encoded EIP-712 hash of the struct must include every member field of that struct, however, in the current implementation, transaction.signature is missing:

File: system-contracts/contracts/libraries/TransactionHelper.sol

bytes32 structHash = keccak256(
            abi.encode(
                EIP712_TRANSACTION_TYPE_HASH,
                _transaction.txType,
                _transaction.from,
                _transaction.to,
                _transaction.gasLimit,
                _transaction.gasPerPubdataByteLimit,
                _transaction.maxFeePerGas,
                _transaction.maxPriorityFeePerGas,
                _transaction.paymaster,
                _transaction.nonce,
                _transaction.value,
                EfficientCall.keccak(_transaction.data),
                keccak256(abi.encodePacked(_transaction.factoryDeps)),
                EfficientCall.keccak(_transaction.paymasterInput)
            )
        );

[QA-5] Functions in Governance.sol do not check if new values are the same as current state

File: ethereum/contracts/governance/Governance.sol

    function updateDelay(uint256 _newDelay) external onlySelf {
        emit ChangeMinDelay(minDelay, _newDelay);
        minDelay = _newDelay;
    }

    /// @dev Updates the address of the security council.
    /// @param _newSecurityCouncil The address of the new security council.
    function updateSecurityCouncil(address _newSecurityCouncil) external onlySelf {
        emit ChangeSecurityCouncil(securityCouncil, _newSecurityCouncil);
        securityCouncil = _newSecurityCouncil;
    }

updateDelay() and updateSecurityCouncil() does not check if input parameter is the same as the current state of the variable which is being updated. If function is being called with the same value as the current state - even though there's no change - function will success and emit an event that the change has been made. Check _newDelay and _newSecurityCouncil is not equal to current state (minDelay and securityCouncil).

[QA-6] Incorrect type of blockGasLimit in SystemContext.sol

EVM defines gasLimit as uint64:

https://github.com/ethereum/go-ethereum/blob/c1d5a012ea4b824e902db14e47bf147d727c2657/core/types/tx_legacy.go#L30

Gas      uint64          // gas limit

However, the current implementation of zkEVM implements blockGasLimit as uint256:

File: system-contracts/contracts/SystemContext.sol#L37

    uint256 public blockGasLimit = type(uint32).max;

There shoudn't be any inconsistency between variable datatypes. The variable type should be the same as the EVM, so uint64.

[QA-7] L1ERC20Bridge won't work with rebasing tokens

There are some ERC-20 tokens which make arbitrary balance modifications outside of transfer. See this link for further reference.

The L1ERC20Bridge saves the amount which user deposited in depositAmount mapping. It is done, in case deposit failed on L2:

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

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

When deposit fails, user can call claimFailedDeposit() function, to withdraw funds from the initiated deposit, that failed when finalizing on L2. However, when token performs a rebase operation (token lowering its amount) before user calls claimFailedDeposit() - the user won't be able to claim that tokens.

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

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

To fix this issue, the most reliable way would be to add amount parameter to claimFailedDeposit() function, and let the user decide how many tokens he wants to withdraw. He should be able to withdraw amount which is the same or lower (in case of rebase) as the cached deposited amount in depositAmount.

[QA-8] getZkSyncMeta() from SystemContractHelper.sol does not include some fields from ZkSyncMeta

The ZkSyncMeta struct is defined as below:

File: system-contracts/contracts/libraries/SystemContractHelper.sol

struct ZkSyncMeta {
    uint32 gasPerPubdataByte;
    uint32 heapSize;
    uint32 auxHeapSize;
    uint8 shardId;
    uint8 callerShardId;
    uint8 codeShardId;
}

Function getZkSyncMeta() from SystemContractHelper.sol is defined as below:

File: system-contracts/contracts/libraries/SystemContractHelper.sol

    function getZkSyncMeta() internal view returns (ZkSyncMeta memory meta) {
        uint256 metaPacked = getZkSyncMetaBytes();
        meta.gasPerPubdataByte = getGasPerPubdataByteFromMeta(metaPacked);
        meta.shardId = getShardIdFromMeta(metaPacked);
        meta.callerShardId = getCallerShardIdFromMeta(metaPacked);
        meta.codeShardId = getCodeShardIdFromMeta(metaPacked);
    }

As it's demonstrated above, it does not include auxHeapSize and heapSize fields, which are defined in ZkSyncMeta. To fix this issue, add additional line to this function:

meta.heapSize = getHeapSizeFromMeta(metaPacked);
meta.auxHeapSize = getAuxHeapSizeFromMeta(metaPacked);

[QA-9] Lack of cleaning of variables before call in inline assembler

File: system-contracts/contracts/libraries/EfficientCall.sol

 assembly {
            // Clearing values before usage in assembly, since Solidity
            // doesn't do it by default
            _whoToMimic := and(_whoToMimic, cleanupMask)

Above code demonstrates how local variables should be cleaned before usage in inline assembly. However, there are multiple of lines in EfficientCall.sol where it's not done. All instances are related to _address variable.

EfficientCall.sol

135:            assembly {
136:                success := call(_address, callAddr, 0, 0, 0xFFFF, 0, 0)   

148:     assembly {
149:                    success := call(msgValueSimulator, callAddr, _value, _address, 0xFFFF, forwardMask, 0)

163:            assembly {
164:            success := staticcall(_address, callAddr, 0, 0xFFFF, 0, 0)

177:      assembly {
178:            success := delegatecall(_address, callAddr, 0, 0xFFFF, 0, 0)

208:                success := call(_address, callAddr, 0, 0, _whoToMimic, 0, 0)

[QA-10] Lack of 0-value check in NonceHolder.sol

File: system-contracts/contracts/NonceHolder.sol

    function increaseMinNonce(uint256 _value) public onlySystemCall returns (uint256 oldMinNonce) {
        require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high");

increaseMinNonce() does not verify if _value > 0. This is important check, otherwise, it would be possible to call increaseMinNonce(0) which actually won't increase the nonce.

[QA-11] processPaymasterInput() may revert for some ERC-20 tokens

There are several popular tokens (e.g. COMP, UNI). which revert when the value passed to approve() is greater than type(uint96).max. The contract won't be able to work with them. This issue affects the library. In my opinion, library functions should be implemented in a way which takes care of every edge-case. The number of supported tokens shoudn't be limited by the implementation of library function.

File: system-contracts/contracts/libraries/TransactionHelper.sol

 (address token, uint256 minAllowance) = abi.decode(_transaction.paymasterInput[4:68], (address, uint256));
            address paymaster = address(uint160(_transaction.paymaster));

            uint256 currentAllowance = IERC20(token).allowance(address(this), paymaster);
            if (currentAllowance < minAllowance) {
                // Some tokens, e.g. USDT require that the allowance is firsty set to zero
                // and only then updated to the new value.

                IERC20(token).safeApprove(paymaster, 0);
                IERC20(token).safeApprove(paymaster, minAllowance);
            }

(address token, uint256 minAllowance) = abi.decode(_transaction.paymasterInput[4:68], (address, uint256)); - this line suggests, that minAllowance is abi.decoded as uint256, thus its value can be greater than type(uint96).max.

If that's the case (minAllowance > type(uint96).max) and if token is a token which reverts when the value passed to approve() is greater than type(uint96).max, then: IERC20(token).safeApprove(paymaster, minAllowance); - this line will revert

[QA-12] Lack of address(0) check in L2ERC20Bridge means that finalizeDeposit() will mint tokens to address(0)

There's no check if _l2Receiver is address(0). This implies, that tokens will be minted to address(0). File: contracts/zksync/contracts/bridge/L2ERC20Bridge.so

IL2StandardToken(expectedL2Token).bridgeMint(_l2Receiver, _amount);

[QA-13] storedBatchHash() might return batch which was reverted by Executor's revertBatches()

Executor provides the ability to revertBatches():

File: code/contracts/ethereum/contracts/zksync/facets/Executor.sol

    function revertBatches(uint256 _newLastBatch) external nonReentrant onlyValidator {
        require(s.totalBatchesCommitted > _newLastBatch, "v1"); // The last committed batch is less than new last batch
        require(_newLastBatch >= s.totalBatchesExecuted, "v2"); // Already executed batches cannot be reverted

        if (_newLastBatch < s.totalBatchesVerified) {
            s.totalBatchesVerified = _newLastBatch;
        }
        s.totalBatchesCommitted = _newLastBatch;

        // Reset the batch number of the executed system contracts upgrade transaction if the batch
        // where the system contracts upgrade was committed is among the reverted batches.
        if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
            delete s.l2SystemContractsUpgradeBatchNumber;
        }

However, function storedBatchHash() does not take into consideration that batch might had been reverted:

File: contracts/ethereum/contracts/zksync/facets/Getters.sol

    function storedBatchHash(uint256 _batchNumber) external view returns (bytes32) {
        return s.storedBatchHashes[_batchNumber];
    }

This behavior may lead to confusion, since it gives the user (who called storedBatchHash() on reverted batch), the impression that that batch is still valid. There should be an additional condition in storedBatchHash(), which verifies:

if (_batchNumber > s.totalBatchesCommitted )

before returning the batch number.

[QA-14] DiamondProxy does not check if contract exists

File: ethereum/contracts/zksync/DiamondProxy.sol

  require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
        // Get facet from function selector
        Diamond.SelectorToFacet memory facet = diamondStorage.selectorToFacet[msg.sig];
        address facetAddress = facet.facetAddress;

        require(facetAddress != address(0), "F"); // Proxy has no facet for this selector
        require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen

Whenever proxy delegates to a address which is not contract, the fallback will return success, which will lead to a silent failure. In the current implementation, both faces and it's selectors are added in the diamondCut, thus having this scenario is not very realistic - thus I've degraded the severity to Low. Nonetheless, it's a good security practice to check for contract existance before delegatecall to that address. Our recommendation is to implement a check which verifies contract's code, before calling it.

[QA-15] Lack of input validation in AccountCodeStorage.sol may lead to overflow

File: system-contracts/contracts/AccountCodeStorage.so

function getCodeSize(uint256 _input) external view override returns (uint256 codeSize) {
(...)
        address account = address(uint160(_input));
        bytes32 codeHash = getRawCodeHash(account);

File: system-contracts/contracts/AccountCodeStorage.so

    function getCodeHash(uint256 _input) external view override returns (bytes32) {
(...)
        address account = address(uint160(_input));

Both functions will return incorrect values for inputs greater than the max value of uint160. E.g. type(uint160).max + X will return X. It's mandatory to enforce that _input is not greater than type(uint160).max.

[QA-16] Incorrect assumption may lead to revert in _burnMsgValue() in system-contracts/contracts/L2EthToken.sol

File: system-contracts/contracts/L2EthToken.sol

 /// NOTE: Since this contract holds the mapping of all ether balances of the system,
    /// the sent `msg.value` is added to the `this` balance before the call.
    /// So the balance of `address(this)` is always bigger or equal to the `msg.value`!
    function _burnMsgValue() internal returns (uint256 amount) {
        amount = msg.value;

        // Silent burning of the ether
        unchecked {
            // This is safe, since this contract holds the ether balances, and if user
            // send a `msg.value` it will be added to the contract (`this`) balance.
            balance[address(this)] -= amount;
            totalSupply -= amount;
        }
    }

Even though: "the sent msg.value is added to the this balance before the call", the totalSupply is not affected by amount. This means that above assumption is not true, because totalSupply may be smaller than amount, which will lead to revert.

[QA-17] The current gas price might be set to 0 in SystemContext

There's missing 0-value check when assigning value to a state variable. This implies that it's possible to set gas price to 0. Our recommendation is to implement additional require which won't let setting _gasPrice to 0: require(_gasPrice !=0, "gas price should be > 0").

File: system-contracts/contracts/SystemContext.sol

    function setGasPrice(uint256 _gasPrice) external onlyCallFromBootloader {
        gasPrice = _gasPrice;
    }

[QA-18] MsgValueSimulator ignores MAX_MSG_VALUE

The value is defined as uint256:

File: system-contracts/contracts/MsgValueSimulator.sol

 fallback(bytes calldata _data) external onlySystemCall returns (bytes memory) {
        (uint256 value, bool isSystemCall, address to) = _getAbiParams();

while the MAX_MSG_VALUE - as:

File: system-contracts/contracts/Constants.sol

uint256 constant MAX_MSG_VALUE = 2 ** 128 - 1;

This suggests that scenario where value > MAX_MSG_VALUE may occur (although, it's very unlikely, thus severity has been degraded to QA. The total supply of ETH is not that big). However, the implementation of fallback does not take into consideration that scenario. Our recommendation is to add additonal check in MsgValueSimulator.sol - it should revert when value is greater than MAX_MSG_VALUE.

The only mechanisms which enforces that value is less than 2**128-1 is line 55: SystemContractHelper.setValueForNextFarCall(Utils.safeCastToU128(value));, however, if protocol will be redeployed with any different value for MAX_MSG_VALUE - this line of code won't be sufficient. It's recommended to explicitly enforce value <= MAX_MSG_VALUE.

[N-1] In ContractDeployer.sol - ensure that new value is different than the current one

File: system-contracts/contracts/ContractDeployer.sol

 function updateAccountVersion(AccountAbstractionVersion _version) external onlySystemCall {
        accountInfo[msg.sender].supportedAAVersion = _version;

        emit AccountVersionUpdated(msg.sender, _version);
    }

There's no verification that _version is different than accountInfo[msg.sender].supportedAAVersion. Whenever there's a updateAccountVersion() call with _version which is already set, the event AccountVersionUpdated() will be emitted. This may be misleading (especially for critical operations, such as updating account version) - because, the version won't be changed (it still will be the same). Whenever calling updateAccountVersion(), make sure it updates accountInfo[msg.sender].supportedAAVersion with new version.

[N-2] Update name of updateNonceOrdering to better reflect its usage

Fiie: system-contracts/contracts/ContractDeployer.sol

    /// @notice Updates the nonce ordering of the account. Currently,
    /// it only allows changes from sequential to arbitrary ordering.
    /// @param _nonceOrdering The new nonce ordering to use.
    function updateNonceOrdering(AccountNonceOrdering _nonceOrdering) external onlySystemCall {

According to NatSpec and function implementation - it's possible to change the ordering from sequential to arbitrary only. This implies, that when ordering will be changed - there's no way to change it back. This behavior implies two suggestions about code refactoring:

• There's no need to have _nonceOrdering parameter, thus there's only one possible value for is - AccountNonceOrdering.Arbitrary
• The name of the function should be changed to reflect its intent, e.g. updateNonceOrderingToArbitrary()

[N-3] In NonceHolder.sol - function getRawNonce() can be used inside more functions

Function getRawNonce() is defined as below: File: system-contracts/contracts/NonceHolder.sol

    function getRawNonce(address _address) public view returns (uint256) {
        uint256 addressAsKey = uint256(uint160(_address));
        return rawNonces[addressAsKey];
    }

However, multiple of functions do not use getRawNonce(), e.g.:

 function increaseMinNonce(uint256 _value) public onlySystemCall returns (uint256 oldMinNonce) {
        require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high");

        uint256 addressAsKey = uint256(uint160(msg.sender));
        uint256 oldRawNonce = rawNonces[addressAsKey];

can be changed to:

 function increaseMinNonce(uint256 _value) public onlySystemCall returns (uint256 oldMinNonce) {
        require(_value <= MAXIMAL_MIN_NONCE_INCREMENT, "The value for incrementing the nonce is too high");

        uint256 oldRawNonce = getRawNonce(msg.sender);

Similarly:

function incrementMinNonceIfEquals(uint256 _expectedNonce) external onlySystemCall {
        uint256 addressAsKey = uint256(uint160(msg.sender));
        uint256 oldRawNonce = rawNonces[addressAsKey];

can be changed to:

function incrementMinNonceIfEquals(uint256 _expectedNonce) external onlySystemCall {
        uint256 oldRawNonce = getRawNonce(msg.sender);

[N-4] Unused code / code for local testing should be removed

File: ethereum/contracts/zksync/DiamondInit.sol

        // While this does not provide a protection in the production, it is needed for local testing
        // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
        assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32);

This code is used only for local testing - as the comment section states. Moreover, L2_TO_L1_LOG_SERIALIZE_SIZE is a constant variable - known before compiling time, thus checking its value is not needed at all. File ethereum/contracts/zksync/Config.sol defines that: uint256 constant L2_TO_L1_LOG_SERIALIZE_SIZE = 88; which implies that this assert is redundant.

[N-5] ContractDeployer.sol events do not emit parameters from old values

Whenever you change a state of some critical parameter, it's good practice to emit the event with old and new value. In ContractDeployer.sol, only the new values are being emitted.

This issue occurs in updateAccountVersion() and updateNonceOrdering():

File: system-contracts/contracts/ContractDeployer.sol

emit AccountVersionUpdated(msg.sender, _version);
(...)
emit AccountNonceOrderingUpdated(msg.sender, _nonceOrdering);

[N-6] NatSpec does not properly describes _setAllowList() in ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

File: ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

    /// @notice Change the address of the allow list smart contract
    /// @param _newAllowList Allow list smart contract address
    function _setAllowList(IAllowList _newAllowList) internal {
        if (_newAllowList == IAllowList(address(0))) {
            return;
        }

NatSpec misses the case, that _setAllowList returns nothing, when _newAllowList == IAllowList(address(0)). The behavior of this edge-case should be described in the NatSpec. Whenever there's a call to _setAllowList() wit address(0), function will not update Allow List - it will return silently.

[N-7] Redundant check in DefaultAccount.sol

File: system-contracts/contracts/DefaultAccount.so

        address recoveredAddress = ecrecover(_hash, v, r, s);

        return recoveredAddress == address(this) && recoveredAddress != address(0);

In function _isValidSignature(), when recoveredAddress == address(this) is true, then we are sure that recoveredAddress != address(0) is also true. This implies, that there's no need to perform the second check and code can be rewritten to: return recoveredAddress == address(this).

[N-8] incrementDeploymentNonce() from NonceHolder.sol doubles access checks:

File: system-contracts/contracts/NonceHolder.sol

    function incrementDeploymentNonce(address _address) external onlySystemCall returns (uint256 prevDeploymentNonce) {
        require(msg.sender == address(DEPLOYER_SYSTEM_CONTRACT), "");

There is no need to use onlySystemCall modifier, since function already enforces that msg.sender == address(DEPLOYER_SYSTEM_CONTRACT).

[N-9] In NonceHolder.sol - the order of getters and setters are mixed

It's a good coding practice to separate functions which set value, from functions which get values. In the NonceHolder.sol, there's lack of that separation:

    function getMinNonce(address _address) public view returns (uint256) {
    function getRawNonce(address _address) public view returns (uint256) {
    function increaseMinNonce(uint256 _value) public onlySystemCall returns (uint256 oldMinNonce) {
    function setValueUnderNonce(uint256 _key, uint256 _value) public onlySystemCall {
    function getValueUnderNonce(uint256 _key) public view returns (uint256) {
    function incrementMinNonceIfEquals(uint256 _expectedNonce) external onlySystemCall {
    function getDeploymentNonce(address _address) external view returns (uint256 deploymentNonce) {
    function incrementDeploymentNonce(address _address) external onlySystemCall returns (uint256 prevDeploymentNonce) {
    function isNonceUsed(address _address, uint256 _nonce) public view returns (bool) {
    function validateNonceUsage(address _address, uint256 _key, bool _shouldBeUsed) external view {
    function _splitRawNonce(uint256 _rawMinNonce) internal pure returns (uint256 deploymentNonce, uint256 minNonce) {

Reorganize the code, so that setters are being first, and below them - getters.

[N-10] Incorrect comment in SystemContext about baseFee

In bootloader.yul, we can check, that baseFee is being calculated dynamically:

File: system-contracts/bootloader/bootloader.yul

baseFee := max(
                    fairL2GasPrice,
                    ceilDiv(pubdataBytePriceETH, MAX_L2_GAS_PER_PUBDATA())
                )

However, the comment in SystemContext.sol states, it's a constant:

File: system-contracts/contracts/SystemContext.sol

    /// @dev It is currently a constant.
    uint256 public baseFee;

Fix this comment, since baseFee should not be treated as constant.

[N-11] The behavior of _markBytecodeAsPublished() may be misleading

File: system-contracts/contracts/KnownCodesStorage.sol

function _markBytecodeAsPublished(bytes32 _bytecodeHash, bool _shouldSendToL1) internal {
        if (getMarker(_bytecodeHash) == 0) {

The name of the function and its NatSpec suggests that it is used to mark a single bytecode hash as known. The current implementation shows, that this marking occurs only when (getMarker(_bytecodeHash) == 0). Otherwise, function does nothing. Based on that behavior, it's hard to distinguish if function marked a single bytecode hash as known, or not. The only way to check that, would be observing if that function emitted MarkedAsKnown event. However, there should be some additional (other than observing off-chain events) to distinguish if bytecode hash was marked or not. Our recommendation is to re-write a function in a way it will be returning bool result - true, when it marked a single bytecode hash as known; false - otherwise.

[N-12] Incorrect comment in TransactionHelper.sol

File: system-contracts/contracts/libraries/TransactionHelper.sol

/// @dev The type id of legacy transactions.
uint8 constant EIP_2930_TX_TYPE = 0x01;

Comment should be changed to: /// @dev The type id of EIP2930 transactions.

[N-13] Improper NatSpec for _isValidSignature() in DefaultAccount.sol

File: system-contracts/contracts/DefaultAccount.sol

/// @return EIP1271_SUCCESS_RETURN_VALUE if the signaure is correct. It reverts otherwise

Function can also return false. This case is not described in the NatSpec.

_isValidSignature() looks like this: return recoveredAddress == address(this) && recoveredAddress != address(0), which means, that above code in NatSpec should be changed to:

/// @return true for valid signature, false or reverts otherwise.

[N-14] Some functions define named returns, but still return value via return statement

File: system-contracts/contracts/NonceHolder.sol

    function getDeploymentNonce(address _address) external view returns (uint256 deploymentNonce) {
(...)
        return deploymentNonce;
    }

File: system-contracts/contracts/ContractDeployer.so

    /// @notice Returns information about a certain account.
    function getAccountInfo(address _address) external view returns (AccountInfo memory info) {
        return accountInfo[_address];

[N-15] Inconsistency in representing hex 0x00

Across the whole contract, the hex value of 0 is represented as 0x00. The only exception has been spotted in two instances:

File: system-contracts/contracts/ContractDeployer.sol

        require(_bytecodeHash != bytes32(0x0), "BytecodeHash cannot be zero");
(...)
            ACCOUNT_CODE_STORAGE_SYSTEM_CONTRACT.getCodeHash(uint256(uint160(_newAddress))) == 0x0,

File: system-contracts/contracts/libraries/TransactionHelper.sol

uint8 constant LEGACY_TX_TYPE = 0x0;

Stick to one way of representing hex numbers. In this case, 0x0 should be 0x00

[N-16] Misleading variable naming in SystemContext.sol

File: system-contracts/contracts/SystemContext.sol

        (uint128 previousBatchNumber, uint128 previousBatchTimestamp) = getBatchNumberAndTimestamp();
        require(_newTimestamp > previousBatchTimestamp, "Timestamps should be incremental");

Function getBatchNumberAndTimestamp() returns current batch number and current batch timestamp, thus naming those variables as previousBatchNumber and previousBatchTimestamp may be misleading. Even though, we're updating those variables with _newTimestamp, before the update, they are still current values, thus we should stick to proper naming convention.

[N-17] Constants in NonceHolder.sol does not have visibility set

File: code/system-contracts/contracts/NonceHolder.sol

    uint256 constant DEPLOY_NONCE_MULTIPLIER = 2 ** 128;
    /// The minNonce can be increased by at 2^32 at a time to prevent it from
    /// overflowing beyond 2**128.
    uint256 constant MAXIMAL_MIN_NONCE_INCREMENT = 2 ** 32;

It's bad coding practice to avoid explicitly defining visibility.

[N-18] Define constants, instead of using magic numbers:

File: system-contracts/contracts/libraries/Utils.sol

 require(bytecodeLenInWords < 2 ** 16, "pp"); // bytecode length must be less than 2^16 words

It will be more readable to define constant, instead of using 2 ** 16 value. The name of the constant will self-explain why bytecodeLenInWords must be less than 2**16

[N-19] Use ternary operators to make if/else statements more readable

Simple if/else construction can be shorten by using ternary operators

File: system-contracts/contracts/DefaultAccount.sol

if (_isValidSignature(txHash, _transaction.signature)) {
            magic = ACCOUNT_VALIDATION_SUCCESS_MAGIC;
        } else {
            magic = bytes4(0);
        }

can be changed to:

magic = _isValidSignature(txHash, _transaction.signature) ? ACCOUNT_VALIDATION_SUCCESS_MAGIC : bytes4(0)

[N-20] ForceDeployment struct can be moved from ConstractDeployer.sol

File: system-contracts/contracts/ContractDeployer.sol

struct ForceDeployment {

It's bad coding and design practice to have structs mixed with functions. Consider moving struct FordeDeployment to system-contracts/contracts/interfaces/IContractDeployer.sol

[N-21] Repeated checks can be implemented as modifiers

File: system-contracts/contracts/openzeppelin/utils/Address.sol

    function sendValue(address payable recipient, uint256 amount) internal {
        require(
            address(this).balance >= amount,
            "Address: insufficient balance"
        );

(...)
   function functionCallWithValue(
        address target,
        bytes memory data,
        uint256 value,
        string memory errorMessage
    ) internal returns (bytes memory) {
        require(
            address(this).balance >= value,
            "Address: insufficient balance for call"
        );

In the above code section, there are two functions: sendValue(), functionCallWithValue(), which performs basically the same check. This check can be rewritten as a modifier. For the reusability purpose - the repeated validation statements can be reimplemented as function modifiers. This will also improve the clarity of the code - since the same block of codes won't be used in multiple of functions.

[N-22] ReentrancyGuard.sol contains links to non-existing webpages

File: ethereum/contracts/common/ReentrancyGuard.sol

20:      * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul].

Make sure that links in the comment section redirects to valid, existing websites.

[N-23] Comments are not grammatically correct:

• File: zksync/contracts/ForceDeployUpgrader.sol

 /// @notice A function that performs force deploy

"force deploy" is not grammatically correct, it should be changed to "force deployment"

• File: zksync/contracts/bridge/L2Weth.sol

/// - It does not have a silent fallback method and will revert if it's called for a method it hasn't implemented.

"hasn't implemented", should be changed to "hasn't been implementing".

• File: system-contracts/contracts/interfaces/ISystemContext.sol

 /// @dev It will used for the L1 batch -> L2 block migration in Q3 2023 only.

"will used", should be changed to "will be used".

• File: system-contracts/contracts/libraries/TransactionHelper.sol

 // If the length is not equal to one, then only using the length can it be encoded definitely.

"length can it be encoded definitely" should be changed to "length, it can be encoded definitely"

• File: system-contracts/contracts/libraries/TransactionHelper.sol

 // If the length is not equal to one, then only using the length can it be encoded definitely.

"length can it be encoded definitely" should be changed to "length, it can be encoded definitely"

• File: system-contracts/contracts/libraries/TransactionHelper.sol

 // If the length is not equal to one, then only using the length can it be encoded definitely.

"length can it be encoded definitely" should be changed to "length, it can be encoded definitely"

• File: system-contracts/contracts/SystemContext.sol

 /// - Store the latest 257 blocks's hashes.

"blocks's" should be changed to "block's".

File: system-contracts/contracts/SystemContext.sol

// Due to virtual blocks upgrade, we'll have to use the following logic for retreiving the blockhash:

"retreiving" should be changed to "retriving".

• File: system-contracts/contracts/DefaultAccount.sol

// The fact there is are enough balance for the account

"there is are" should be changed to "there is"

• File: system-contracts/contracts/NonceHolder.sol

* The users can either marked a range of nonces by increasing the `minNonce`. This way all the nonces

"can either marked" should be changed to "can either mark".

[N-24] Messages in require are not descriptive

Make sure to use more descriptive messages in require statements. It will be helpful in debuging reverts. Moreover, it will increase a user experience - if his/her transaction reverts, he/she will see more descriptive reason of that revert.

This issue occurs in multiple of instances:

./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(_l2TokenBeacon != address(0), "nf");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(_governor != address(0), "nh");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(_factoryDeps.length == 3, "mk");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(_amount != 0, "2T");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(amount == _amount, "1T");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(proofValid, "yn");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(amount > 0, "y1");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(!isWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex], "pw");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:            require(success, "nq");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(_l2ToL1message.length == 76, "kk");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:        require(bytes4(functionSignature) == this.finalizeWithdrawal.selector, "nt");
./contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol:            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
./contracts/ethereum/contracts/bridge/L1WethBridge.sol:            require(success, "vq");
./contracts/ethereum/contracts/bridge/L1WethBridge.sol:        require(msg.sender == l1WethAddress || msg.sender == address(zkSync), "pn");
./contracts/ethereum/contracts/common/AllowList.sol:        require(targetsLength == _accessModes.length, "yg");
./contracts/ethereum/contracts/common/AllowList.sol:        require(callersLength == _targets.length, "yw");
./contracts/ethereum/contracts/common/AllowList.sol:        require(callersLength == _functionSigs.length, "yx");
./contracts/ethereum/contracts/common/AllowList.sol:        require(callersLength == _enables.length, "yy");
./contracts/ethereum/contracts/common/AllowListed.sol:            require(_allowList.canCall(msg.sender, address(this), msg.sig), "nr");
./contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol:        require(_bytecode.length % 32 == 0, "pq");
./contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol:        require(bytecodeLenInWords < 2**16, "pp");
./contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol:        require(bytecodeLenInWords % 2 == 1, "ps");
./contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol:        require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf");
./contracts/ethereum/contracts/common/libraries/L2ContractHelper.sol:        require(_bytecodeLen(_bytecodeHash) % 2 == 1, "uy");
./contracts/ethereum/contracts/common/ReentrancyGuard.sol:        require(lockSlotOldValue == 0, "1B");
./contracts/ethereum/contracts/common/ReentrancyGuard.sol:        require(_status == _NOT_ENTERED, "r1");
./contracts/ethereum/contracts/zksync/DiamondInit.sol:        require(address(_initalizeData.verifier) != address(0), "vt");
./contracts/ethereum/contracts/zksync/DiamondInit.sol:        require(_initalizeData.governor != address(0), "vy");
./contracts/ethereum/contracts/zksync/DiamondInit.sol:        require(_initalizeData.admin != address(0), "hc");
./contracts/ethereum/contracts/zksync/DiamondInit.sol:        require(_initalizeData.priorityTxMaxGasLimit <= L2_TX_MAX_GAS_LIMIT, "vu");
./contracts/ethereum/contracts/zksync/DiamondProxy.sol:        require(_chainId == block.chainid, "pr");
./contracts/ethereum/contracts/zksync/DiamondProxy.sol:        require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
./contracts/ethereum/contracts/zksync/DiamondProxy.sol:        require(!diamondStorage.isFrozen || !facet.isFreezable, "q1");
./contracts/ethereum/contracts/zksync/facets/Admin.sol:        require(msg.sender == pendingGovernor, "n4");
./contracts/ethereum/contracts/zksync/facets/Admin.sol:        require(msg.sender == pendingAdmin, "n4");
./contracts/ethereum/contracts/zksync/facets/Admin.sol:        require(_newPriorityTxMaxGasLimit <= L2_TX_MAX_GAS_LIMIT, "n5");
./contracts/ethereum/contracts/zksync/facets/Admin.sol:        require(!diamondStorage.isFrozen, "a9");
./contracts/ethereum/contracts/zksync/facets/Admin.sol:        require(diamondStorage.isFrozen, "a7");
./contracts/ethereum/contracts/zksync/facets/Base.sol:        require(msg.sender == s.governor, "1g");
./contracts/ethereum/contracts/zksync/facets/Base.sol:        require(s.validators[msg.sender], "1h");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(expectedNumberOfLayer1Txs == _newBatch.numberOfLayer1Txs, "ta");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(batchTimestamp == _expectedBatchTimestamp, "tb");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(_previousBatchTimestamp < batchTimestamp, "h3");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(block.timestamp - COMMIT_TIMESTAMP_NOT_OLDER <= batchTimestamp, "h1");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(lastL2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA, "h2");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:            require(!_checkBit(processedLogs, uint8(logKey)), "kp");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_TO_L1_MESSENGER_SYSTEM_CONTRACT_ADDR, "lm");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_TO_L1_MESSENGER_SYSTEM_CONTRACT_ADDR, "ln");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(providedL2ToL1PubdataHash == logValue, "wp");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_TO_L1_MESSENGER_SYSTEM_CONTRACT_ADDR, "lb");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_SYSTEM_CONTEXT_SYSTEM_CONTRACT_ADDR, "sc");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_SYSTEM_CONTEXT_SYSTEM_CONTRACT_ADDR, "sv");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_BOOTLOADER_ADDRESS, "bl");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_BOOTLOADER_ADDRESS, "bk");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(logSender == L2_BOOTLOADER_ADDRESS, "bu");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:                require(_expectedSystemContractUpgradeTxHash == logValue, "ut");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:            require(processedLogs == 127, "b7");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:            require(processedLogs == 255, "b8");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(s.l2SystemContractsUpgradeBatchNumber == 0, "ik");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(_hashStoredBatchInfo(_prevBatch) == s.storedBatchHashes[currentTotalBatchesVerified], "t1");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(s.totalBatchesCommitted > _newLastBatch, "v1");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(_newLastBatch >= s.totalBatchesExecuted, "v2");
./contracts/ethereum/contracts/zksync/facets/Executor.sol:        require(_batch.systemLogs.length <= MAX_L2_TO_L1_LOGS_COMMITMENT_BYTES, "pu");
./contracts/ethereum/contracts/zksync/facets/Getters.sol:        require(ds.selectorToFacet[_selector].facetAddress != address(0), "g2");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(callSuccess, "pz");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(_batchNumber <= s.totalBatchesExecuted, "xx");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(hashedLog != L2_L1_LOGS_TREE_DEFAULT_LEAF_HASH, "tw");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(!s.isEthWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex], "jj");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(proofValid, "pi");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(_l2GasPerPubdataByteLimit == REQUIRED_L2_GAS_PRICE_PER_PUBDATA, "qp");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(s.totalDepositedAmountPerUser[_depositor] + _amount <= limitData.depositCap, "d2");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "uj");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:            require(msg.value >= baseCost + _l2Value, "mv");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(_message.length >= 56, "pm");
./contracts/ethereum/contracts/zksync/facets/Mailbox.sol:        require(bytes4(functionSignature) == this.finalizeEthWithdrawal.selector, "is");
./contracts/ethereum/contracts/zksync/libraries/Diamond.sol:        require(_facet == address(0), "a1");
./contracts/ethereum/contracts/zksync/libraries/Diamond.sol:            require(oldFacet.facetAddress != address(0), "a2");
./contracts/ethereum/contracts/zksync/libraries/Diamond.sol:            require(_isSelectorFreezable == ds.selectorToFacet[selector0].isFreezable, "J1");
./contracts/ethereum/contracts/zksync/libraries/Diamond.sol:            require(data.length == 32, "lp");
./contracts/ethereum/contracts/zksync/libraries/Merkle.sol:        require(pathLength > 0, "xc");
./contracts/ethereum/contracts/zksync/libraries/Merkle.sol:        require(pathLength < 256, "bt");
./contracts/ethereum/contracts/zksync/libraries/Merkle.sol:        require(_index < (1 << pathLength), "px");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(l2GasForTxBody <= _priorityTxMaxGasLimit, "ui");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(l2GasForTxBody / _transaction.gasPerPubdataByteLimit <= PRIORITY_TX_MAX_PUBDATA, "uk");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.from <= type(uint16).max, "ua");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.to <= type(uint160).max, "ub");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.paymaster == 0, "uc");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.value == 0, "ud");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.reserved[0] == 0, "ue");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.reserved[1] <= type(uint160).max, "uf");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.reserved[2] == 0, "ug");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.reserved[3] == 0, "uo");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.signature.length == 0, "uh");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.paymasterInput.length == 0, "ul");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_transaction.reservedDynamic.length == 0, "um");
./contracts/ethereum/contracts/zksync/libraries/TransactionValidator.sol:        require(_totalGasLimit >= overhead, "my");
./contracts/ethereum/contracts/zksync/ValidatorTimelock.sol:        require(msg.sender == validator, "8h");
./contracts/ethereum/contracts/zksync/ValidatorTimelock.sol:                require(block.timestamp >= commitBatchTimestamp + delay, "5c");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:        require(_l1Bridge != address(0), "bf");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:        require(_l2TokenProxyBytecodeHash != bytes32(0), "df");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:        require(_governor != address(0), "sf");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:        require(AddressAliasHelper.undoL1ToL2Alias(msg.sender) == l1Bridge, "mq");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:            require(deployedToken == expectedL2Token, "mt");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:            require(currentL1Token == _l1Token, "gg");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:        require(l1Token != address(0), "yh");
./contracts/zksync/contracts/bridge/L2ERC20Bridge.sol:        require(success, "mk");
./contracts/zksync/contracts/bridge/L2WethBridge.sol:        require(msg.sender == l2WethAddress, "pd");
./system-contracts/contracts/libraries/Utils.sol:        require(_bytecode.length % 32 == 0, "po");
./system-contracts/contracts/libraries/Utils.sol:        require(bytecodeLenInWords < 2 ** 16, "pp");
./system-contracts/contracts/libraries/Utils.sol:        require(bytecodeLenInWords % 2 == 1, "pr");

We have an historical record of awarding non-evm equivalence that can cause damage as med, so I'm inclined to maintain the severity

(...)
Because the goal of the zkSync EVM is to be the EVM compatible, Medium Severity seems appropriate

(...)
I believe Medium Severity is most appropriate as the finding demonstrates an inconsistent behaviour of the EVM with the zkSyncVM

I agree with the impact, but believe that such finding belongs to the Medium Severity classification

Because the behaviour differs from the EVM, I believe that Medium Severity is appropriate

https://github.com/ethereum/go-ethereum/blob/c1d5a012ea4b824e902db14e47bf147d727c2657/core/types/tx_legacy.go#L30

Gas      uint64          // gas limit

    uint256 public blockGasLimit = type(uint32).max;

As stated in the scope - the Gas Report had been prepared for L1 contracts only.

[G-01] Pre-calculate keccak256 for constant variables in L2ContractHelper.sol

File: ethereum/contracts/common/libraries/L2ContractHelper.sol

bytes32 constant CREATE2_PREFIX = keccak256("zksyncCreate2");

[G-02] Length of _bytecode is calculated twice in L2ContractHelper.sol

Calculating length of bytes every time costs gas. The better solution would be to calculate the length once and save it in a local variable:

File: ethereum/contracts/common/libraries/L2ContractHelper.sol

        require(_bytecode.length % 32 == 0, "pq");

        uint256 bytecodeLenInWords = _bytecode.length / 32;

can be changed to:

		uint256 _bytecodeLength = _bytecode.length;
        require(_bytecodeLength % 32 == 0, "pq");

        uint256 bytecodeLenInWords = _bytecodeLength / 32;

[G-03] require in hashL2Bytecode in L2ContractHelper.sol can be moved on top

File: ethereum/contracts/common/libraries/L2ContractHelper.sol

        require(_bytecode.length % 32 == 0, "pq");

        uint256 bytecodeLenInWords = _bytecode.length / 32;
        require(bytecodeLenInWords < 2**16, "pp"); // bytecode length must be less than 2^16 words

Since _bytecode.length is divisible by 32 (the first require), we know, that we can divide _bytecode.length by 32, without a remainder.

Let's take a look at the 2nd require:

bytecodeLenInWords < 2**16 => _bytecode.length / 32 < 2**16 => _bytecode.length < 2**16 * 32 => _bytecode.length < 2**21

This implies that we can rewrite above code snippet as:

        require(_bytecode.length % 32 == 0, "pq");
        require(_bytecode.length < 2**21, "pp"); // bytecode length must be less than 2^16 words
        uint256 bytecodeLenInWords = _bytecode.length / 32;
        

That way, when _bytecode.length will be too big, we will revert immediately, instead of wasting gas on executing line: uint256 bytecodeLenInWords = _bytecode.length / 32. That line will be executed after the 2nd require.

Moreover, please keep in mind, to cache _bytecode.length into local variable - as it was suggested in G-02:

        uint256 _bytecodeLength = _bytecode.length;
        require(_bytecodeLength % 32 == 0, "pq");
        require(_bytecodeLength < 2**21, "pp"); // bytecode length must be less than 2^16 words
        uint256 bytecodeLenInWords = _bytecodeLength / 32;

[G-04] Unnecessary variables in L2ContractHelper.sol

• Function validateBytecodeHash

Variable version is used only once, which means, it doesn't need to be declared at all:

Below code snippet can be changed from:

File: ethereum/contracts/common/libraries/L2ContractHelper.sol

        uint8 version = uint8(_bytecodeHash[0]);
        require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); // Incorrectly formatted bytecodeHash

to:

require(uint8(_bytecodeHash[0]) == 1 && _bytecodeHash[1] == bytes1(0), "zf"); // Incorrectly formatted bytecodeHash

• Function computeCreate2Address

Variables senderBytes, data are used only once, which means, they don't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/common/libraries/L2ContractHelper.sol

        bytes32 senderBytes = bytes32(uint256(uint160(_sender)));
        bytes32 data = keccak256(
            bytes.concat(CREATE2_PREFIX, senderBytes, _salt, _bytecodeHash, _constructorInputHash)
        );

        return address(uint160(uint256(data)));

to:

        return address(uint160(uint256(keccak256(bytes.concat(CREATE2_PREFIX, bytes32(uint256(uint160(_sender))), _salt, _bytecodeHash, _constructorInputHash)))));

[G-05] Unnecessary variable in AllowList.sol

• Function _setPermissionToCall

Variable currentPermission is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/common/AllowList.sol

 bool currentPermission = hasSpecialAccessToCall[_caller][_target][_functionSig];

        if (currentPermission != _enable) {

to:

        if (hasSpecialAccessToCall[_caller][_target][_functionSig] != _enable) {

[G-06] Length of _factoryDeps is calculated multiple of times in _verifyFactoryDeps in BaseZkSyncUpgrade.sol.

Calculating length of array cost gas. Instead of doing it every time, calculate it once and assign that value to local variable.

_factoryDeps.length is being calculated:

• require(_factoryDeps.length == _expectedHashes.length, "Wrong number of factory deps"); in the 1st require
• require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "Factory deps can be at most 32"); in the 2nd require
• for (uint256 i = 0; i < _factoryDeps.length; ++i) { in loop (this one was reported in the bot-race)

File: ethereum/contracts/upgrades/BaseZkSyncUpgrade.sol

        require(_factoryDeps.length == _expectedHashes.length, "Wrong number of factory deps");
        require(_factoryDeps.length <= MAX_NEW_FACTORY_DEPS, "Factory deps can be at most 32");

        for (uint256 i = 0; i < _factoryDeps.length; ++i) {

[G-07] Unnecessary variable in BridgeInitializationHelper.sol

• Function requestDeployTransaction Variable deployCalldata is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/common/AllowList.sol

        bytes memory deployCalldata = abi.encodeCall(
            IL2ContractDeployer.create2,
            (bytes32(0), _bytecodeHash, _constructorData)
        );
        _zkSync.requestL2Transaction{value: _deployTransactionFee}(
            L2_DEPLOYER_SYSTEM_CONTRACT_ADDR,
            0,
            deployCalldata,
            DEPLOY_L2_BRIDGE_COUNTERPART_GAS_LIMIT,
            REQUIRED_L2_GAS_PRICE_PER_PUBDATA,
            _factoryDeps,
            msg.sender
        );

to:

        _zkSync.requestL2Transaction{value: _deployTransactionFee}(
            L2_DEPLOYER_SYSTEM_CONTRACT_ADDR,
            0,
            abi.encodeCall(
            IL2ContractDeployer.create2,
            (bytes32(0), _bytecodeHash, _constructorData)
        ),
            DEPLOY_L2_BRIDGE_COUNTERPART_GAS_LIMIT,
            REQUIRED_L2_GAS_PRICE_PER_PUBDATA,
            _factoryDeps,
            msg.sender
        );

[G-08] Unnecessary variables in L1WethBridge.sol

• Function initialize Variables l2WethBridgeImplementationBytecodeHash and l2WethBridgeProxyBytecodeHash are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/bridge/L1WethBridge.sol

        bytes32 l2WethBridgeImplementationBytecodeHash = L2ContractHelper.hashL2Bytecode(_factoryDeps[0]);
        bytes32 l2WethBridgeProxyBytecodeHash = L2ContractHelper.hashL2Bytecode(_factoryDeps[1]);

        // Deploy L2 bridge implementation contract
        address wethBridgeImplementationAddr = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeImplementationFee,
            l2WethBridgeImplementationBytecodeHash,
            "", // Empty constructor data
            _factoryDeps // All factory deps are needed for L2 bridge
        );

(...)

        // Deploy L2 bridge proxy contract
        l2Bridge = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeProxyFee,
            l2WethBridgeProxyBytecodeHash,
            l2WethBridgeProxyConstructorData,
            // No factory deps are needed for L2 bridge proxy, because it is already passed in the previous step
            new bytes[](0)
        );

to:

 
        // Deploy L2 bridge implementation contract
        address wethBridgeImplementationAddr = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeImplementationFee,
            L2ContractHelper.hashL2Bytecode(_factoryDeps[0]);,
            "", // Empty constructor data
            _factoryDeps // All factory deps are needed for L2 bridge
        );

(...)
        // Deploy L2 bridge proxy contract
        l2Bridge = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeProxyFee,
            L2ContractHelper.hashL2Bytecode(_factoryDeps[1]),
            l2WethBridgeProxyConstructorData,
            // No factory deps are needed for L2 bridge proxy, because it is already passed in the previous step
            new bytes[](0)
        );

• Function finalizeWithdrawal Variables alreadyFinalised, l2ToL1Message and success are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/bridge/L1WethBridge.sol

bool alreadyFinalised = zkSync.isEthWithdrawalFinalized(_l2BatchNumber, _l2MessageIndex);
        if (alreadyFinalised) {
            // Check that the specified message was actually sent while withdrawing eth from L2.
            L2Message memory l2ToL1Message = L2Message({
                txNumberInBatch: _l2TxNumberInBatch,
                sender: L2_ETH_TOKEN_SYSTEM_CONTRACT_ADDR,
                data: _message
            });
            bool success = zkSync.proveL2MessageInclusion(_l2BatchNumber, _l2MessageIndex, l2ToL1Message, _merkleProof);
            require(success, "vq");

to:

        if (zkSync.isEthWithdrawalFinalized(_l2BatchNumber, _l2MessageIndex)) {
            require(zkSync.proveL2MessageInclusion(_l2BatchNumber, _l2MessageIndex, L2Message({
                txNumberInBatch: _l2TxNumberInBatch,
                sender: L2_ETH_TOKEN_SYSTEM_CONTRACT_ADDR,
                data: _message
            }), _merkleProof), "vq");

[G-09] Check if amount is greater than zero before performing safeTransfer in L1WethBridge.sol

File: ethereum/contracts/bridge/L1WethBridge.sol

        IERC20(l1WethAddress).safeTransfer(l1WethWithdrawReceiver, amount);

safeTransfer'ing when amount = 0 is a waste of gas, since there will be no funds transferred. To save gas, check if amount is greater than 0, before calling safeTransfer.

[G-10] Do not assign offset in the last call of UnsafeBytes.readAddress in L1WethBridge.sol

A simple test in Remix IDE has been created, to compare gas usage of assigning more than one value from function call:

  function getUints() public pure returns (uint, uint) {return (1, 2);}
   function getAll() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, b) = getUints();
    console.log(g - gasleft());
   }
    function getOne() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, ) = getUints();
    console.log(g - gasleft());
   }

getOne() costs 74 gas, while getAll() costs 82 gas. This implies, that we shouldn't assign every parameter from function call if it's not needed.

In the last call of UnsafeBytes.readAddress(_message, offset) (line 298):

File: ethereum/contracts/bridge/L1WethBridge.sol

(l1WethReceiver, offset) = UnsafeBytes.readAddress(_message, offset);

we won't need offset anymore, thus we can change line 298 to: (l1WethReceiver, ) = UnsafeBytes.readAddress(_message, offset).

[G-11] Unnecessary variables in L1ERC20Bridge.sol

• Function initialize

Variables l2WethBridgeImplementationBytecodeHash and l2WethBridgeProxyBytecodeHash are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

        bytes32 l2BridgeImplementationBytecodeHash = L2ContractHelper.hashL2Bytecode(_factoryDeps[0]);
        bytes32 l2BridgeProxyBytecodeHash = L2ContractHelper.hashL2Bytecode(_factoryDeps[1]);

        // Deploy L2 bridge implementation contract
        address bridgeImplementationAddr = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeImplementationFee,
            l2BridgeImplementationBytecodeHash,
            "", // Empty constructor data
            _factoryDeps // All factory deps are needed for L2 bridge
        );

(...)

        // Deploy L2 bridge proxy contract
        l2Bridge = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeProxyFee,
            l2BridgeProxyBytecodeHash,
            l2BridgeProxyConstructorData,
            // No factory deps are needed for L2 bridge proxy, because it is already passed in previous step
            new bytes[](0)
        );
    

to:

        // Deploy L2 bridge implementation contract
        address bridgeImplementationAddr = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeImplementationFee,
            L2ContractHelper.hashL2Bytecode(_factoryDeps[0]),
            "", // Empty constructor data
            _factoryDeps // All factory deps are needed for L2 bridge
        );

(...)

        // Deploy L2 bridge proxy contract
        l2Bridge = BridgeInitializationHelper.requestDeployTransaction(
            zkSync,
            _deployBridgeProxyFee,
            L2ContractHelper.hashL2Bytecode(_factoryDeps[1]),
            l2BridgeProxyConstructorData,
            // No factory deps are needed for L2 bridge proxy, because it is already passed in previous step
            new bytes[](0)
        );

• Function _depositFunds

Variable balanceAfter is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/bridge/L1ERC20Bridge.sol

        uint256 balanceAfter = _token.balanceOf(address(this));

        return balanceAfter - balanceBefore;

to:

return _token.balanceOf(address(this)) - balanceBefore;

• Function claimFailedDeposit Variable proofValid is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/bridge/L1ERC20Bridge.sol

 bool proofValid = zkSync.proveL1ToL2TransactionStatus(
            _l2TxHash,
            _l2BatchNumber,
            _l2MessageIndex,
            _l2TxNumberInBatch,
            _merkleProof,
            TxStatus.Failure
        );
        require(proofValid, "yn");

to:

        require(zkSync.proveL1ToL2TransactionStatus(
            _l2TxHash,
            _l2BatchNumber,
            _l2MessageIndex,
            _l2TxNumberInBatch,
            _merkleProof,
            TxStatus.Failure
        ), "yn");

• Function finalizeWithdrawal Variable success is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/bridge/L1ERC20Bridge.sol

            bool success = zkSync.proveL2MessageInclusion(_l2BatchNumber, _l2MessageIndex, l2ToL1Message, _merkleProof);
            require(success, "nq");

to:

            require(zkSync.proveL2MessageInclusion(_l2BatchNumber, _l2MessageIndex, l2ToL1Message, _merkleProof), "nq");

• Function l2TokenAddress

Variables constructorInputHash and salt are used only once, which means they don't need to be declared at all. Below code snippet can be changed from: File: ethereum/contracts/bridge/L1ERC20Bridge.sol

        bytes32 constructorInputHash = keccak256(abi.encode(address(l2TokenBeacon), ""));
        bytes32 salt = bytes32(uint256(uint160(_l1Token)));

        return L2ContractHelper.computeCreate2Address(l2Bridge, salt, l2TokenProxyBytecodeHash, constructorInputHash);

to:

        return L2ContractHelper.computeCreate2Address(l2Bridge, bytes32(uint256(uint160(_l1Token))), l2TokenProxyBytecodeHash, keccak256(abi.encode(address(l2TokenBeacon), "")));

[G-12] Calculation which won't underflow can be unchecked in L1ERC20Bridge.sol

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

return balanceAfter - balanceBefore;

Since balanceAfter contains token's balance after the transfer, and balanceBefore contains token's balance before the transfer, we know, that balanceAfter >= balanceBefore, thus balanceAfter - balanceBefore will never underflow and can be unchecked.

[G-13] Check if amount is greater than zero before performing safeTransfer in L1ERC20Bridge.sol

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

        IERC20(l1Token).safeTransfer(l1Receiver, amount);

safeTransfer'ing when amount = 0 is a waste of gas, since there will be no funds transferred. To save gas, check if amount is greater than 0, before calling safeTransfer.

[G-14] Do not assign offset in the last call of UnsafeBytes.readUint256 in L1ERC20Bridge.sol

A simple test in Remix IDE has been created, to compare gas usage of assigning more than one value from function call:

  function getUints() public pure returns (uint, uint) {return (1, 2);}
   function getAll() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, b) = getUints();
    console.log(g - gasleft());
   }
    function getOne() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, ) = getUints();
    console.log(g - gasleft());
   }

getOne() costs 74 gas, while getAll() costs 82 gas. This implies, that we shouldn't assign every parameter from function call if it's not needed.

In the last call of UnsafeBytes.readUint256(_l2ToL1message, offset) (line 336):

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

(amount, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset);

we won't need offset anymore, thus we can change line 336 to: (amount, ) = UnsafeBytes.readUint256(_l2ToL1message, offset);.

[G-15] One + operation is redundant in _verifyDepositLimit in L1ERC20Bridge.sol

File: ethereum/contracts/bridge/L1ERC20Bridge.sol

 } else {
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }

We don't need to add amount twice - firstly - to check require statement, and secondly - to increase totalDepositedAmountPerUser[_l1Token][_depositor]. We can change the order of require to do so:

 } else {
            
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
            require(totalDepositedAmountPerUser[_l1Token][_depositor]  <= limitData.depositCap, "d1");
        }

If totalDepositedAmountPerUser[_l1Token][_depositor] + _amount exceeds limitData.depositCap, we will still revert.

To prove, that this will indeed save some gas, a minimalized code-snippet in Remix IDE has been prepared:

mapping(address => mapping(address => uint256)) public totalDepositedAmountPerUser;
function current(uint _amount) public {
    uint g = gasleft();
    require(totalDepositedAmountPerUser[msg.sender][msg.sender] + _amount <= 1000, "d1");
    totalDepositedAmountPerUser[msg.sender][msg.sender] += _amount;
    console.log(g - gasleft());
}

function afterFix(uint _amount) public {
    uint g = gasleft();
    totalDepositedAmountPerUser[msg.sender][msg.sender] += _amount;
    require(totalDepositedAmountPerUser[msg.sender][msg.sender]  <= 1000, "d1");
    console.log(g - gasleft());
}

Function current() uses 5895 gas, while function afterFix() uses 5707, which implies, that proposed fix is better optimalized for a gas usage.

[G-16] Unnecessary variables in LibMap.sol

• Function get

Variables mapValue and bitOffset are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/zksync/libraries/LibMap.sol

uint256 mapValue = _map.map[_index / 8];

// First three bits of the original `_index` denotes the position of the uint32 in that slot.
// So, '(_index & 7) * 32' is done to find the bit position of the uint32 in that storage slot.
uint256 bitOffset = (_index & 7) * 32;

// Shift the bits to the right and retrieve the uint32 value.
result = uint32(mapValue >> bitOffset);

to:

result = uint32(_map.map[_index / 8] >> ( (_index & 7) * 32));

• Function set

Variables oldValue and nexValueXorOldValue are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from:

File: ethereum/contracts/zksync/libraries/LibMap.sol

            uint32 oldValue = uint32(mapValue >> bitOffset);

            // Calculate the XOR of the new value and the existing value.
            uint256 newValueXorOldValue = uint256(oldValue ^ _value);

            // Finally, we XOR the slot with the XOR of the new value and the existing value,
            // shifted to its proper position. The XOR operation will effectively replace the old value with the new value.
            _map.map[mapIndex] = (newValueXorOldValue << bitOffset) ^ mapValue;

to:

            _map.map[mapIndex] = (uint256(uint32(mapValue >> bitOffset) ^ _value) << bitOffset) ^ mapValue;

[G-17] Division by powers of two should use bit shifting in Merkle.sol

While bot race reported every division by powers of two in a form: A / 2, it misses an instance, which looks like: A /= 2.

File: ethereum/contracts/zksync/libraries/Merkle.sol

_index /= 2;

It should be changed to: _index = _index >> 1;

[G-18] Precalculate constants in Config.sol.

While constants have known value, to save more gas, calculate their values before compile-time.

File: ethereum/contracts/zksync/Config.sol

uint256 constant MAX_L2_TO_L1_LOGS_COMMITMENT_BYTES = 4 + L2_TO_L1_LOG_SERIALIZE_SIZE * 512
uint256 constant MAX_INITIAL_STORAGE_CHANGES_COMMITMENT_BYTES = 4 + INITIAL_STORAGE_CHANGE_SERIALIZE_SIZE * 4765;
uint256 constant MAX_REPEATED_STORAGE_CHANGES_COMMITMENT_BYTES = 4 + REPEATED_STORAGE_CHANGE_SERIALIZE_SIZE * 7564;

[G-19] Function pushBack in PriorityQueue.sol can be optimized

File: ethereum/contracts/zksync/libraries/PriorityQueue.sol

    function pushBack(Queue storage _queue, PriorityOperation memory _operation) internal {
        // Save value into the stack to avoid double reading from the storage
        uint256 tail = _queue.tail;

        _queue.data[tail] = _operation;
        _queue.tail = tail + 1;
    }

Above function can be optimized by utilizing post-incrementing feature. It will allow to remove tail variable. Function won't be creating and reading from that variable:

    function pushBack(Queue storage _queue, PriorityOperation memory _operation) internal {
        // Save value into the stack to avoid double reading from the storage
        _queue.data[_queue.tail++] = _operation;
        
    }

[G-20] Remove local-testing related code in DiamondInit.sol

File: ethereum/contracts/zksync/DiamondInit.sol

        // While this does not provide a protection in the production, it is needed for local testing
        // Length of the L2Log encoding should not be equal to the length of other L2Logs' tree nodes preimages
        assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32);

According to above code, assert(L2_TO_L1_LOG_SERIALIZE_SIZE != 2 * 32); is redundant and can be removed in production, since this line of code is dedicated for local testing only. This can be confirmed in file ethereum/contracts/zksync/Config.sol, where constant L2_TO_L1_LOG_SERIALIZE_SIZE is defined as 88. Since this is a constant value, its value will never change, thus this assert is unnecessary and can be removed.

[G-21] Length of _encoded is calculated twice in validateL1ToL2Transaction in TransactionValidator.sol.

Calculating length is gas-costly operation. It's better to calculate it once and store it in local variable.

File: ethereum/contracts/zksync/libraries/TransactionValidator.sol

        uint256 l2GasForTxBody = getTransactionBodyGasLimit(
            _transaction.gasLimit,
            _transaction.gasPerPubdataByteLimit,
            _encoded.length
        );

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

_encoded.length is being calculated twice - in line 26 and line 38.

[G-22] Use constants instead of type(uintX).max in TransactionValidator.sol

When you use type(uintX).max - it may result in higher gas costs because it involves a runtime operation to calculate the type(uintX).max at runtime. This calculation is performed every time the expression is evaluated.

To save gas, it is recommended to use constants to represent the maximum value. Declaration of a constant with the maximum value means that the value is known at compile-time and does not require any runtime calculations.

File: ethereum/contracts/zksync/libraries/TransactionValidator.sol

        require(_transaction.from <= type(uint16).max, "ua");
        require(_transaction.to <= type(uint160).max, "ub");
        require(_transaction.paymaster == 0, "uc");
        require(_transaction.value == 0, "ud");
        require(_transaction.reserved[0] == 0, "ue");
        require(_transaction.reserved[1] <= type(uint160).max, "uf");

[G-23] Multiple adding operations can be reduced in getMinimalPriorityTransactionGasLimit in TransactionValidator.sol

File: ethereum/contracts/zksync/libraries/TransactionValidator.sol

                costForComputation = L1_TX_INTRINSIC_L2_GAS;
(...)
            costForComputation += Math.ceilDiv(_encodingLength * L1_TX_DELTA_544_ENCODING_BYTES, 544);
(...)
            costForComputation += _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_L2_GAS;
(...)
            costForComputation = Math.max(costForComputation, L1_TX_MIN_L2_GAS_BASE);
(...)
            costForPubdata = L1_TX_INTRINSIC_PUBDATA * _l2GasPricePerPubdata;
(...)
            costForPubdata += _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_PUBDATA * _l2GasPricePerPubdata;

There's no need to perform multiple costForComputation +=, when it can be calculated at once:

costForComputation = L1_TX_INTRINSIC_L2_GAS +  Math.ceilDiv(_encodingLength * L1_TX_DELTA_544_ENCODING_BYTES, 544) + _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_L2_GAS;
costForComputation = Math.max(costForComputation, L1_TX_MIN_L2_GAS_BASE);

The whole function can be rewritten to:

  function getMinimalPriorityTransactionGasLimit(
        uint256 _encodingLength,
        uint256 _numberOfFactoryDependencies,
        uint256 _l2GasPricePerPubdata
    ) internal pure returns (uint256) {
        return Math.max(L1_TX_INTRINSIC_L2_GAS + Math.ceilDiv(_encodingLength * L1_TX_DELTA_544_ENCODING_BYTES, 544) + _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_L2_GAS, L1_TX_MIN_L2_GAS_BASE) + (L1_TX_INTRINSIC_PUBDATA * _l2GasPricePerPubdata + _numberOfFactoryDependencies * L1_TX_DELTA_FACTORY_DEPS_PUBDATA * _l2GasPricePerPubdat);
    }

[G-24] Unnecessary variables in TransactionValidator.sol

• Function getOverheadForTransaction

Variables txSlotOverhead, overheadForLength, numerator and denominator are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/libraries/TransactionValidator.sol

uint256 txSlotOverhead = Math.ceilDiv(batchOverheadGas, MAX_TRANSACTIONS_IN_BATCH);
batchOverheadForTransaction = Math.max(batchOverheadForTransaction, txSlotOverhead);

// The overhead for occupying the bootloader memory can be derived from encoded_len
uint256 overheadForLength = Math.ceilDiv(_encodingLength * batchOverheadGas, BOOTLOADER_TX_ENCODING_SPACE);
batchOverheadForTransaction = Math.max(batchOverheadForTransaction, overheadForLength);

(...)

uint256 numerator = batchOverheadGas * _totalGasLimit + L2_TX_MAX_GAS_LIMIT;
uint256 denominator = L2_TX_MAX_GAS_LIMIT + batchOverheadGas;

overheadForGas = (numerator - 1) / denominator;

to:

batchOverheadForTransaction = Math.max(batchOverheadForTransaction, Math.ceilDiv(batchOverheadGas, MAX_TRANSACTIONS_IN_BATCH));

// The overhead for occupying the bootloader memory can be derived from encoded_len

batchOverheadForTransaction = Math.max(batchOverheadForTransaction, Math.ceilDiv(_encodingLength * batchOverheadGas, BOOTLOADER_TX_ENCODING_SPACE));

(...)

overheadForGas = (batchOverheadGas * _totalGasLimit + L2_TX_MAX_GAS_LIMIT - 1) / (L2_TX_MAX_GAS_LIMIT + batchOverheadGa);

[G-25] Unnecessary variables in Getters.sol

• Function isFacetFreezable Variables selectorsArrayLen and selector0 are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/facets/Getters.sol

 uint256 selectorsArrayLen = ds.facetToSelectors[_facet].selectors.length;
        if (selectorsArrayLen != 0) {
            bytes4 selector0 = ds.facetToSelectors[_facet].selectors[0];
            isFreezable = ds.selectorToFacet[selector0].isFreezable;

to:

        if (ds.facetToSelectors[_facet].selectors.length != 0) {
            isFreezable = ds.selectorToFacet[ds.facetToSelectors[_facet].selectors[0]].isFreezable;

[G-26] Unnecessary variables in Diamond.sol

• Function diamondCut

Variables facet and isFacetFreezable are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/libraries/Diamond.sol

 Action action = facetCuts[i].action;
            address facet = facetCuts[i].facet;
            bool isFacetFreezable = facetCuts[i].isFreezable;
            bytes4[] memory selectors = facetCuts[i].selectors;

            require(selectors.length > 0, "B"); // no functions for diamond cut

            if (action == Action.Add) {
                _addFunctions(facet, selectors, isFacetFreezable);
            } else if (action == Action.Replace) {
                _replaceFunctions(facet, selectors, isFacetFreezable);
            } else if (action == Action.Remove) {
                _removeFunctions(facet, selectors);

to:

 Action action = facetCuts[i].action;
            bytes4[] memory selectors = facetCuts[i].selectors;

            require(selectors.length > 0, "B"); // no functions for diamond cut

            if (action == Action.Add) {
                _addFunctions(facetCuts[i].facet, selectors, facetCuts[i].isFreezable);
            } else if (action == Action.Replace) {
                _replaceFunctions(facetCuts[i].facet, selectors, facetCuts[i].isFreezable);
            } else if (action == Action.Remove) {
                _removeFunctions(facetCuts[i].facet, selectors);

• Function _saveFacetIfNew Variable selectorsLength is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/libraries/Diamond.sol

uint256 selectorsLength = ds.facetToSelectors[_facet].selectors.length;
        // If there are no selectors associated with facet then save facet as new one
        if (selectorsLength == 0) {

to:

if (ds.facetToSelectors[_facet].selectors.length == 0) {

• Function _addOneFunction Variable selector0 is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/libraries/Diamond.sol

 bytes4 selector0 = ds.facetToSelectors[_facet].selectors[0];
            require(_isSelectorFreezable == ds.selectorToFacet[selector0].isFreezable, "J1");

to:

            require(_isSelectorFreezable == ds.selectorToFacet[ds.facetToSelectors[_facet].selectors[0]].isFreezable, "J1");

[G-27] Function diamondCut() from Diamond.sol can be redesigned to revert earlier

File: ethereum/contracts/zksync/libraries/Diamond.sol

101:    Action action = facetCuts[i].action;
102:    address facet = facetCuts[i].facet;
103:    bool isFacetFreezable = facetCuts[i].isFreezable;
104:    bytes4[] memory selectors = facetCuts[i].selectors;
105:
106:    require(selectors.length > 0, "B"); // no functions for diamond cut

You can check selectors' length ealier. If their length is 0, then function will revert without wasting gas on executing lines: 101-103. Change above code to:

bytes4[] memory selectors = facetCuts[i].selectors;
require(selectors.length > 0, "B"); // no functions for diamond cut
Action action = facetCuts[i].action;
address facet = facetCuts[i].facet;
bool isFacetFreezable = facetCuts[i].isFreezable;

[G-28] In Diamond.sol, move some require on top of function

Functions: _addFunctions, _replaceFunctions, removeFunctions, firstly gets the pointer to the diamond storage: DiamondStorage storage ds = getDiamondStorage(); and then, they perform a check, if user passed a proper _facet address. Those lines can, however, be swapped. If user provides incorrect _facet w then function should immediately revert. Afterwards, it should get the pointer to the diamond storage.

In _addFunctions() change:

DiamondStorage storage ds = getDiamondStorage();
require(_facet != address(0), "G"); // facet with zero address cannot be added

to:

require(_facet != address(0), "G"); // facet with zero address cannot be added
DiamondStorage storage ds = getDiamondStorage();

In _replaceFunctions change:

DiamondStorage storage ds = getDiamondStorage();
require(_facet != address(0), "K"); // cannot replace facet with zero address

to:

require(_facet != address(0), "K"); // cannot replace facet with zero address
DiamondStorage storage ds = getDiamondStorage();

In _removeFunctions change:

DiamondStorage storage ds = getDiamondStorage();
require(_facet == address(0), "a1"); // facet address must be zero

to:

require(_facet == address(0), "a1"); // facet address must be zero
DiamondStorage storage ds = getDiamondStorage();

[G-29] In Diamond.sol in _initializeDiamondCut(), order of require can be changed

File: ethereum/contracts/zksync/libraries/Diamond.sol

require(data.length == 32, "lp");
require(abi.decode(data, (bytes32)) == DIAMOND_INIT_SUCCESS_RETURN_VALUE, "lp1");

The order of require can be changed, firstly check if abi.decode is DIAMOND_INIT_SUCCESS_RETURN_VALUE, then check its length:

require(abi.decode(data, (bytes32)) == DIAMOND_INIT_SUCCESS_RETURN_VALUE, "lp1");
require(data.length == 32, "lp");

It will save some gas in multiple of scenario:

• when data.length < 32 - decoding will revert and there will be no need to execute require(data.length == 32, "lp");
• when data.length > 32, but first 32 bytes of data are not DIAMOND_INIT_SUCCESS_RETURN_VALUE - comparison will revert and there will be no need to execute require(data.length == 32, "lp");

[G-30] Unnecessary variables in Mailbox.sol

• Function _proveL2LogInclusion

Variables calculatedRootHash and actualRootHash are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/facets/Mailbox.sol

bytes32 calculatedRootHash = Merkle.calculateRoot(_proof, _index, hashedLog);
        bytes32 actualRootHash = s.l2LogsRootHashes[_batchNumber];

        return actualRootHash == calculatedRootHash;

to:

return s.l2LogsRootHashes[_batchNumber] == Merkle.calculateRoot(_proof, _index, hashedLog);

• Function l2TransactionBaseCost Variable l2GasPrice is used only once, which means it doesn't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/facets/Mailbox.sol

        uint256 l2GasPrice = _deriveL2GasPrice(_gasPrice, _l2GasPerPubdataByteLimit);
        return l2GasPrice * _l2GasLimit;

to:

return _deriveL2GasPrice(_gasPrice, _l2GasPerPubdataByteLimit) * _l2GasLimit;

• Function _deriveL2GasPrice

Variables pubdataPriceETH and minL2GasPriceETH are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/facets/Mailbox.sol

        uint256 pubdataPriceETH = L1_GAS_PER_PUBDATA_BYTE * _l1GasPrice;
        uint256 minL2GasPriceETH = (pubdataPriceETH + _gasPricePerPubdata - 1) / _gasPricePerPubdata;

        return Math.max(FAIR_L2_GAS_PRICE, minL2GasPriceETH);

to:

        return Math.max(FAIR_L2_GAS_PRICE, (L1_GAS_PER_PUBDATA_BYTE * _l1GasPrice + _gasPricePerPubdata - 1) / _gasPricePerPubdata);

• Function _requestL2Transaction

Variables expirationTimestamp, txId, baseCost are used only once, which means they don't need to be declared at all.

Below code snippet can be changed from: File: ethereum/contracts/zksync/facets/Mailbox.sol

uint64 expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION); // Safe to cast
uint256 txId = s.priorityQueue.getTotalPriorityTxs();

(...)

uint256 baseCost = params.l2GasPrice * _l2GasLimit;
require(msg.value >= baseCost + _l2Value, "mv"); // The `msg.value` doesn't cover the transaction cost

(...)

params.sender = _sender;
params.txId = txId;
params.l2Value = _l2Value;
params.contractAddressL2 = _contractAddressL2;
params.expirationTimestamp = expirationTimestamp;
params.l2GasLimit = _l2GasLimit;
params.l2GasPricePerPubdata = _l2GasPerPubdataByteLimit;
params.valueToMint = msg.value;
params.refundRecipient = refundRecipient;

to:

require(msg.value >= params.l2GasPrice * _l2GasLimit + _l2Value, "mv"); // The `msg.value` doesn't cover the transaction cost

(...)

params.sender = _sender;
params.txId = s.priorityQueue.getTotalPriorityTxs();
params.l2Value = _l2Value;
params.contractAddressL2 = _contractAddressL2;
params.expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION);
params.l2GasLimit = _l2GasLimit;
params.l2GasPricePerPubdata = _l2GasPerPubdataByteLimit;
params.valueToMint = msg.value;
params.refundRecipient = refundRecipient;

[G-31] Reduce number of additions and readings of s.totalDepositedAmountPerUser[_depositor] in _verifyDepositLimit() in Mailbox.sol

File: ethereum/contracts/zksync/facets/Mailbox.sol

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

There's no need to access s.totalDepositedAmountPerUser[_depositor] twice and perform addition two times. Much cheaper solution would be to cache s.totalDepositedAmountPerUser[_depositor] + _amount. Code can be rewritten to:

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

we've cached s.totalDepositedAmountPerUser[_depositor] + _amount, and then later checked if it does not exceed limit. If it does, function will revert.

[G-32] Operations which won't overflow in Mailbox.sol can be unchecked

If, for any reason recommendation for G-31 won't be implemented, another gas optimization would be insert line 280: s.totalDepositedAmountPerUser[_depositor] += _amount; into unchecked block. Since require(s.totalDepositedAmountPerUser[_depositor] + _amount <= limitData.depositCap, "d2"); guarantees, that s.totalDepositedAmountPerUser[_depositor] + _amount won't overflow, we can perform 2nd addition as unchecked: unchecked {s.totalDepositedAmountPerUser[_depositor] += _amount;}

[G-33] Addition in _requestL2Transaction in Mailbox.sol can be either removed or unchecked.

File: ethereum/contracts/zksync/facets/Mailbox.sol

uint64 expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION); // Safe to cast

Constant PRIORITY_EXPIRATION is defined in ethereum/contracts/zksync/Config.sol as:

/// NOTE: The constant is set to zero for the Alpha release period
uint256 constant PRIORITY_EXPIRATION = 0 days;

This addition is then redundant (since we're adding 0). However, if for further reference you want to keep this code as it is (e.g. new contract will be deployed with different Alpha release period), make this operation unchecked at least. Since PRIORITY_EXPIRATION is a constant value, known during compiling time, and block.timestamp is impossible to overflow, it's safe to assume, that block.timestamp + PRIORITY_EXPIRATION will never overflow. The max value of uint64 converted into timestamp is Sun Jul 21 2554. It's almost impossible that block.timestamp + PRIORITY_EXPIRATION will ever exceed that number (assuming that PRIORITY_EXPIRATION will be set to some reasonable value).

[G-34] Do not assign offset in the last call of UnsafeBytes.readUint256() in Mailbox.sol

A simple test in Remix IDE has been created, to compare gas usage of assigning more than one value from function call:

  function getUints() public pure returns (uint, uint) {return (1, 2);}
   function getAll() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, b) = getUints();
    console.log(g - gasleft());
   }
    function getOne() public view {
    uint a; uint b; 
    (a, b) = getUints();
    uint g = gasleft();
    (a, ) = getUints();
    console.log(g - gasleft());
   }

getOne() costs 74 gas, while getAll() costs 82 gas. This implies, that we shouldn't assign every parameter from function call if it's not needed.

In the last call of UnsafeBytes.readUint256(_message, offset) (line 427):

File: ethereum/contracts/zksync/facets/Mailbox.sol

(amount, offset) = UnsafeBytes.readUint256(_message, offset);

we won't need offset anymore, thus we can change line 427 to: (amount, ) = UnsafeBytes.readUint256(_message, offset);.

[G-35] Loop in _commitBatchesWithSystemContractsUpgrade() in Executor.sol can be optimized

File: ethereum/contracts/zksync/facets/Executor.sol

        for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
            // The upgrade transaction must only be included in the first batch.
            bytes32 expectedUpgradeTxHash = i == 0 ? _systemContractUpgradeTxHash : bytes32(0);

The for loop iterates from 0 to _newBatchesData.length. In every iterations (at line 243), it checks if i == 0. However, this condition occurs only during the first loop iteration. This means, that it will be better to divide the loop into two smaller loops:

• for (uint256 i = 0; i < 1; i = i.uncheckedInc()) { bytes32 expectedUpgradeTxHash = _systemContractUpgradeTxHash; (...)
• for (uint256 i = 1; i < _newBatchesData.length; i = i.uncheckedInc()) { bytes32 expectedUpgradeTxHash = bytes32(0); (...)

That way, we will be performing i == 0 check only during the first loop iteration, thus this comparison will be done at least once.

Moreover, to better optimize these two loops, please make sure to declare i counter outside that loops. That way, you won't be redeclaring i inside the 2nd loop:

uint256 i = 0;
for (i; i < 1; i = i.uncheckedInc()) { 
bytes32 expectedUpgradeTxHash =  _systemContractUpgradeTxHash; 

(...)
}

for (i; i < _newBatchesData.length; i = i.uncheckedInc())
{
    bytes32 expectedUpgradeTxHash =  bytes32(0); 
    (...)
}

Additionally, as you can notice, expectedUpgradeTxHash is used only once, so it does not need to be declared at all. It can be used directly:

• in the first loop:

_lastCommittedBatchData = _commitOneBatch(
                _lastCommittedBatchData,
                _newBatchesData[i],
                _systemContractUpgradeTxHash
            );

• in the second loop:

_lastCommittedBatchData = _commitOneBatch(
                _lastCommittedBatchData,
                _newBatchesData[i],
                bytes32(0)
            );

[G-36] Calculations which won't underflow/overflow in Executor.sol can be unchecked

File: ethereum/contracts/zksync/facets/Executor.sol

require(block.timestamp - COMMIT_TIMESTAMP_NOT_OLDER <= batchTimestamp, "h1"); // New batch timestamp is too small
require(lastL2BlockTimestamp <= block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA, "h2"); // The last L2 block timestamp is too big

Since COMMIT_TIMESTAMP_NOT_OLDER and COMMIT_TIMESTAMP_APPROXIMATION_DELTA are constant values, which are known before compilation time, it's reasonable to assume, that they are chosen in a way, which:

• block.timestamp >= COMMIT_TIMESTAMP_NOT_OLDER
• block.timestamp + COMMIT_TIMESTAMP_APPROXIMATION_DELTA won't overflow. If that's the case - those two lines can be unchecked.

[G-37] Move some require checks on top of function in Executor.sol

File: ethereum/contracts/zksync/facets/Executor.sol

uint256 currentTotalBatchesVerified = s.totalBatchesVerified;
uint256 committedBatchesLength = _committedBatches.length;

// Save the variable from the storage to memory to save gas
VerifierParams memory verifierParams = s.verifierParams;

// Initialize the array, that will be used as public input to the ZKP
uint256[] memory proofPublicInput = new uint256[](committedBatchesLength);

// Check that the batch passed by the validator is indeed the first unverified batch
require(_hashStoredBatchInfo(_prevBatch) == s.storedBatchHashes[currentTotalBatchesVerified], "t1");

require(_hashStoredBatchInfo(_prevBatch) == s.storedBatchHashes[currentTotalBatchesVerified], "t1"); can be moved up:

uint256 currentTotalBatchesVerified = s.totalBatchesVerified;

// Check that the batch passed by the validator is indeed the first unverified batch
require(_hashStoredBatchInfo(_prevBatch) == s.storedBatchHashes[currentTotalBatchesVerified], "t1");

uint256 committedBatchesLength = _committedBatches.length;

// Save the variable from the storage to memory to save gas
VerifierParams memory verifierParams = s.verifierParams;

// Initialize the array, that will be used as public input to the ZKP
uint256[] memory proofPublicInput = new uint256[](committedBatchesLength);

That way, when this check will revert, function won't waste gas on performing other instructions.

[G-38] Simply unchecked operations can be done directly in the code, instead of being imported from the library

File ethereum/contracts/common/libraries/UncheckedMath.sol is a library which implements two simple unchecked functions: uncheckedInc() and uncheckedAdd(). Those functions are used across the whole code-base, mostly inside loops:

• File ethereum/contracts/zksync/facets/Executor.sol

209:        for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
241:        for (uint256 i = 0; i < _newBatchesData.length; i = i.uncheckedInc()) {
263:        for (uint256 i = 0; i < _nPriorityOps; i = i.uncheckedInc()) {
293:        for (uint256 i = 0; i < nBatches; i = i.uncheckedInc()) {
330:        for (uint256 i = 0; i < committedBatchesLength; i = i.uncheckedInc()) {

• File ethereum/contracts/zksync/facets/Mailbox.sol

395:        for (uint256 i = 0; i < factoryDepsLen; i = i.uncheckedInc()) {

• File ethereum/contracts/zksync/libraries/Diamond.sol

100:        for (uint256 i = 0; i < facetCutsLength; i = i.uncheckedInc()) {
138:        for (uint256 i = 0; i < selectorsLength; i = i.uncheckedInc()) {
159:        for (uint256 i = 0; i < selectorsLength; i = i.uncheckedInc()) {
179:        for (uint256 i = 0; i < selectorsLength; i = i.uncheckedInc()) {

• File ethereum/contracts/zksync/facets/Getters.sol

182:        for (uint256 i = 0; i < facetsLen; i = i.uncheckedInc()) {

• File ethereum/contracts/zksync/libraries/Merkle.sol

29:        for (uint256 i; i < pathLength; i = i.uncheckedInc()) {

• File: ethereum/contracts/common/AllowList.sol

62:        for (uint256 i = 0; i < targetsLength; i = i.uncheckedInc()) {
96:        for (uint256 i = 0; i < callersLength; i = i.uncheckedInc()) {

However, using library functions, instead of directly increasing i in an unchecked block is very gas ineffective.

To prove that, a simple test in Remix IDE was prepared:

    function uncheckedInt() public view {
    uint gas = gasleft();
    for (uint i = 0; i < 100; ) {
        require(i == i);
    unchecked {++i;}
    }

    console.log(gas - gasleft());
}

    function uncheckedIntLib() public view {
    uint gas = gasleft();
    for (uint i = 0; i < 100; i = i.uncheckedInc()) {require(i == i);
    }
    console.log(gas - gasleft());
    }

Function uncheckedInt() costs 7442 gas, while uncheckedIntLib() costs 11742 gas. This implies, that every loop in a form:

for (uint256 i = 0; (...) i = i.uncheckedInc()) {

should be rewritten to:

for (uint256 i = 0; (...) i = i.uncheckedInc()) {
(...)
unchecked {++i;}
}

There are two additional places in ethereum/contracts/zksync/facets/Executor.sol, where the similar rule applies

• File ethereum/contracts/zksync/facets/Executor.sol

123:        for (uint256 i = 0; i < emittedL2Logs.length; i = i.uncheckedAdd(L2_TO_L1_LOG_SERIALIZE_SIZE)) {
331:            currentTotalBatchesVerified = currentTotalBatchesVerified.uncheckedInc();

In line 123, i = i + L2_TO_L1_LOG_SERIALIZE_SIZE should also be unchecked. In line 331, it should be: unchecked {++currentTotalBatchesVerified;}

[G-39] Redundant if condition in Executor.sol

File: ethereum/contracts/zksync/facets/Executor.sol

 if (_proof.serializedProof.length > 0) {
            bool successVerifyProof = s.verifier.verify(
                proofPublicInput,
                _proof.serializedProof,
                _proof.recursiveAggregationInput
            );
            require(successVerifyProof, "p"); // Proof verification fail
        }
        // #else
        bool successVerifyProof = s.verifier.verify(
            proofPublicInput,
            _proof.serializedProof,
            _proof.recursiveAggregationInput
        );
       require(successVerifyProof, "p"); // Proof verification fail

In above code section, we're performing zkp verification twice. For the first time - when if (_proof.serializedProof.length > 0). And then, for the second time - in line 363.

This is redundant. Remove the whole if (_proof.serializedProof.length > 0) block, because we're repeating s.verifier.verify() call after that block for the second time.

[G-40] Event in Executor.sol can be optimized

File: ethereum/contracts/zksync/facets/Executor.sol

 s.totalBatchesCommitted = _newLastBatch;

        // Reset the batch number of the executed system contracts upgrade transaction if the batch
        // where the system contracts upgrade was committed is among the reverted batches.
        if (s.l2SystemContractsUpgradeBatchNumber > _newLastBatch) {
            delete s.l2SystemContractsUpgradeBatchNumber;
        }

        emit BlocksRevert(s.totalBatchesCommitted, s.totalBatchesVerified, s.totalBatchesExecuted);

Since s.totalBatchesCommitted = _newLastBatch, we can change event to:

 emit BlocksRevert(_newLastBatch, s.totalBatchesVerified, s.totalBatchesExecuted);

[G-41] Save one negation by rewritting the statement in DiamonProxy.sol

File: ethereum/contracts/zksync/DiamondProxy.sol

require(!diamondStorage.isFrozen || !facet.isFreezable, "q1");

!A || !B <=> !(A & B) which means, it can be rewritten to:

require(!(diamondStorage.isFrozen && facet.isFreezable)), "q1");

uint64 expirationTimestamp = uint64(block.timestamp + PRIORITY_EXPIRATION);

uint64 expirationTimestamp = uint64(block.timestamp + 0);