We diff audited the matter-labs/era-contracts repository at BASE commit f29f2b7 and HEAD commit d5c5d7a.
In scope were the following files:
l1-contracts/
└─ contracts/
└─ state-transition/
├─ IStateTransitionManager.sol
├─ StateTransitionManager.sol
├─ ValidatorTimelock.sol
├─ chain-deps/
│ ├─ DiamondInit.sol
│ ├─ DiamondProxy.sol
│ ├─ ZkSyncStateTransitionStorage.sol
│ └─ facets/
│ ├─ Admin.sol
│ ├─ Executor.sol
│ ├─ Getters.sol
│ ├─ Mailbox.sol
│ └─ ZkSyncStateTransitionBase.sol
├─ l2-deps/
│ └─ ISystemContext.sol
└─ libraries/
└─ TransactionValidator.sol
This upgrade introduces Matter Labs' latest development efforts of implementing the ZK Stack. More specifically, this architectural overhaul requires abstractions to the Layer 1 (L1) contracts to enable deploying so-called ZK Chains, which are ZK rollup instances just like the Era chain.
The following is a summary of the changes.
StateTransitionManagerThis new contract is responsible for deploying and managing ZK Chains. It has two roles: the owner who is fully in charge and the admin who is in charge of a subset of the functions. ZK Chains can be created by the BridgeHub address. This deploys a DiamondProxy contract and initializes it with a diamond cut. It also upgrades the chain through the Admin facet right away to set the given chain ID in the SystemContext contract on the L2 side.
Regarding managing features, the owner is capable of defining how the ZK Chains are initialized and upgraded. Upgrades are also defined through diamond cuts. These can be set for any protocol version while also maintaining the protocol version state variable of the contract as a source of truth for its ZK Chains. This means for an upgrade from version X to Y, the diamond cut to set the chain's version to Y is defined for version X. This allows flexibility for consecutive upgrades but also laggard ZK Chains to skip versions. Other capabilities involve freezing and unfreezing a specific chain, reverting a chain's batches, and setting the ValidatorTimelock address.
ValidatorTimelockThe ValidatorTimelock is a contract set as a validator for a ZK Chain. It supports the interface of the Executor's state-transition functions (commit, prove, revert, execute) and forwards the call to the ZK Chain, while making sure that a delay between committing and executing has passed. This upgrade has generalized the contract to be suitable for all ZK Chains of a StateTransitionManager. Hence, given the chain ID, the call is forwarded to the respective ZK Chain (i.e., DiamondProxy). Validators that can call the state-transition functions of the ValidatorTimelock can be set per chain ID by their admin. For backwards compatibility, there is a redundancy in the interface to be chain-ID-specific for the new ZK Chains, and non-chain-ID-specific for the existing Era chain. This change in interface is reflected in the Executor facet as well.
Admin: Most of the changes accommodate the role change from the deprecated Governor to the StateTransitionManager. New features of the facet include:
Rollup and Validium mode. In Validium mode, no pubdata is committed to the Executor. More about it in the ZKsync documentation.Mailbox, the ETH price needs to be converted into the base token currency given the defined nominator and denominator.StateTransitionManger may execute the predefined diamond cut as an upgrade from the current version of the chain, as outlined above. The StateTransitionManager contract is additionally entitled to perform any diamond cuts, as done for the chain ID upgrade after initialization.Executor: The Executor now supports the aforementioned Validium mode that does require empty Pubdata as part of the committed batch data. Furthermore, it extends the interface of the state-finalization functions (commit, prove, revert, execute) to accommodate the ValidatorTimelock usage.
Getters: This facet was simply extended with functions to query the new storage variables that are part of the other facets' changes.
Mailbox: The Mailbox was changed to support the interactions with the BridgeHub and shared bridge. For backwards compatibility with the Era chain specifically, the Mailbox still supports being called directly to finalize a withdrawal, although it is just forwarding the call to the shared bridge. Also, the BridgeHub may directly request L2 transactions. Lastly, the L2 gas price calculations take into account the aforementioned nominator and denominator to convert the ETH price into the base token.
This upgrade has shifted some roles. Below, we list each role's capabilities per contract:
StateTransitionManager:
setPendingAdmin (owner, admin): Setting the pending admin who can accept the role by calling acceptAdminsetValidatorTimelock (owner, admin): The ValidatorTimelock contract address that will be set up as the only validator during ZK Chain initializationsetInitialCutHash (owner): Setting the hash of the diamond cut that needs to be provided during ZK Chain initializationsetNewVersionUpgrade (owner): Setting a diamond cut to be upgraded from a version while also updating the state's protocol versionsetUpgradeDiamondCut (owner): Setting a diamond cut to be upgraded from a versionfreezeChain (owner): Freezing a specific chainunfreezeChain (owner): Unfreezing a specific chainrevertBatches (owner, admin): Reverting batches for a specific chainregisterAlreadyDeployedStateTransition (owner): Adding an arbitrary address to the mapping of chain IDs and their respective DiamondProxy addresscreateNewChain (BridgeHub): Creating a new ZK Chain that is managed by the StateTransitionManagerNote that the StateTransitionManager's owner is in charge of defining all upgrades for its ZK Chains. This role is said to be held by the Governance contract which is operated by a multisig wallet and security council. During the audit, we raised concerns to the Matter Labs team about potential pitfalls with respect to the protocol version that could be introduced during the upgrade process. However, as more flexibility is desired, it is expected that the Governance role does its due diligence for the upgrade's correctness in the best interest of the user base.
ValidatorTimelock:
setStateTransitionManager (owner): Setting the StateTransitionManager to be used to query the admin and address of a ZK Chain by chain IDaddValidator, removeValidator (chain admin): Add/remove validators that may interact with their chain ID's ExecutorsetExecutionDelay (owner): Define the minimum delay that must be waited between committing and executing batchesThe Admin facet of a ZK Chain:
setPendingAdmin (admin): Setting the pending admin who can accept the role by calling acceptAdminsetValidator (StateTransitionManager): Setting the validator status of the ZK ChainsetPorterAvailability(StateTransitionManager): Setting the zkPorter availabilitysetPriorityTxMaxGasLimit (StateTransitionManager): Setting the maximum L2 gas limit for L1 to L2 transactionschangeFeeParams (admin, StateTransitionManager): Setting the fee parameters for L1 to L2 transactions for the ZK Chain, including PubdataPricingMode, batchOverheadL1Gas, maxPubdataPerBatch, maxL2GasPerBatch, priorityTxMaxPubdata, and minimalL2GasPricesetTokenMultiplier (admin, StateTransitionManager): Setting the token multipliers that convert the L1 base token (ETH for Ethereum mainnet) into the L2 base tokensetValidiumMode (admin): Setting the PubdataPricingMode in the fee parameters to be either Validium or RollupupgradeChainFromVersion (admin, StateTransitionManager): Upgrading the diamond cut from a specific version as defined in the StateTransitionManagerexecuteUpgrade (StateTransitionManager): Executing an arbitrary upgrade by the StateTransitionManagerfreezeDiamond (admin, StateTransitionManager): Instantly pausing the functionality of all freezable facets and their selectorsunfreezeDiamond (admin, StateTransitionManager): Unpausing the functionality of all freezable facets and their selectorsFor all roles and capabilities, it is expected that the parties in charge always act in the best interest of the user base and the ecosystem.
StateTransitionManager Cannot Unfreeze ChainsThe StateTransitionManager is privileged to freeze and unfreeze the chains it manages. This is intended by calling the respective function in the Admin facet. The problem is that the unfreezeChain function of the StateTransitionManager mistakenly also calls the freezeDiamond function, thereby leading to a revert. To recover from this frozen state, the StateTransitionManager is dependent upon the chain admin who may also call the unfreezeDiamond function.
Consider correcting the unfreezeChain function to allow the StateTransitionManager to unfreeze any frozen ZK Chain instead of reaching out to the ZK Chain admin to do the unfreezing. Also, consider implementing a unit test for this functionality.
Update: Resolved in pull request #293.
PubdataPricingMode ConfigurationThe setValidiumMode function of the Admin facet allows the admin role to toggle the pubdata pricing mode between Rollup and Validium as long as no batches are committed. While the function name only suggests that the mode can be changed from Rollup to Validium, the mode can freely be set (e.g., from Rollup to Validium and back to Rollup). Furthermore, the comment "Validium mode can be set only before the first batch is committed" is wrong for two reasons:
PubdataPricingMode can be set through the changeFeeParams function at any time.Thus, when users request an L2 transaction through the Mailbox facet, they sometimes could get charged for L1 PubData, and sometimes not. Moreover, if the Validium mode is motivated by enterprise or privacy reasons, a mode change to Rollup would entail leaking sensitive data.
Consider clarifying the intention of when and how the PubdataPricingMode may be changed. For example, ensure that the PubdataPricingMode can only be changed until the first batch is written to storedBatchHashes, which is not deleted during a batch revert. In addition, consider checking that the mode is not changed during changeFeeParams.
Update: Resolved in pull request #292 and pull request #298. The Matter Labs team stated:
PubdataPricingModecan be changed only before the first batch is processed, this is also reflected in the comment and in the doc-comments introduced with the fix in L-06.
In the StateTransitionManager, the acceptAdmin function allows the pending admin to become the admin of the contract. This finally leads to the emission of the NewAdmin event. However, this event emits the old admin through the previousAdmin stack variable and the just-deleted pendingAdmin (address(0)) as the new admin. This makes it significantly more difficult for off-chain clients to track the admin role.
Consider emitting the currentPendingAdmin stack variable as the new admin instead.
Update: Resolved in pull request #294.
In the StateTransitionManager, the following functions change the storage of the contract but do not emit an event:
This updated information includes sensitive configuration such as how the chains are upgraded for a new version. Therefore, consider emitting an event for more transparency and better monitoring capabilities.
Update: Resolved in pull request #295.
StateTransitionManager Role-Specific Functions Cannot Be Reached in Admin FacetIn the Admin facet, some functions are limited only to be called by the StateTransitionManager. For instance:
These functions will not be accessible as there are no such function calls from the StateTransitionManager contract. Furthermore, the following functions are callable by both the admin and the StateTransitionManager role, but currently not callable through the StateTransitionManager contract:
Consider adding these function calls to the StateTransitionManager contract to facilitate the StateTransitionManager role in changing the configuration of registered ZK Chains.
Update: Resolved in pull request #296 at commit 6c0c01c.
Throughout the codebase, the following instances were identified where stronger validation can be applied:
Admin facet, the setTokenMultiplier function allows the admin or StateTransitionManager to set the nominator and denominator which is used by the Mailbox to scale the L1 gas price. While the denominator can be freely set, the value 0 would lead to a revert in the Mailbox for any requested L2 transaction. Consider applying the same check when setting the value.StateTransitionManager, the setNewVersionUpgrade function allows the owner to set a diamond cut for an old protocol version. Simultaneously, the new protocol version argument overwrites the protocolVersion of the StateTransitionManager and thereby determines which version the diamond cut upgrades to. The problem is that the diamond cut also contains a protocolVersion in the encoded ProposedUpgrade struct as the initCalldata that then sets the ZK Chain's s.protocolVersion. It is possible that the encoded protocolVersion is not consistent with the new protocol version from the function argument. This mismatch would lead to all ZK Chains of the StateTransitionManager not being able to commit more batches to their Executor facet due to a version check. Consider decoding the diamond cut argument to validate that the protocol versions of the upgrade align.DiamondInit contract initializes the storage of a ZK Chain. This includes the addresses of the verifier, admin, and validatorTimelock that are checked to not be zero. However, with the recent upgrade, more addresses were introduced: bridgehub, stateTransitionManager, baseToken, baseTokenBridge, and blobVersionedHashRetriever. These addresses are not checked to not be zero. While some of these addresses originate from trusted sources like the BridgeHub or the StateTransitionManager, consider double checking them for consistency with the existing addresses and guaranteeing the functional correctness of the chain.StateTransitionManager can set arbitrary addresses as registered ZK Chain contracts per chain ID. This may include overwriting a chain ID with the zero address. By the logic of this contract, this could allow creating a new chain for this ID by passing the zero address check. Although, this it not allowed by the BridgeHub in charge, consider checking that upon registration of already deployed ZK Chains, the address cannot be set to zero.Update: Partially resolved in pull request #297. The Matter Labs team stated:
Partially fixed. We decided not to apply the suggestion for
setNewVersionUpgradeto avoid additional complexity.
PubdataSource Does Not Account for Validium ModeThe Executor facet can handle two sources of public data inputs when committing batch information from L2: calldata and EIP-4844 blobs. This data is expected to be present when the chain operates in Rollup mode. However in Validium mode, no public data must be present. Despite operating in Validium mode, the first byte of pubdataCommitments, which indicates the public data input source, is required to be either 0 or 1 for calldata and blob source respectively. This is not accurate as the commitment data is supposed to be stored elsewhere off-chain.
For explicitness, consider extending the PubdataSource enum with None or Validium and checking the first pubdataCommitments byte against it when the chain operates in Validium mode. In addition, be sure to provide a meaningful revert message when pubdataCommitments does not meet a length of 1.
Update: Resolved in pull request #299. The Matter Labs team stated:
We added the third option (validium) case to the require statement.
We did not want to add the Validium/None case to the PubdataSource options, since the PubdataSource is a different concept from DA mode. DA mode refers to the fact that DA needs to be published or not, while PubdataSource is just the form of publishing it (if needed). This is shown by the fact that we have to read ValidiumMode from storage (since that depends on the chain permanently), while Pubdata source can be read from the function input, since it can change batch to batch.
We also updated the error message.
Throughout the codebase, there are multiple code instances that do not have docstrings.
upgradeChainFromVersion and setValidiumMode functions in Admin.solregisterAlreadyDeployedStateTransition and createNewChain (_diamondCut parameter especially) functions in StateTransitionManager.solConsider 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). Furthermore, the comment "new fields" in ZkSyncStateTransitionStorage.sol could be more meaningful (e.g., by specifying a version).
Update: Resolved in pull request #298.
The following imports are unused:
ERA_DIAMOND_PROXY and ERA_CHAIN_ID in StateTransitionManager.solUnsafeBytes, L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR, and IBridgehub in Mailbox.solFeeParams and ISystemContext.sol in DiamondInitConsider removing unused imports for improved code clarity.
Update: Resolved in pull request #300.
Throughout the codebase, there are several instances of incorrect documentation:
StateTransitionManager, the onlyOwnerOrAdmin modifier reverts with the message "Bridgehub: not owner or admin" on failed authorization, thus indicating the wrong contract.upgradeChainFromVersion function indicate the main (Diamond) contract "StateTransition: ..." as the error source, whereas other facets usually indicate the facet as a source. Thus, consider changing the message to "Admin: ...".requestL2Transaction function says "legacy interface only available for era token", while probably "era chain" is meant.onlyBaseTokenBridge modifier does not specify the contract, whereas the other revert strings in ZkSyncStateTransitionBase do._requestL2Transaction function, the new scope as described in the comment "Using a new scope to prevent 'stack too deep' error" is not present anymore._deriveL2GasPrice function refer to ETH, although the price is calculated in the base token.nonce for the L2CanonicalTransaction when creating a new chain says that the nonce is "the priority operation id" whereas it is the protocolVersion.StateTransitionManagerInitializeData struct parameters have leading underscores whereas the fields have no underscores.Consider applying the above changes for a more correct documentation of the codebase.
Update: Resolved in pull request #304.
When the StateTransitionManager is initialized, a StoredBatchInfo struct is declared and hashed for later use.
For better readability, consider declaring the struct with the more explicit key: value syntax as seen in the _setChainIdUpgrade function.
Update: Resolved in pull request #306.
Throughout the codebase, the following instances of redundant code were identified:
Mailbox facet, two structs are handled: BridgehubL2TransactionRequest and WritePriorityOpParams. The former reflects the user input of the transaction, while the latter handles the additional fields txId, expirationTimestamp, and l2GasPrice that are derived during scheduling of the priority operation.WritePriorityOpParams could simply be extended by a BridgehubL2TransactionRequest request field. Throughout the usage of the WritePriorityOpParams struct, its values are always read explicitly so that this change should not introduce any repercussions (e.g., from hashing the whole struct). Furthermore, the interface of the internal functions _requestL2Transaction, _serializeL2Transaction, and _writePriorityOp can be simplified as all arguments are given in the WritePriorityOpParams struct.onlyValidator modifier of the ValidatorTimelock has a slight redundancy of checking the mapping entry against true as it could also be evaluated directly.ValidatorTimelock functions commitBatches and commitBatchesSharedBridge as well as executeBatches and executeBatchesSharedBridge implement the same function body with the only difference of the _chainId. Instead, consider making the [...]SharedBridge functions' body an internal function that is called by their respective external functions by forwarding the _chainId argument or specifying the ERA_CHAIN_ID. This would ease maintenance, be less error-prone, and clarify the functional difference of the interfaces.getBridgehub, getStateTransitionManager, getBaseToken and getBaseTokenBridge functions.Update: Resolved in pull request #308.
Throughout the codebase, the following typographical errors were identified:
Executor.sol, a comment says "With the protocol upgrade we expect 8 logs: 2^10 - 1 = 1023", meaning 10 logs instead of 8.StateTransitionManager, the comment "Cannot do it an initialization" should say "during initialization".Update: Resolved in pull request #303.
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:
stateTransition state variable in the StateTransitionManager contract.upgradeCutHash state variable in the StateTransitionManager contract.committedBatchTimestamp state variable in the ValidatorTimelock contract.Consider adding named parameters to the mappings to improve the readability and maintainability of the code.
Update: Resolved in pull request #305.
int/uint Instead of int256/uint256Within Executor.sol, int/uint is being used instead of int256/uint256.
In favor of explicitness, consider replacing all instances of int/uint with int256/uint256.
Update: Resolved in pull request #290 at commit 8d771cc.
Within ValidatorTimelock.sol, two events do not have indexed parameters:
To improve the ability of off-chain services to search and filter for specific events, consider indexing event parameters.
Update: Resolved in pull request #309. The Matter Labs team stated:
We only added indexes to first params, which are "chainId".
Throughout the codebase, the following naming suggestions are seen as more accurate:
DiamondProxy contracts that have access to facets just like the Era chain. However, the term seems to be closer to the Executor facet's functionality of handling the L2 state, which is just one aspect of the chain. Therefore, the more general term "ZK Chain" appears to be more fitting to refer to the DiamondProxy contract. Consider replacing "StateTransition" with "ZK Chain" in identifiers that use it.setValidiumMode function suggests that the mode can only be changed from Rollup to Validium, although it can be freely set. Therefore, consider naming the function setPubdataPricingMode.StateTransitionManagerInitializeData struct is used to initialize the StateTransitionManager. However, the governor address is used to set up the owner role of the contract, which can be confusing. For clarity, consider calling this field owner instead.s.baseTokenBridge is sometimes referred to as sharedBridge (e.g., in the Mailbox facet as well as in the error message in ZkSyncStateTransitionBase). To avoid confusion, consider using a consistent name for the same bridge.Update: Resolved in pull request #307 and pull request #310. Most of the naming suggestions were applied, while the StateTransitionManager contract name and its references were left unchanged. The Matter Labs team stated:
In the future when there are multiple STMs each one will manage a single zone of ZK Chains, and what each zone will have in common is their State Transition function. The STMs do manage everything about the chains (I agree on this fact), but in the bigger picture the different zones will be zones of State Transitions, while every chain under the Bridgehub will be a ZK Chain. To me calling them ZK ChainManager would imply that they manage all ZK Chains, which is not true.
payableThe bridgehubRequestL2Transaction function of the Mailbox facet is callable by the BridgeHub to forward a user's L2 transaction request. The calls to this function are done without forwarding any value since the shared bridge is intended to hold all assets. As such, there is no reason for this function to be payable.
Consider removing the payable keyword from the function selector in order to reduce the chances of getting ETH locked in the ZK Chains.
Update: Resolved in pull request #301.
The StateTransitionManager is an upgradeable contract to be used behind a proxy. It extends the Ownable2Step contract for the owner role management.
Despite the StateTransitionManager being upgradeable, Ownable2Step is imported from @openzeppelin/contracts, not @openzeppelin/contracts-upgradeable. The difference is that the contract has no storage gap and would usually be set up through the constructor. This is not a problem as the initial owner is set up in the initialize function and as long as the storage layout of the extended contracts do not shift, which is unlikely for the Ownable2Step and Ownable contract.
However, to be safe and to adhere to the best practice of using storage gaps, consider using the contracts-upgradeable version of Ownable2Step.
Update: Resolved at commit 4befd8a.
This system upgrade has introduced various abstractions to the L1 contracts that will enable Ethereum scalability through ZKsync ZK Chains. The new StateTransitionManager is the central piece used to spawn and manage these ZK Chains. Further changes to the existing facets were made to accommodate the overall architectural change of bridging assets.
The audit yielded two medium-severity issues. One simple mistake causes the StateTransitionManager to not be able to unfreeze chains, while the other problem lies in the ambiguity of the Pubdata pricing mode configuration. Among these, multiple lower-severity issues have been raised and recommendations made. We appreciate Matter Labs' helpful input to our questions throughout this engagement.