November Diff Audit

Table of Contents

Summary

Type

Layer 2

Timeline

From 2023-11-27

To 2023-12-05

Languages

Solidity, Yul

Total Issues

14 (9 resolved, 1 partially resolved)

Critical Severity Issues

0 (0 resolved)

High Severity Issues

0 (0 resolved)

Medium Severity Issues

0 (0 resolved)

Low Severity Issues

3 (2 resolved)

Notes & Additional Information

11 (7 resolved, 1 partially resolved)

Scope

Most of the contracts in scope have been audited at a past commit. In addition, the Matter Labs codebase has moved from private to public repositories. For this reason, we audited code from 4 different repositories and between several commits.

Specifically, we audited select files from the era-system-contracts repository, in the v1.4.1-integration branch, at commit ef0eb0c, and from the era-contracts repository, in the v1.4.1-integration branch, at commit 518bfff.

The exact scope is as follows:

System Overview

No major system changes were included in this audit's scope. Most of the contracts have been inspected for code diffs compared to a previously audited version. The changes include fixes from past audits and some issues reported during the Code4rena contest. Also, note that the private zksync-2-contracts repository has been moved to the public era-contracts repository, while the private system-contracts repository has been moved to the public era-system-contracts repository.

The most notable changes, per contract, are as follows:

• zksync-2-contracts/era-contracts:
  • Mailbox.sol: Fixed wording issues, removed user access restrictions and deposit limit, and introduced formatting changes.
  • Executor.sol: Fixed wording issues, introduced extra sanity checks on the L2 batch and block timestamp (which are now included within each batch), moved batch proof verification code to the verifier contract, and made formatting changes.
  • Getters.sol: Made formatting changes, performed code organization, and removed dead code.

The Base.sol, Admin.sol and TransactionValidator.sol contracts were audited as a whole.

• system-contracts/era-system-contracts:
  • Compressor.sol: Implemented a new format for the state diffs compression encoding and enhanced the verification of data compression correctness.
  • bootloader.yul: Made formatting and code organization changes, added additional temporary code needed for the keccak256 precompile upgrade procedure, implemented more sanity checks, and introduced extra functionality to publish L2 batch and block timestamp data to L1.
  • L1Messenger.sol: Updated state diffs verification to follow the new compression format and made gas charges more accurate.

Security Model and Trust Assumptions

No further trust assumptions were introduced in the audited contracts. The allowList has been removed from the Mailbox contract, allowing any user to request an L2 transaction.

Low Severity

Missing Docstrings

Throughout the codebase, there are several parts that do not have docstrings:

• The Admin.sol, Executor.sol, Getters.sol, and Mailbox.sol contracts are missing docstrings for the getName constant.
• The IAdmin.sol, IExecutor.sol, IGetters.sol, ILegacyGetters.sol, IMailbox.sol, IVerifier.sol, and IZkSync.sol interfaces are missing docstrings for multiple function declarations.

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 #127 at commit d4bd820.

The getFirstUnprocessedPriorityTx Function Does Not Revert When There Are No Unprocessed Transactions

The getFirstUnprocessedPriorityTx function of the Getters contract is expected to return the first unprocessed priority transaction. In case there are no unprocessed priority transactions, the function should revert, as indicated in the NatSpec comment comment. The issue is that the function uses the PriorityQueue library, which simply returns the head of the queue and does not revert when there are no unprocessed priority transactions.

Consider reverting when there are no unprocessed priority transactions.

Update: Resolved in pull request #137 at commit ff8a9cd. The Matter Labs team stated:

We decide to keep the current behavior for the simplicity of the function. However, it does make sense to update the comment.

The Refund Recipient Might Be Aliased to an Unexpected Address

The Mailbox contract implements the requestL2Transaction function that allows communication from L1 to L2. It allows to specify a _refundRecipient address on L2 that will receive the refund after the transaction's completion. The function handles the following scenarios:

• if _refundRecipient is a contract on L1, the refund will be sent to the aliased _refundRecipient.
• If _refundRecipient is set to address(0) and the sender has not deployed bytecode on L1, the refund will be sent to the msg.sender address.
• If _refundRecipient is set to address(0) and the sender has deployed bytecode on L1, the refund will be sent to the aliased msg.sender address.

The internally triggered _requestL2Transaction function checks if refundRecipient is a contract by comparing its code size with 0 and applying L1 to L2 alias only in case it is a contract. In addition, at the beginning of the function, msg.sender address is aliased in case it is a contract. This means that in case the sender is a smart contract and _refundRecipient is set to address(0), the address aliasing will be applied twice on msg.sender.

Consider fixing the code so that the address aliasing is applied exactly once when needed. Even if the aliased addresses are not currently immediately used for claiming refunds, the current implementation returns unexpected results and could lead to serious bugs in future upgrades.

Update: Acknowledged, not resolved. We concluded that the problematic scenario is probabilistically impossible, so the issue is dismissed. The Matter Labs team stated:

In case the sender is a smart contract and the recipient is address(0), then we apply the alias to the sender. Additionally, in case the the refundRecipient is address(0), then the refund recipient would become equal to the aliased sender. Note that we will never get into the next if since the aliased sender can not contain any code because of collision resistance.

Notes & Additional Information

Todo Comments in the Code

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

The identified instance of TODO/Fixme comments:

• The TODO comment on line 151 in TransactionValidator.sol.

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: Acknowledged, will resolve. The Matter Labs team stated:

This comment will be resolved in the future release.

Inconsistent Use of Named Returns

Throughout the codebase, there are multiple contracts that have inconsistent usage of named returns in their functions:

• In ExecutorFacet contract of Executor.sol
• In GettersFacet contract of Getters.sol
• In MailboxFacet contract of Mailbox.sol
• In IGetters interface of IGetters.sol
• In IMailbox interface of IMailbox

To improve the readability and consistency of a contract, use the same return style in all of its functions. As such, consider naming the return variables of all functions.

Update: Acknowledged, not resolved. The Matter Labs team stated:

Especially for smaller functions, always using named returned variables will unnecessarily increase the amount of code so it is better to keep things as they are for readability.

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.

Throughout the codebase, global imports are being used:

• Lines 5-6 and line 8 in Admin.sol.
• Lines 5-6 in Base.sol
• Lines 5-10 in Getters.sol.
• Line 5 in IAdmin.sol
• Line 5 and line 7 in IGetters.sol
• Line 5 in ILegacyGetters.sol
• Line 6 in IMailbox.sol
• Lines 5-8 in IZkSync.sol
• Lines 5-8 in TransactionValidator.sol

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 #126 at commit 5ae779e. The Matter Labs team stated:

This PR additionally creates some new implicit imports in other files, but those will be fixed in subsequent PRs (i.e. those files are in scope of other audits).

Unused Function With Internal Visibility

The _maxU256 internal function of Executor.sol contract is not used.

To improve the overall clarity, intentionality, and readability of the codebase, consider using or removing any currently unused functions.

Update: Resolved in pull request #125 at commit 7e24b66.

Typographical Errors

The following typographical errors were identified in the codebase:

• The comment in TransactionValidator.sol should be dependencies not dependenies.
• The comment in TransactionValidator.sol should be auxiliary not auxilary.
• The comment in TransactionValidator.sol should be dependencies not dependenies.
• The comment in Executor.sol should be noticeable not noticable.
• The comment in Bootloader.yul should be maintenance not maintainance.
• The comment in Bootloader.yul should be Scratch not Scatch
• The comment in Bootloader.yul should be accommodate not accomodate
• The comment in Bootloader.yul should be trigger not triger
• The comment in Bootloader.yul should be Transferring not Transfering
• The comment in Bootloader.yul should be exhausted not exhaused
• The comment in ILegacyGetters.sol should be kept not keot.
• The comment in Getters.sol should be blockchain not batchchain.

Consider fixing the aforementioned typographical errors to improve the readability and clarity of the codebase.

Update: Resolved in pull request #120 at commit fdd9006 and pull request #91 at commit cf955cc.

The isFacetFreezable function Returns Value for Non-Existent Facets

The isFacetFreezable function of the Getters contract returns a boolean value (true or false) depending on whether the given facet is freezable or not. However, the current implementation logic defaults to returning false even for non-existent facets.

Instead of returning false, consider reverting in case the facet does not exist.

Update: Acknowledged, not resolved. The Matter Labs team stated:

The current interface subjectively looks easier to use and maintain. Besides, the function is generally rarely used. If someone is not sure whether a facet is a valid one, they could also request the facetFunctionSelectors() function.

Lack of Consistency in Error Messages

The Base contract, inherited by the facets, implements modifiers to handle access control for specific functions. The error messages in the require statements use codes, but for the onlyGovernorOrAdmin modifier, the full error message is used.

For the sake of clarity and consistency, consider using the error code for validation in the onlyGovernorOrAdmin modifier.

Update: Resolved in pull request #121 at commit 4051533.

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 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 them, it becomes easier for the maintainers of those libraries to make contact with the appropriate person about the problem and provide mitigation instructions.

Throughout the codebase, there are contracts that do not have a security contact:

• The IAdmin contract
• The IExecutor contract
• The IGetters contract
• The ILegacyGetters contract
• The IMailbox contract
• The IVerifier contract
• The IZkSync contract

Consider adding a NatSpec comment containing a security contact above the contract definition. 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 #122 at commit c969649.

Unused Imports

The following unused imports were identified throughout the codebase:

• In Executor.sol, the constants MAX_INITIAL_STORAGE_CHANGES_COMMITMENT_BYTES and MAX_REPEATED_STORAGE_CHANGES_COMMITMENT_BYTES imported from Config.sol are unused.
• In Executor.sol, the constant L2_KNOWN_CODE_STORAGE_SYSTEM_CONTRACT_ADDR imported from L2ContractAddresses.sol is unused.

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

Update: Resolved in pull request #123 at commit 3f2f947.

Incorrect or Misleading Docstrings

The following instances of incorrect or misleading docstrings were identified throughout the codebase:

• In the publishPubdataAndClearState function of the L1Messenger contract, the docstring explaining the expected encoding of the state diffs in the _totalL2ToL1PubdataAndStateDiffs parameter is partially incorrect. The docstring describes the encoding as:

 header (1 byte version, 3 bytes total len of compressed, 1 byte enumeration index size, 2 bytes number of initial writes)
body (N bytes of initial writes [32 byte derived key || compressed value], M bytes repeated writes [enumeration index || compressed value])

whereas, in fact, it is:

 header (1 byte version, 3 bytes total len of compressed, 1 byte enumeration index size)
body (`compressedStateDiffSize` bytes,  4 bytes number of state diffs, `numberOfStateDiffs` * `STATE_DIFF_ENTRY_SIZE` the state diffs) 

• At Line 81 of Mailbox.sol, key is documented as being the hash of L1ToL2Transaction whereas it is the hash of the L2 transaction.

Consider fixing all instances of incorrect or misleading documentation to enhance the overall clarity and readability of the codebase.

Update: Partially resolved in pull request #92 at commit 66481dd. The Matter Labs team stated:

The first issue is acknowledged, but the second one is disagreed with, since this function is intended to be used specifically for L1->L2 transaction and not just any L2 transaction.

Inconsistent Use of override Keyword

Throughout the codebase, several functions have the override keyword in their signature as they are being implemented from an interface. However, in the most current versions of the Solidity compiler, the override keyword is not necessary for interface implementation.

Specifically:

• The commitBatches function of the Executor contract
• The proveL1ToL2TransactionStatus and finalizeEthWithdrawal functions of the Mailbox contract

Consider removing the redundant override keywords to simplify and be consistent with the rest of the codebase.

Update: Resolved in pull request #124 at commit 03c579f.

Conclusion

The audit scope did not include any major system changes. Most of the contracts have been audited for code diffs compared to a previously audited version. The changes include fixes from past audits and some issues reported during the Code4rena bug bounty contest.

Only a few low-severity issues were reported during the audit. In addition, several fixes were recommended to improve the readability of the codebase, and facilitate future audits and development for additional functionality. The Matter Labs team has been responsive and eager to provide additional insights and documentation whenever asked.