OpenZeppelin Blog

zkSync L1Messenger Upgrade Audit

Written by OpenZeppelin Security | January 29, 2024

Table of Contents

Summary

Type
Layer 2
Timeline
From 2023-08-30
To 2023-09-14
Languages
Solidity
Total Issues
14 (14 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
1 (1 resolved)
Medium Severity Issues
2 (2 resolved)
Low Severity Issues
4 (4 resolved)
Notes & Additional Information
7 (7 resolved)
Client Reported Issues
0 (0 resolved)

Scope

We audited the matter-labs/system-contracts repository in pull request #283 at commit 4dca36d. The following files were in scope:

 ├── bootloader
│   └── bootloader.yul
├── contracts
│   ├── ComplexUpgrader.sol
│   ├── Compressor.sol
│   ├── Constants.sol
│   ├── ContractDeployer.sol
│   ├── KnownCodesStorage.sol
│   ├── L1Messenger.sol
│   ├── L2EthToken.sol
│   └── SystemContext.sol
├── interfaces
│   ├── IComplexUpgrader.sol
│   ├── ICompressor.sol
│   ├── IKnownCodesStorage.sol
│   ├── IL1Messenger.sol
│   ├── ISystemContext.sol
│   └── ISystemContract.sol
└── libraries
    ├── SystemContractHelper.sol
    └── UnsafeBytesCalldata.sol

Additionally, we audited the matter-labs/zksync-2-contracts repository for pull request #165 at commit 098bc7e. The following files were in scope:

 ethereum
└── contracts
    ├── common
    │   └── L2ContractAddresses.sol
    └── zksync
        ├── Config.sol
        ├── DiamondInit.sol
        ├── facets
        │   ├── Executor.sol
        │   └── Mailbox.sol
        └── interfaces
            └── IExecutor.sol

System Overview

The system upgrade introduces two major changes to the protocol. The first major change is the handling of logs and the delineation between system and user logs. The second change is how state diffs will be handled by the system. The motivation behind these changes primarily revolves around moving certain verifications from the circuit to the smart contracts for ease of upgradeability in the future if necessary. Furthermore, these changes were made as a first step in preparation for EIP-4844, where certain types of data will be placed into the blob while others will continue being part of the calldata.

In this upgrade, the L1Messenger contract maintains three rolling hashes: one of the L2 to L1 logs, one of the messages, and one of the bytecode hashes. At the end of bootloader execution, these hashes are validated and then sent to L1 as system logs along with a hash of the state diff. The sequencer, when committing blocks onto L1, submits the preimages as calldata to ensure data availability.

In order to save gas, the bytecode and state diffs that are submitted as pubdata are compressed. The Compressor system contract validates that the compression is reflective of the original data, while the ZK proof is still tasked with verifying the correctness of execution. More specifically, with these changes, the operator would provide both full and compressed state diffs to the Compressor contract which would verify that the compression matches the original state diffs. Subsequently, the L1Messenger will send a system log to L1 with the hash of the original state diffs, as well as the hash of the logs, messages, and the bytecode hashes. When committing a block on L1, the operator would provide the full preimage of all these hashes in the calldata.

Trust Assumptions

This upgrade does not introduce new roles to the system. As before, the operator role is in control of the inputs to the bootloader as well as the proof generation. They are bound to only generate valid proofs secured by cryptography but are trusted not to censor any transactions. Currently, the operator is centralized through Matter Labs with the goal of decentralizing it in the medium-term future.

 

High Severity

Lack of Data Availability of Bytecode

For rollups, it is crucial to have data availability on the L1 chain. In a ZK-rollup, the validity proof ensures that the L2 sequencer cannot create invalid transactions. However, if the sequencer were to go down or become malicious, having only the proof itself would be insufficient for another node to reconstruct the state on L2. In addition, data availability of bytecode is essential to ensure that not only the state on L2 is able to be reconstructed, but also that the deployed contracts will maintain integrity should the L2 sequencer go down. Furthermore, in the current design with a centralized sequencer, this plays an even more important role as there is a single point of failure.

In the design of the protocol, transactions on L2 can originate from L1 or L2. If a message starts from L1, it is not necessary to resend the bytecode back down to L1, as the bytecode is already available on that layer. However, if a transaction starts on L2 and deploys a new contract, the bytecode must be sent to L1 to ensure data availability.

Currently, a transaction on L2 gets processed by the bootloader using the processL2Tx function. Stepping into this function eventually leads to a call to publishCompressedBytecode in Compressor. After verifying that the original bytecode and the compressed version of the bytecode match, the function markBytecodeAsPublished is called in KnownCodesStorage. This makes a call to _markBytecodeAsPublished with the variable _shouldSendToL1 set as false. Therefore, the step to requestBytecodeL1Publication will be skipped and the bytecode will not be sent to L1.

For transactions that originate on L2, consider making the bytecode available on L1 to ensure data availability.

Update: Resolved in pull request #332 at commit 801b2a2. The raw compressed data is now sent to L1 in the publishCompressedBytecode function.

Medium Severity

Gas Charged Twice for Sending Message to L1

The sendToL1 function of the L1Messenger contract deployed on L2 facilitates the direct transmission of messages to L1. To execute this operation successfully and publish data, it requires the user to provide gas. However, the calculated amount of gas is charged twice. Initially, gas is consumed through the SystemContractHelper library, utilizing the burnGas function, and subsequently, another gas charge occurs through a direct call to the precompile at address 0.

Consider eliminating the direct call to the precompile address and charging gas only once by using the burnGas function provided in the SystemContractHelper library.

Update: Resolved in pull request #331 at commit b351f13. The redundant precompile call was removed and the gas is charged once through the burnGas function of the SystemContractHelper library.

Naming Error

Within the bootloader.yul code, two instances referencing non-existent variables and functions were identified. Compiling the code will lead to a compilation error, as the referred variable and function names lack definitions:

Consider implementing these changes in the aforementioned instances to ensure that the correct variables and functions are referenced.

Update: Resolved in pull request #330 at commit 0027afa.

Low Severity

Missing Error Messages in require Statements

The unsafePrecompileCall and precompileCall functions of the SystemContractHelper contract use require statements that lack descriptive error messages. This can make debugging difficult if the affected functions revert.

These are the dentified instances:

Consider including specific, informative error messages in require statements to improve the overall clarity of the codebase and facilitate troubleshooting whenever a requirement is not satisfied.

Update: Resolved in pull request #333 at commit 2deaa57. The functions that utilized require statements without error messages have been deleted.

Use of Magic Numbers

Magic numbers are used throughout the codebase. This practice is generally discouraged in software development as it can lead to issues when refactoring code, particularly if the magic value is used in more than one location. These are some examples where magic values are used in the codebase:

  • In line 93 of Compressor.sol, the right-hand side uses the magic numbers 4 and 64. Consider replacing 4 with compInitialStateDiffPtr and 64 with a constant that represents the size of the initial write, such as SIZE_OF_INITIAL_WRITE.
  • In line 114 of Compressor.sol, the same magic values of 4 and 64 are used.

Consider updating these magic numbers with a constant that represents their value and storing that constant in the Constants.sol file so that any updates only need to take place in one location in the codebase.

Update: Resolved in pull request #334 at commit f681c23.

Misleading Comment

In line 484 of Executor.sol, it says "Returns if the bit at index {_index} is 1". It should instead say "Returns true if the bit at index {_index} is 1".

Consider resolving this instance of misleading documentation to improve the clarity and readability of the codebase.

Update: Resolved in pull request #219 at commit 59fd7f2.

Improper Hash Value for Unused Leaves in Merkle Tree

The leaves of the Merkle tree that are not used by the L2 logs are set to the default value L2_L1_LOGS_TREE_DEFAULT_LEAF_HASH, which is equal to the hash of the all-zero byte string keccak256(new bytes(L2_TO_L1_LOG_SERIALIZE_SIZE)). It is best practice not to set a default hash to a value where the preimage is known. While a malicious bootloader cannot prepare an array of logs each having a hash equal to L2_L1_LOGS_TREE_DEFAULT_LEAF_HASH (i.e., the preimage of each hash is the all-zero byte string) and have it emitted due to other safeguards in the system contracts, it is still discouraged to use this value, as future upgrades to the codebase could potentially impact these safeguards.

Instead of L2_L1_LOGS_TREE_DEFAULT_LEAF_HASH, consider using L2_L1_LOGS_TREE_DEFAULT_LEAF_HASH - 1 for the value of the unused leaves. This value would most likely have an unknown preimage, which would further protect against a malicious operator should the codebase be changed.

Update: Resolved in pull request #341 at commit c405437.

Notes & Additional Information

Non-Explicit Imports Are Used

The use of non-explicit imports in the codebase can decrease the clarity of the code and may create naming conflicts between locally defined and imported variables. This is particularly relevant when multiple contracts exist within the same Solidity files or when inheritance chains are long. The following instances were identified:

In zksync-2-contracts:

In system-contracts:

Following the principle that clearer code is better code, consider using named import syntax (import {A, B, C} from "X") to explicitly declare which contracts are being imported.

Update: Resolved in pull request #220 at commit 3db8f16 and pull request #335 at commit b2d76b7.

State Variable Visibility Not Explicitly Declared

Throughout the protocol, the visibility of state variables is explicitly declared, but within the L2EthToken.sol contract, the state variable balance is missing such a declaration.

For clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.

Update: Resolved in pull request #336 at commit 032c88a.

Using uint Instead of uint256

Throughout the protocol, the type uint256 is used, with the exception of the L1Messenger.sol contract where uint is used instead.

To improve the codebase's quality and in favor of explicitness, consider replacing all instances of int/uint with int256/uint256.

Update: Resolved in pull request #337 at commit 0b29a6a.

Naming Suggestions

To favor explicitness and readability, the following locations in the contracts may benefit from better naming:

  • The enum SystemLogKey in IExecutor.sol and in Constants.sol has a value named EXPECTED_SYSTEM_CONTRACT_UPGRADE_TX_HASH. Consider renaming this to EXPECTED_SYSTEM_CONTRACT_UPGRADE_TX_HASH_KEY to be consistent with the other values in the enum.
  • The enum SystemLogKey in Constants.sol has a value PREV_BATCH_HASH_KEY. Consider renaming this to PREV_BLOCK_HASH_KEY to be more aligned with its intention and to be consistent with the value in IExecutor.sol
  • The function _setBit in Executor.sol takes in a parameter named _num, which represents a bitmap. Consider renaming this to _bitMap to provide more clarity on the purpose of this parameter.
  • The function publishPubdataAndClearState has a local variable named compressedRepeatedStateDiffs. Consider renaming this to compressedStateDiffs, as this variable stores both the repeated and initial state diffs.

Consider implementing these naming modifications to improve the consistency and readability of the codebase.

Update: Resolved in pull request #221 at commit f9f39bc and pull request #338 at commit 07647f8.

Lack of Security Contact

Providing a specific security contact, such as an email or ENS, within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice proves beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. Additionally, if the contract incorporates third-party libraries and a bug surfaces in these, it becomes easier for the creators of those libraries to make contact, inform the code owners about the problem, and provide mitigation instructions.

Consider adding a NatSpec comment on top of the contracts' definition with a security contact. Using the @custom:security-contact convention is recommended as it has been adopted by the Openzeppelin Wizard and the ethereum-lists.

Update: Resolved in pull request #223 at commit 2fc13dd and pull request #342 at commit a60dbae.

Inconsistency Between Decimal and Hex Representation

In bootloader.yul, there are instances where the add function is called with one operand equal to 32 typed in decimal as add(txDataOffset, 32), while in other places the operand is typed in hexadecimal as add(txDataOffset, 0x20). Some examples of the decimal representation (32) are in line 846, line 869 and line 1094. Some examples of the hexadecimal representation (0x20) are in line 561, line 673 and line 688.

Consider using a single representation throughout the implementation for consistency and readability.

Update: Resolved in pull request #339 at commit 7b97e4d.

Unused Imports

Throughout the protocol, there are imports that are unused and could be removed.

In zksync-2-contracts:

In system-contracts:

Consider removing unused imports to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #222 at commit 061de34 and pull request #340 at commit 4e51af8.

 

Conclusions

This audit was conducted over the course of two weeks. One high-severity issue was identified. A few medium and low-severity issues were found alongside some notes to improve the clarity and readability of the codebase. Some changes were also proposed to ensure smart contract security best practices are followed. We found the dedicated documentation provided by the Matter Labs team to be very helpful in understanding the audited code changes. Furthermore, their team was very supportive in answering questions in a timely manner and even syncing with our team to talk through the changes.
View full post