Matterlabs EIP-4844 Support Audit

OpenZeppelin Security | February 5, 2024

Table of Contents

Summary

Type
Layer 2
Timeline
From 2024-01-15
To 2024-01-19
Languages
Solidity, Yul
Total Issues
12 (10 resolved, 2 partially resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
1 (1 resolved)
Low Severity Issues
2 (1 resolved, 1 partially resolved)
Notes & Additional Information
9 (8 resolved, 1 partially resolved)

Scope

We audited specific files in the zk-4844 branch of the matter-labs/era-contracts repository, at commit abcbaf3. New contracts were audited in full, whereas the rest of the in-scope codebase was audited only for the diffs.

In scope were the following files:

 l1-contracts
├── contracts
│   ├── common
│   │   └── L2ContractAddresses.sol
│   └── zksync
│       ├── Config.sol
│       ├── facets
│       │   └── Executor.sol
│       ├── interfaces
│       │   └── IExecutor.sol
│       └── utils
│           └── BlobVersioniedHash.yul
system-contracts
├── Constants.sol
├── L1Messenger.sol
├── PubdataChunkPublisher.sol
├── interfaces
│   └── IPubdataChunkPublisher.sol
└── bootloader
   └── bootloader.yul

System Overview

This system upgrade has been executed to support proto-danksharding and the utilization of blob-carrying transactions defined in EIP-4844. With proto-danksharding, the protocol can now make use of cheaper storage for pubdata on L1. This allows for packing more transactions per batch, resulting in cheaper transactions. At a contract level, the protocol now supports processing pubdata via blobs, in addition to supporting the processing of pubdata via calldata. This will also set the stage for future changes aimed at separating pubdata publishing from batch commitment, thereby enabling the handling of different data availability solutions.

The changes in this upgrade span across both L2 system contracts and L1 zkSync contracts, namely Executor.sol. Two new contracts were also included to support the additional changes. The PubdataChunkPublisher contract was added on the L2 system contracts side, whereas the BlobVersionedHash contract was added to the L1 zkSync contracts. The added contracts will be explained in detail below, followed by an outline of changes for the existing contracts.

PubdataChunkPublisher Contract

The new PubdataChunkPublisher system contract takes the full pubdata, creates chunks, and commits them in the form of two system logs. The primary purpose of this is to commit to blobs and have those commitments travel to L1 via system logs.

BlobVersionedHash Contract

The EIP-4844 introduces a new opcode called BLOBHASH. Since Solidity does not yet natively support calling the BLOBHASH opcode, the temporary BlobVersionedHash.yul contract had to be added, which acts in place of the eventual BLOBHASH opcode. Once support for BLOBHASH is added to Solidity, all calls to the BlobVersionedHash contract will be substituted for calls to the BLOBHASH opcode.

Bootloader Contract

With the increase in the amount of pubdata due to blobs, changes have been made to the bootloader memory to facilitate more L2-to-L1 logs, compressed bytecodes, and pubdata.

Executor Contract

While the function signature for commitBatches and the structure of CommitBatchInfo stays the same, the format of CommitBatchInfo::pubdataCommitments changes. Prior to EIP-4844, this field held a byte array of pubdata. Now, it can either hold the total pubdata as before or a list of concatenated information for KZG blob commitments.

In order to distinguish between the two, a header byte is added to the byte array. This header byte indicates the source of pubdata, with 0 representing calldata and 1 representing blobs. Any other values in the first byte are rejected. The list of supported values for the header byte can be expanded in the future.

Trust Assumptions

This upgrade does not introduce new roles to the system. The implementation depends on the temporary BlobVersionedHash.yul contract serving as a placeholder for the upcoming BLOBHASH opcode. It is assumed that once support for the BLOBHASH opcode is implemented, calls to the BlobVersionedHash contract will be replaced with the utilization of the BLOBHASH opcode.

 

Medium Severity

Bootloader Uses Incorrect Value for System Contract Upgrade Log Key

The SystemLogKey enum is employed by the L2 system contracts to distinguish between logs. As per its definition, the value assigned to EXPECTED_SYSTEM_CONTRACT_UPGRADE_TX_HASH_KEY is 9. However, in the bootloader.yul file, the protocolUpgradeTxHashKey function associated with EXPECTED_SYSTEM_CONTRACT_UPGRADE_TX_HASH_KEY returns the value 7. This inconsistency may lead to unexpected behavior during the processing of L2-to-L1 logs.

Consider changing the return value of the protocolUpgradeTxHashKey function to the correct value matching EXPECTED_SYSTEM_CONTRACT_UPGRADE_TX_HASH_KEY, which is 9.

Update: Resolved in pull request #178 at commit 88d22b5.

Low Severity

require Statement With Multiple Conditions

In Executor.sol, there is a require statement that checks multiple conditions to be satisfied.

To simplify the codebase and show the most helpful error messages for failing require statements, consider having a single require statement per condition.

Update: Resolved in pull request #179 at commit cf63da3.

Missing Tests

The proposed changes to the system contracts lack tests that could confirm the correctness of the implementation. For instance, the chunkAndPublishPubdata function of the PubdataChunkPublisher contract requires additional testing.

Consider adding tests to enhance the quality and health of the codebase.

Update: Partially resolved in pull request #188 at commit 43e3ecd. The Matter Labs team stated:

We added tests for failure scenarios with relevant revert checks. For success cases, we couldn't check whether the correct system log was published within the test, but have verified it manually.

Notes & Additional Information

Misleading Documentation

Documentation is misleading at some places in the codebase. For instance, the NatSpec comment describing MAX_MEM_SIZE suggests that the memory page consists of 24000000 / 32 VM words. However, the value returned is 30000000.

Consider rephrasing misleading comments to match the intention of the code.

Update: Resolved in pull request #180, at commit 84fbcfd.

Todo Comments in the Code

During development, having well-described TODO/Fixme comments will make the process of tracking and solving them easier. Without this information, these comments might age and important information for the security of the system might be forgotten by the time the system is released to production. These comments should be tracked in the project's issue backlog and resolved before the system deploys.

Multiplies instances of TODO/Fixme comments were found in the codebase:

Consider removing all instances of TODO/Fixme comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO/Fixme comment to the corresponding issues backlog entry.

Update: Resolved in pull request #181, at commit d23d5c3.

Lack of Security Contact

Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice is quite 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. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for the maintainers of those libraries to contact the appropriate person about the problem and provide mitigation instructions. The IPubdataChunkPublisher interface does not appear to have a security contact.

Consider adding a NatSpec comment containing a security contact above the contract definitions. 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 #182 at commit 54e982e.

Uninitialized Local Variable

The local variable blobVersionedHash in Executor.sol is declared and subsequently initialized with the return value of _getBlobVersionedHash function later in the code.

To enhance the overall clarity, intent, and readability of the codebase, consider declaring and initializing the blobVersionedHash variable in a single line.

Update: Resolved in pull request #183 at commit 0edd4ed.

Addresses Are Not Ordered Correctly

The constant values of the addresses in the L2ContractAddresses contract are not ordered incrementally which is error-prone.

Consider ordering all addresses incrementally.

Update: Resolved in pull request #184 at commit 4da7ca8.

Typographical Errors

The following typographical errors were identified throughout the codebase:

Consider fixing the above errors for improved clarity and readability.

Update: Resolved in pull request #185 at commit e9479f2.

Unused Code

In the Executor contract, the check on line 189 implies that the number of _newBatchesData is equal to 1. This check makes the following parts of the code irrelevant in subsequent executions:

  1. The check on line 192 is obsolete because _newBatchesData.length is equal to 1 (i.e., greater than 0).
  2. Both for-loops on lines 216 and 248 can be refactored. If the length of the _newBatchesData array is known to be 1, there is no value in iterating over it.

Moreover, consider restructuring the commitBatches function to only accept data from a single new batch instead of a CommitBatchInfo array. Furthermore, consider assessing further interdependent functions within the Executor contract to accommodate this modification.

Update: Partially resolved in pull request #186 at commit 2aeb40a. The Matter Labs team stated:

We chose to remove the require on length being greater than 0 as the only change. Supporting 1 batch per commitment is a temporary change so leaving the code minimizes unnecessary work.

Chunks Are Published Before Data Is Validated

The publishPubdataAndClearState function of L1Messenger contract calls the pubdata chunk publisher contract in order to chunk and publish data. The issue lies in the fact that the call occurs before the check that ensures that the calldata does not contain extra data.

Consider swapping the checks to ensure that the data is in the correct format before calling the pubdata chunk publisher contract.

Update: Resolved in pull request #187 at commit c53456f.

Missing Docstrings

Throughout the codebase, there are several parts that have an incomplete docstring:

Consider thoroughly documenting all functions (and their parameters) that are part of any contract's public API. Functions implementing sensitive functionality, even if not public, should be clearly documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Update: Resolved in pull request #189 at commit f244ede.

 

Conclusion

The system upgrade introduces proto-danksharding and implements blob-carrying transactions as per EIP-4844, resulting in cheaper transactions on L1. The upgrade affects L2 system contracts, such as the addition of PubdataChunkPublisher for committing pubdata to blobs and BlobVersionedHash for handling the new BLOBHASH opcode. The Bootloader contract is adjusted to accommodate increased pubdata, while the Executor contract undergoes changes in the format of pubdata commitments to support both calldata and blob commitments, distinguished by a header byte. The added contracts and modifications set the foundation for future data availability solutions.

The audit uncovered one issue of medium severity in addition to several other issues of low severity. Various recommendations have also been made to enhance the quality and documentation of the codebase. We found the dedicated documentation provided by Matter Labs team to be very helpful in understanding the audited code changes. Throughout the audit period, the Matter Labs team was very supportive and answered all questions we had in a timely manner.

Related Posts