December Diff and Governance Audit

OpenZeppelin Security | January 29, 2024

Table of Contents

Summary

Type
ZK Rollup
Timeline
From 2023-12-04
To 2023-12-22
Languages
Solidity
Total Issues
21 (16 resolved, 1 partially resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
2 (1 resolved, 1 partially resolved)
Low Severity Issues
5 (4 resolved)
Notes & Additional Information
14 (11 resolved)

Scope

We audited the diff of the matter-labs/era-contracts repository from the base at commit c44852f and the head at commit 2e0734b, as well as the diff of the matter-labs/era-system-contracts repository from the base at commit 4dca36d and the head at commit 0b238cd. New and marked contracts were audited in full while the rest of the in-scope codebase was audited only for the diffs.

In scope were the following contracts:

 era-contracts/
├─ ethereum/
│  └─ contracts/
│     ├─ bridge/
│     │  ├─ L1ERC20Bridge.sol
│     │  ├─ L1WethBridge.sol
│     │  └─ interfaces/
│     │     └─ IL1Bridge.sol
│     ├─ common/
│     │  ├─ L2ContractAddresses.sol
│     │  ├─ ReentrancyGuard.sol
│     │  └─ libraries/
│     │     ├─ L2ContractHelper.sol
│     │     ├─ UncheckedMath.sol
│     │     └─ UnsafeBytes.sol
│     ├─ governance/
│     │  ├─ Governance.sol
│     │  └─ IGovernance.sol
│     ├─ upgrades/
│     │  ├─ BaseZkSyncUpgrade.sol
│     │  └─ DefaultUpgrade.sol
│     └─ zksync/
│        ├─ Config.sol
│        ├─ DiamondInit.sol
│        ├─ DiamondProxy.sol
│        ├─ Storage.sol
│        ├─ ValidatorTimelock.sol (full audit)
│        └─ libraries/
│           ├─ Diamond.sol
│           ├─ LibMap.sol
│           ├─ Merkle.sol
│           └─ PriorityQueue.sol
└─ zksync/
   └─ contracts/
      ├─ L2ContractHelper.sol
      └─ bridge/
         ├─ L2ERC20Bridge.sol
         ├─ L2StandardERC20.sol
         ├─ L2Weth.sol
         └─ L2WethBridge.sol

 era-system-contracts/
└─ contracts/
   ├─ BootloaderUtilities.sol
   ├─ ComplexUpgrader.sol
   ├─ Constants.sol
   ├─ ImmutableSimulator.sol
   ├─ KnownCodesStorage.sol
   ├─ L2EthToken.sol
   ├─ MsgValueSimulator.sol
   ├─ NonceHolder.sol
   ├─ SystemContext.sol
   ├─ interfaces/
   │  ├─ IComplexUpgrader.sol
   │  ├─ ICompressor.sol
   │  ├─ IKnownCodesStorage.sol
   │  ├─ IL1Messenger.sol
   │  ├─ ISystemContext.sol
   │  ├─ ISystemContextDeprecated.sol
   │  └─ ISystemContract.sol
   └─ libraries/
      ├─ RLPEncoder.sol
      ├─ SystemContractHelper.sol
      └─ UnsafeBytesCalldata.sol

System Overview

This system upgrade refactors existing code while also adding new contracts to the protocol. Most prominent is the introduction of the new Governance contract which has been implemented to slow down the execution of protocol changes. In addition, future upgrades will be based on the new DefaultUpgrade contract. These contracts will be explained in detail below, followed by an outline of changes for the existing contracts.

Governance Contract

The new Governance contract is inspired by OpenZeppelin's TimelockController and the in-house Diamond Proxy upgrade mechanism. This contract is fundamental to managing and coordinating upgrades and changes across Matter Labs' governed zkSync Era contracts. The Governance contract's functionalities include the following:

  1. Operational Management: The Governance contract's owner can schedule, execute, and cancel operations, while ensuring that each step occurs with the appropriate permissions and after the set delay.
  2. Transparent and Shadow Upgrades: The Governance contract supports two types of upgrades: transparent upgrades with on-chain data exposure, and shadow upgrades where the operation data is not published on-chain before the execution. This dual approach balances transparency with security needs.
  3. Security Council: A unique feature of the Governance contract is the integration of a security council, presumably a multisig contract. This council plays a crucial role in overseeing operations and is capable of executing scheduled proposals instantly. This adds a layer of flexibility and rapid response capabilities to the governance process.
  4. Timelock Mechanism: The contract enforces a timelock mechanism, ensuring that operations are subject to a delay before they can be executed. This feature prevents abrupt changes and gives the community time to review and respond to proposed updates or actions. The security council is able to skip the delay for instant execution of scheduled operations.

Upgrade Contracts

The BaseZkSyncUpgrade contract provides a foundation for upgrading the zkSync protocol. It introduces a ProposedUpgrade struct to represent an upgrade proposal. It enforces the upgrade to happen at a given time and provides functions to do the following:

  • Set the L2 bootloader bytecode hash.
  • Set the default account bytecode hash.
  • Set the new verifier address and its parameters.
  • Set the L2 transaction hash that can perform more L2 contract changes. This hash is checked against during batch finalization in the Executor.
  • Set a new protocol version.

The DefaultUpgrade contract extends BaseZkSyncUpgrade and serves as a default implementation for upgrading the zkSync protocol. It defines placeholder functions for custom logic which is used to upgrade L1 contracts and perform post-upgrade actions. The upgrade function orchestrates the entire upgrade process, invoking functions from the base contract.

Era Contracts Changes

Further era-contracts-related changes involve the following:

  • The ValidatorTimelock contract now stores batch timestamps as 32-bit values instead of 256-bit values to use less storage. A modified version of vectorized/solady's LibMap is used to pack eight 32-bit values into one storage slot.
  • The ValidatorTimelock::executeBatches function now allows for committing, proving, and executing a batch within the same block. Previously, it was not possible to commit, prove, and execute a batch in the same block.
  • With the removal of the AllowList contract, the senderCanCallFunction modifier was also removed from the affected functions.
  • The deposit limit check as part of the AllowList functionality was also taken out of the L1ERC20Bridge.
  • The L2StandardERC20 contract was extended with a reinitialization function to overwrite the token metadata.
  • Several constants were added to some in-scope contracts.
  • Formatting, documentation, and code-style changes were made in some in-scope contracts.
  • The Solidity version was upgraded to 0.8.20 for all in-scope contracts.

Era System Contracts Changes

The era-system-contracts changes are as follows:

  • The SystemContractHelper::unsafePrecompileCall internal function no longer ensures that there is enough gas for burning.
  • The SystemContractHelper::precompileCall internal function was removed.
  • The SystemContractHelper::getZkSyncMeta internal function now assigns values to the ZkSyncMeta::heapSize and ZkSyncMeta::auxHeapSize return parameters.
  • The UnsafeBytesCalldata::readUint256 internal function was added.
  • The NonceHolder::incrementDeploymentNonce function no longer has the ISystemContract::onlySystemCall modifier but still requires the message sender to be DEPLOYER_SYSTEM_CONTRACT.
  • The KnownCodesStorage::_burnGas internal function was removed.
  • SystemLogKey::NEW_TIMESTAMP_KEY was renamed to SystemLogKey::PACKED_BATCH_AND_L2_BLOCK_TIMESTAMP_KEY and now contains both the 128-bit batch timestamp and the 128-bit L2 block timestamp in 256 bits.
  • Several constants were added to some in-scope contracts.
  • Formatting, documentation, and code-style changes were made in some in-scope contracts.
  • The Solidity version was upgraded to 0.8.20 for all in-scope contracts.

Security Model and Trust Assumptions

This new upgrade introduces trust assumptions that are mostly related to the new governance contract:

  • The owner of the Governance and TimelockValidator contracts is anticipated to be a multisig setup conforming to industry standards and incorporating the use of cold wallets.
  • The security council is envisioned to be a multisig entity characterized by diversity, substantial size, and a high decision-making threshold. As of the time of this audit, the exact design and composition of the security council are still under development.
  • In an event where both the owner and the security council are compromised, they still have the capability to execute immediate actions. The expectation is that these entities always act with the userbase's best interests in mind.
  • The protocol is supposed to set a delay that provides users with enough time to withdraw their funds in case of any suspicious activities.

Privileged Roles

  • Governance Contract Owner: Possesses the authority to implement significant changes within the protocol, albeit with a built-in delay to allow for community reaction and response.
  • Security Council: The only actor that can bypass the delay in case of emergencies or immediate threats.
  • ValidatorTimelock Contract Owner: Possesses the authority to implement significant changes by updating the verifier EOA address and the minimum delay required between committing and executing batches.

The design of these roles shows a balance between centralized control and distributed trust, aiming to safeguard the protocol while enabling effective governance and rapid response to any arising challenges or threats.

 

Medium Severity

Overwriting ERC-20 Metadata Can Cause Unexpected Behaviors

The L2StandardERC20 contract is the default contract that is deployed when any ERC-20 token is bridged for the first time. This contract has been extended with a reinitializeToken function that allows the Beacon owner to overwrite the symbol, name, and decimals state variables. This feature is particularly beneficial when token developers aim to deploy an official custom contract on the L2, enabling differentiation between the official and default token by altering the symbol and the name. While the rationale for updating the token symbol and name is valid, modifying the decimal value could result in unpredictable behavior and thus introduce significant risk. Furthermore, note that by setting a new name in the EIP712Upgradeable contract through the ERC20PermitUpgradeable contract, previously signed permits will become invalid.

To mitigate these risks, consider removing the option of overwriting the decimals value in the reinitializeToken function. Moreover, consider issuing clear warnings regarding any changes in the token's name in order to ensure that users are fully aware of the potential invalidation of their previously signed permits.

Update: Resolved in pull request #139 at commit a6ceba0. The decimals overwrite was removed from the function.

Critical and High Vulnerabilities in Yarn Module Dependencies

The project's current dependencies include versions of yarn modules that are known to have critical and high vulnerabilities. The following dependencies are not imported directly but are nonetheless included as part of other project dependencies:

In the era-contracts repository:

In the era-system-contracts repository:

To mitigate the risks associated with these vulnerabilities, consider upgrading these modules to versions that have resolved those issues. This may involve updating the primary dependencies that include these modules. If the vulnerable modules are part of unmaintained projects, consider seeking alternative modules that offer similar functionality without the security risks. In cases where immediate replacement or upgrading is not feasible, it is crucial to document these issues clearly and address the potential risks in the project's documentation and security protocols. This proactive approach ensures awareness and readiness to handle any security challenges that might arise from these vulnerabilities.

Update: Partially resolved in pull request #98 at commit ada1bac, and in pull request #153 at commit e9246e3. The Matter Labs team stated:

We upgraded most of the listed dependencies, except for flat and minimatch. These dependencies belong to the Hardhat gas reporter which is still in use in our repo today. Since the main product of the repo is smart contracts, the corresponding JavaScript vulnerabilities cannot be exploited.

Furthermore, it was recommended to set up Dependabot to monitor vulnerable dependencies and keep them up to date more automatically.

Low Severity

minDelay Set to Zero Undermines Security Council Purpose

The Governance contract is a timelock that allows scheduling operations that make critical changes to the protocol. These scheduled operations can be executed after waiting for a minDelay period in seconds. During construction or through a scheduled operation, this minDelay can be set to zero (or a low value).

This option of a zero minDelay has the following implications:

  • A compromised owner can perform malicious actions immediately.
  • Hence, the role of the security council to skip the delay is obsolete.
  • Users do not have enough time to withdraw their funds.

To mitigate these risks, consider defining a constant minimum limit for minDelay (e.g., by facilitating the UPGRADE_NOTICE_PERIOD constant), thereby allowing adequate time for stakeholders to respond to any proposed modifications.

Update: Acknowledged, not resolved. While the team intends to have a non-zero minDelay after the initial stage, a minimal minDelay has not been hard coded. The Matter Labs team stated:

At the initial stage of the protocol, the minDelay is 0. It is part of the initial training wheels of the protocol to allow for swift reaction in case of a needed emergency upgrade. While the alternative to achieve the same effect is to leverage the security council (basically set the security council to be the same entity as the owner of the Governance), having it as zero is more explicit and easier to maintain.

Undisclosed Functionality Mismatch in L1 and L2 Token Functions Post-Bridging

When bridging tokens from L1 to L2, by default, the L2StandardERC20 contract will be deployed initially. However, this default implementation can introduce a gap in functionalities when compared to their L1 counterparts, such as rebase methods or voting capabilities. This discrepancy can cause confusion for users or developers trying to interact with these tokens like they would on L1.

To enhance transparency, consider adding a disclaimer to the existing note in the documentation as well as to the ERC-20 bridge contracts documentation [1, 2]. In addition, the user interface of the bridge, which is controlled by Matter Labs, should inform users of the potential limitations in token functionality post-bridging.

Update: Resolved in pull request #140 at commit 5cbf05b, and in pull request #845 at commit 82232bc.

Inaccurate Attribution in LibMap.sol

In the LibMap.sol contract, specifically at line 5, the authorship is attributed to Solady. However, upon examination, it is clear that the code is not directly sourced from Solady but rather draws inspiration from their work. This misattribution could lead to confusion regarding the origins and the licensing of the code.

To maintain clarity and proper intellectual property acknowledgment, consider revising the comment to accurately reflect the nature of Solady's influence, perhaps indicating that the implementation is inspired by Solady's work rather than directly copied from there.

Update: Resolved in pull request #141 at commit 9712419.

Documentation Mismatch

Throughout the codebase, there are several documentation mismatches:

  1. In the Governance.sol contract, specifically within the cancel method, the documentation indicates that both the owner and the securityCouncil have the ability to cancel. However, the implementation restricts this action solely to the owner through the onlyOwner modifier. This means in the event of the owner account getting compromised, the securityCouncil cannot intervene.
  2. In the L2ContractHelper contract, the IL2Messenger interface is described for sending arbitrary-length messages from L2 to L1. It describes the four parameters senderAddress, isService, key, and value of the underlying operation, but then refers to isService as marker. Consider sticking to one word for consistency.
  3. In LibMap.sol, the _index parameter of the set function is inaccurately described. The documentation states "The index of the uint32 value to retrieve", whereas it is used for setting the value.
  4. The documentation of the verifierParams element of the ProposedUpgrade struct says that "If either of its fields is 0, the params will not be updated". However, in the _setVerifierParams function, the action of setting new parameters is only skipped when all parameters are zero. Consider correcting the documentation to match the implementation.

Update: Resolved in pull request #142 at commit a320b9b.

Upgrade Validation and Execution Is Not Strict

The BaseZkSyncUpgrade contract is an abstract contract used to perform standardized upgrades on the rollup. To ensure stricter execution and validation in this process, consider the following suggestions.

The DefaultUpgrade contract extends the BaseZkSyncUpgrade contract to invoke its functions from the overridden upgrade function using the values passed in through the ProposedUpgrade struct. In order to enforce that the new values will be set through this struct, and given that the BaseZkSyncUpgrade functions will only execute on non-zero inputs (otherwise it will be a no-op), consider moving the invoked functions from DefaultUpgrade.upgrade to BaseZkSyncUpgrade.upgrade to provide a stricter execution through the parent contract.

In addition, the _setVerifier function is used to set the verifier contract that will be used on L1 to verify the ZK proofs of the L2 rollup. As the documentation explains, batches that have been committed but not verified with the old verifier cannot be verified afterwards.

To prevent this mistake on a smart contract level, consider checking that the number of batches verified matches the number of the batches committed in the storage.

Update: Resolved in pull request #158 at commit b34ca08. The Matter Labs team stated:

We decided to not check that all the batches are verified, due to the following reasons:

  1. Firstly, it is not the only thing that needs to be checked. For instance, we need to ensure that all the batches with the old version have been committed prior to executing the upgrade. This cannot be done on-chain.
  2. The following scenario is possible:
    • We may start an upgrade (that fixes some theoretical issue with the verifier, i.e., the system does not change its logic but there is an edge case whereby a batch may become unprovable and we are trying to fix it).
    • Let's assume that we introduced a delay in upgrades (i.e., they are not instant). During the waiting period, the issue has been triggered. Meaning, we cannot achieve the "all committed blocks must be verified" requirement without the upgrade itself.
    • Now, in order to turn off the "all committed blocks must be verified" requirement, we will have to restart the waiting period (or do an emergency upgrade) to use the upgrade implementation that does not enforce it.

While the benefits are clear, it introduces edge cases that are rather easier to avoid by just always double-checking the requirements off-chain before triggering the upgrade.

Notes & Additional Information

Misleading Contract Name

The Governance contract, while descriptive of its operational role, can cause confusion given its name. The term "Governance" could be perceived as encompassing all governance aspects, such as voting, proposal submissions, and community decision-making. However, as the documentation suggests, the purpose of this contract is to act as a timelock for operations that change critical properties and functionalities of the protocol.

For clarity, consider naming the contract in a way that relates to its implementation (e.g., "TimelockedGovernor" or "UpgradeScheduler").

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

The current name was used because this contract is intended to serve as the governor of the main DiamondProxy. While it is true that switching to a name that better represents the functionality of the contract could be considered, the Governance contract has already been deployed. To avoid the risk-prone process of migrating the governance, and the discrepancies between the open-source repository and the deployed code, it is better to avoid renaming the contract at this point.

Multiple Instances of Missing Named Parameters in Mappings

Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType keyName => ValueType valueName). This updated syntax provides a more transparent representation of a mapping's purpose.

Throughout the codebase, there are multiple mappings without named parameters:

Consider adding named parameters to the mappings in order to improve the readability and maintainability of the codebase.

Update: Resolved in pull request #143 at commit 647a798, and in pull request #96 at commit 8b19ffc.

Incomplete Documentation of Differences Between WETH9 and L2Weth Contracts

The documentation for the L2Weth contract outlines its differences from the WETH9 contract. However, this list is not exhaustive. Key functionalities like the added depositTo and withdrawTo methods, along with the bridge functions, are missing.

To enhance clarity and accuracy, consider expanding the list of differences to include these additional features or rephrasing the documentation to indicate that the listed differences are not exhaustive.

Update: Resolved in pull request #144 at commit 0f5d29d. The depositTo and withdrawTo functions were added to the documentation as WETH9 differences. The bridge-related differences were not specifically mentioned, but these functions are not callable by users.

Unnecessary Cast

At line 328 of L1ERC20Bridge, the l2TokenBeacon variable is already an address type, but is unnecessarily cast explicitly as an address.

To improve the overall clarity, intent, and readability of the codebase, consider removing unnecessary casts.

Update: Resolved in pull request #145 at commit 7759538.

State Variable Visibility Not Explicitly Declared

Throughout both codebases, there are state variables that lack an explicitly declared visibility.

In the era-contracts repository:

In the era-system-contracts repository:

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

Update: Resolved in pull request #97 at commit 20a8fb7, and in pull request #146 at commit 93c019d.

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. Even interfaces and libraries, which may not be deployed in and of themselves, may be used by another protocol, which would need a security contact when coming across a vulnerability in the integration.

Throughout both codebases, the following libraries and interfaces do not have a security contact.

In the era-contracts repository:

In the era-system-contracts repository:

Consider placing a NatSpec comment containing a security contact above the contract, library, or interface 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 #149 at commit c6ed3fa, and in pull request #99 at commit f4a906e.

Lack of Indexed Event Parameters

Throughout the codebase, two events do not have their parameters indexed:

Consider indexing event parameters to improve the ability of off-chain services to search and filter for specific events.

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

Using Indexed parameters could indeed be preferred. However, at this point, it is better not to add them due to the following considerations:

  • These contracts are not upgradable. To upgrade ValidatorTimelock, we need to deploy a new one, introducing additional risks during the process of migrations as the commitment times between batches will not be migrated. The migration of governance is even more risk-prone and time-consuming.
  • These changes might be breaking for those who already track these events.

Unused Constants

The following constants were identified as being unused:

Consider clarifying whether these constants are necessary. Otherwise, consider removing them.

Update: Resolved in pull request #154 at commit 23a7381.

Non-Explicit Imports Are Used

The use of non-explicit imports in the codebase can decrease the code clarity, 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 codebases, global imports are being used. For instance:

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 #100 at commit 9926470, and in pull request #155 at commit df43911.

Literal Number With Many Digits

In the SystemContext contract, the block difficulty is defined as 2500000000000000. Literal numbers with many digits are hard to parse and harm the readability of the code.

Consider using the scientific notation instead.

Update: Resolved in pull request #101 at commit 0666f02.

Unused Import

The import of SystemContractHelper is not used throughout the KnownCodesStorage contract.

Consider removing this import to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #102 at commit 8152d5c.

Typographical Errors

The following typographical errors were identified throughout the codebases:

Update: Resolved in pull request #156 at commit 3d7cc8e, and in pull request #103 at commit 508f6e8.

Missing and Incomplete Docstrings

Throughout the codebases, some parts are missing docstrings while some docstrings are incomplete:

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 documented as well. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).

Update: Resolved in pull request #157 at commit efab770, and in pull request #104 at commit 16bfe51.

Function Is Updating the State Without Event Emissions

In the constructor of the ValidatorTimelock contract, the initial values for the executionDelay and validator state variables are set. However, the corresponding events, NewExecutionDelay and NewValidator, are not emitted at this point. While these state variables can optionally be changed later, and such changes are communicated through the events, the absence of initial values means that the complete history is not accurately reflected in the emitted events.

Consider emitting events in the constructor in order to enable off-chain indexers to track the complete history for these particular values.

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

The addition of these events is preferred. However, to upgrade ValidatorTimelock, we need to deploy a new one, introducing additional risks during the process of migrations as the commitment times between batches will not be migrated. So, to avoid additional risks with redeployment, or discrepancies between the open-source repo and the deployed code, it is better not to introduce these events at this point.

 

Conclusion

This system upgrade introduces important measures aimed at improving the safety of the protocol. Delaying the protocol changes through the Governance contract and the finalization of L2 batches through the ValidatorTimelock contract allows the stakeholders to act on any suspicious activity in time. Of course, such measures are only as safe as the implementation of privileged roles and security properties, which is still under exploration for the security council.

The risk of vulnerabilities in yarn dependencies and the overwriting of token metadata have been presented as medium-severity issues. Other than that, we found the code quality to be good and only reported low-severity issues and improvement recommendations. Throughout the audit period, the Matter Labs team was helpful and responsive in answering our questions.

Related Posts