OpenZeppelin conducted a differential audit of the matter-labs/era-contracts repository against the base commit 7278847. The audit was performed in two phases:
Phase 1: Reduced Interoperability feature at head commit 3092fc0.
In scope were the following files:
l1-contracts
├── contracts
│ ├── bridgehub
│ │ ├── IMessageRoot.sol
│ │ ├── L2MessageVerification.sol
│ │ └── MessageRoot.sol
│ ├── common
│ │ ├── Messaging.sol
│ │ ├── interfaces
│ │ │ └── IL2InteropRootStorage.sol
│ │ └── libraries
│ │ └── MessageHashing.sol
│ └── state-transition
│ ├── chain-deps
│ │ └── facets
│ │ ├── Executor.sol
│ │ ├── Mailbox.sol
│ │ └── MessageVerification.sol
│ └── chain-interfaces
│ ├── IExecutor.sol
│ ├── IMailbox.sol
│ ├── IMailboxImpl.sol
│ └── IMessageVerification.sol
system-contracts
├── bootloader
│ └── bootloader.yul
└── contracts
└── L2InteropRootStorage.sol
Phase 2: Fast Finality feature and other smaller changes in several parts of the system at head commit 903cfed2.
In scope were the following files:
l1-contracts
├── contracts
│ ├── bridge
│ │ └── L1BridgeContractErrors.sol
│ ├── bridgehub
│ │ ├── Bridgehub.sol
│ │ ├── CTMDeploymentTracker.sol
│ │ ├── ChainAssetHandler.sol
│ │ ├── IBridgehub.sol
│ │ ├── IChainAssetHandler.sol
│ │ └── L1BridgehubErrors.sol
│ ├── chain-registrar
│ │ └── ChainRegistrar.sol
│ ├── common
│ │ ├── Config.sol
│ │ ├── L1ContractErrors.sol
│ │ ├── Messaging.sol
│ │ ├── interfaces
│ │ │ └── IL1Messenger.sol
│ │ ├── l2-helpers
│ │ │ ├── IL2ToL1Messenger.sol
│ │ │ ├── L2ContractAddresses.sol
│ │ │ ├── L2ContractHelper.sol
│ │ │ └── SystemContractsCaller.sol
│ │ └── libraries
│ │ └── MessageHashing.sol
│ ├── governance
│ │ ├── PermanentRestriction.sol
│ │ └── ServerNotifier.sol
│ ├── state-transition
│ │ ├── AccessControlEnumerablePerChainAddressUpgradeable.sol
│ │ ├── ChainTypeManager.sol
│ │ ├── IChainTypeManager.sol
│ │ ├── IValidatorTimelock.sol
│ │ ├── L1StateTransitionErrors.sol
│ │ ├── ValidatorTimelock.sol
│ │ ├── chain-deps
│ │ │ ├── DiamondInit.sol
│ │ │ ├── GatewayCTMDeployer.sol
│ │ │ ├── ZKChainStorage.sol
│ │ │ └── facets
│ │ │ ├── Admin.sol
│ │ │ ├── Executor.sol
│ │ │ └── ZKChainBase.sol
│ │ ├── chain-interfaces
│ │ │ ├── IAdmin.sol
│ │ │ ├── IDiamondInit.sol
│ │ │ └── IExecutor.sol
│ │ ├── l2-deps
│ │ │ └── IL2GenesisUpgrade.sol
│ │ ├── libraries
│ │ │ └── BatchDecoder.sol
│ │ └── verifiers
│ │ ├── L1VerifierFflonk.sol
│ │ └── L2VerifierPlonk.sol
│ ├── transactionFilterer
│ │ └── GatewayTransactionFilterer.sol
│ └── upgrades
│ ├── BaseZkSyncUpgrade.sol
│ ├── BaseZkSyncUpgradeGenesis.sol
│ ├── IL2V29Upgrade.sol
│ ├── L1V29Upgrade.sol
│ ├── UpgradeStageValidator.sol
│ └── ZkSyncUpgradeErrors.sol
system-contracts
├── bootloader
│ └── bootloader.yul
├── contracts
│ ├── Constants.sol
│ ├── EvmEmulator.yul
│ ├── EvmPredeploysManager.sol
│ ├── L2GenesisForceDeploymentsHelper.sol
│ ├── L2InteropRootStorage.sol
│ ├── L2UpgradeUtils.sol
│ ├── L2V29Upgrade.sol
│ ├── PubdataChunkPublisher.sol
│ ├── SystemContext.sol
│ ├── SystemContractErrors.sol
│ ├── interfaces
│ │ ├── IBridgedStandardERC20.sol
│ │ ├── IBridgehub.sol
│ │ ├── IL2AssetRouter.sol
│ │ ├── IL2GenesisUpgrade.sol
│ │ ├── IL2NativeTokenVault.sol
│ │ └── IPubdataChunkPublisher.sol
│ └── precompiles
│ ├── CodeOracle.yul
│ ├── EcAdd.yul
│ ├── EcMul.yul
│ └── Modexp.yul
└── evm-emulator
├── EvmEmulatorLoop.template.yul
└── EvmEmulatorLoopUnusedOpcodes.template.yul
Update: During the fix review period the pull requests #1538 and #1540 have also been investigated. All of the reported issues have been fully resolved at commit babbed5.
Until now, L2 ZKChains were only able to communicate with their settlement layers. The Reduced Interoperability feature enables cross-chain plain message passing among the ZKChains which settle on Gateway. The developed mechanism sets the base for a future release of full interoperability features, where actual value transfer among the L2 ZKChains will be supported.
Reduced Interoperability can be seen as an automation mechanism where the commitments of the settled batches on Gateway are shared with and stored on all ZKChains. Thus, in order to prove on a ZKChain B that a specific transaction took place on a ZKChain A, a user should submit a valid Merkle Proof of the transaction log inclusion against the commitment of the settled batch containing the transaction.
A step-wise description of the basic flow of the new mechanism is given below:
chainTree is updated by appending the new batch leaf. This results in a new chainRoot and so the corresponding leaf in the sharedTree changes, resulting in an updated sharedTree root.sharedTree root reflects all the settled ZKChain batches within that Gateway batch.sharedTree root within a Gateway batch is an interop root. An external node monitors the network for NewInteropRoot events and broadcasts the new interop root to all ZKChains that settle on Gateway.L2InteropRootStorage contract.sharedTree on Gateway. Otherwise, the batch is reverted.The Fast Finality feature enables faster finality times for ZKChains that settle on Gateway. Nodes can monitor for pre-commitments on L1 instead of waiting for Gateway batch commitment, proving, and execution (which is gas-intensive for the operator to do too often) on L1. This allows third-party services, such as exchanges supporting ZKChains, to offer faster settlement leading to an improved user experience.
On-chain, this is achieved by "pre-committing" to L1 the ordering, content, and execution status of transactions processed on Gateway. Specifically, each transaction hash is appended, along with its execution status, to a rolling hash and stored on L1 during pre-commitment. This rolling hash is then validated against the rolling hash of Gateway transactions computed in the bootloader during batch commitment, reverting the commitment if the two do not match. Practically, this means that the consumers of these pre-commitments (e.g., exchanges) can rely on these Gateway transactions being executed later in the batch in a certain order and with a certain status. Otherwise, the chain would stall. Since Gateway transactions contain the commitment, proving, and execution of ZKChain batches, consumers can infer the state of these ZKChains.
In addition, the right to revert committed batches and pre-commitments on Gateway will be transferred from the operator to the security council. This means that pre-commitments can only be reverted by the security council. More generally, a flexible mechanism to manage the different roles (pre-commit, commit, proof, execution, and revert) was introduced through the ValidatorTimelock contract.
As noted earlier, the interop feature is currently reliant on ZKChain operators including the new interop root. However, it should be noted that because the trees are append-only, messages can only be censored by way of the operator refusing to sync any new root. Regarding pre-commitments, it is important for consumers to understand what they are and what are the provided guarantees.
Pre-commitments guarantee the order and execution status of Gateway transactions. While this guarantee is powerful, as these transactions are expected to be deterministic, the Gateway operator remains a source of non-determinism because of its ability to influence variables such as the block timestamp, the block number, the block hash, or the inclusion of interop roots. It is thus important for the pre-commitment consumers to ensure that all the transactions which are pre-committed are deterministic, even in the worst case of an adversarial operator. For example, a transaction executing a smart contract that executes a ZKChain batch only if the block.timestamp is even should not be relied upon because the timestamp can be later manipulated to fulfill the promise without executing the batch. Generally, the consumers of pre-commitments are recommended to ensure that all pre-committed transactions were sent by EOAs to whitelisted contracts (e.g., Executor.sol) to mitigate the risk of such scenarios from materializing.
Note that it is always possible for the operator to submit random data as part of pre-commitments, which would force the intervention of the security council in order to revert those. All the data should thus always be validated to correspond to valid transactions. Additionally, pre-commitments can be invalidated by chain upgrades.
The following trust assumptions were made during the audit:
ValidatorTimelock contract will behave honestly.bootloader Initialization ValuesThe bootloader uses multiple variables to track its progress when processing interop roots:
LAST_PROCESSED_BLOCK_NUMBERCURRENT_NUMBER_OF_ROOTS_IN_BLOCKCURRENT_INTEROP_ROOTHowever, these variables are not validated to be initialized to 0. This lack of validation makes it possible for the operator to deviate from the intended operation of the bootloader in various ways. For example, the operator could set LAST_PROCESSED_BLOCK_NUMBER or CURRENT_NUMBER_OF_ROOTS_IN_BLOCK to a non-zero value to ignore some roots, or set CURRENT_INTEROP_ROOT to a very high value to create a buffer overflow and read roots from beyond the array bounds. Such attacks could result in a range of outcomes, including the forgery of incorrect interop roots, which could lead to user funds being stolen by the operator.
Consider ensuring that these variables are initialized properly during the execution of the bootloader.
Update: Resolved in pull request #1505 at commit 14571a2. All the variables are explicitly initialized to 0.
Messages can be sent across ZKChains by calling the sendToL1 function of the L1Messenger contract. Such messages are aggregated in the L2_TO_L1_MERKLE_TREE and sent to the settlement layer at the end of the execution of the bootloader on a batch. The synchronization of the Merkle trees coming from different ZKChains sharing the same settlement layer can then be ensured by the operator of a ZKChain when executing the bootloader on a batch. The operator can add interop roots within the batch's data so that they are stored in the L2InteropRootStorage contract on the ZKChain.
To ensure that valid interop roots are synchronized, the stored roots are compared against the roots stored on the settlement layer during settlement. Cross-chain messages can then be proven on the destination ZKChain by providing a Merkle proof against an interop root. However, there is a flaw in the bootloader's memory management design making it possible for the operator to circumvent the interop root validation mechanism. Specifically, within the bootloader contract, the loop setting the interop roots and the loop validating them can iterate over different slices of the INTEROP_ROOTS array.
For example, a malicious operator could pass an interop root with a sides length of 0 as the first interop root and initialize CURRENT_INTEROP_ROOT to 1. The setInteropRootForBlock function would then iterate over the roots at indexes [1:100], while the validation loop would loop over index [0] and exit early. The settlement would complete successfully as dependencyRootsRollingHash is 0 as if no new roots had been added. This design flaw allows the operator to forge arbitrary roots.
The core of the issue is that the domains of the loops in which interop roots are set and validated can be different. To address this issue, consider explicitly merging the aforementioned loops into a single loop to ensure that any interop root added is always validated.
Update: Resolved in pull request #1521 at commit 2aae32a. The rolling hash validated during settlement is now computed when setting the interop roots on L2.
interopRoots in bootloader::setInteropRootForBlockWithin the bootloader contract, specifically in the setInteropRootForBlock function, there is an inconsistency in the indexing methodology applied to interopRoots. The getNumberOfInteropRootInCurrentBlock function is designed to return the total number of interopRoots for a given block. This value appears to represent the count of interopRoots included in the block, without any reference to the sequential order of roots being processed in the entire batch.
In contrast, nextInteropRootNumber is utilized as an index that accounts for the sequential order of interopRoots within the entire batch. This discrepancy in indexing approaches—where one value represents an absolute count within a single block and the other signifies a sequential index across the entire batch—leads to a fundamental inconsistency. Comparing these two values directly in the for loop condition is illogical due to their differing contextual bases. This inconsistency could lead to only storing part of interopRoots set in the L2InteropRootStorage contract.
To address this issue and ensure clarity and correctness in the processing of interopRoots, consider standardizing the indexing approach to consistently reflect either an absolute count within a block or a sequential index within the batch.
Update: Resolved in pull request #1501 at commit 4852fe8. The loop now iterates over the correct range.
bootloaderWhen processing interop roots, the bootloader is responsible for iterating over several variables stored in its memory. During this process, the bootloader iterates through each transaction, fetches the associated block number, and retrieves the number of roots stored in INTEROP_BLOCKS. Then, the bootloader reads that many elements from the INTEROP_ROOTS array, which contains multiple fields per root: the processed block, the chain ID, the dependency block, the sides length, and the sides data.
However, when processing a root, the "processed block" associated with the current root is compared to the current block and loop breaks early if the processed block is above the current block. This situation should never occur, as it suggests that an incorrect number of roots was set in INTEROP_BLOCKS. This could allow the operator to set a very high processed block value and ignore some roots.
At a high level, this issue results from information being duplicated between the number of roots in a block and the block associated with each root. As such, consider either removing the duplicated information by only keeping the number of roots or reverting when an incorrect block number is encountered.
Update: Resolved in pull request #1523 at commit 518305e. The bootloader will now revert if a root is encountered with an inconsistent block number.
The MessageVerification contract is designed to facilitate the verification of messages or logs from ZKChains, enhancing cross-chain communication capabilities. This contract is extended by the Mailbox facet on settlement layers and the L2MessageVerification contract on ZKChains. The Mailbox facet already encapsulates the necessary functionality for message verification on settlement layers, rendering the proving functions of MessageVerification redundant in such contexts.
The redundancy becomes problematic when, due to a discrepancy in handling the chainId of the sending ZKChain, the proving functions within MessageVerification are invoked instead of those in Mailbox on a settlement layer. Mailbox internally determines chainId, ensuring consistency and security in message verification. Conversely, MessageVerification requires chainId as an external argument, opening the door to inconsistencies and incorrect parameter submissions. This discrepancy undermines the security and reliability of cross-chain message verification, as illustrated in the following scenarios:
A message from ZKChainA is correctly proven within its Diamond proxy on Gateway, but using the chainId of ZKChainB. Despite the incorrect chainId, the proof succeeds due to accurate proof data and the non-reliance on the erroneous chainId.
Proving a message from ZKChainA on L1 within the Diamond Proxy of ZKChainB also leads to a successful proof. This is because the chainId of ZKChainA is provided externally and the proof data is accurate, with the final batch root hash verification occurring on Gateway's Diamond Proxy, which does not differentiate between ZKChains settling on it.
Consider introducing additional validation logic to detect and reject proofs submitted with inconsistent parameters, especially focusing on scenarios where chainId could be misused. Specifically, consider always verifying that the ZKChain's chainId for which the message is proven is the expected one according to the context.
Update: Resolved in pull request #1525 at commit 6f9abb0. The shared functions used for message verification were marked as virtual in MessageVerification, and overridden by functions validating the chainId in Mailbox.
In the bootloader and MessageRoot contracts, certain names do not accurately reflect the data they represent, leading to potential confusion.
In bootloader.yul, the CURRENT_NUMBER_OF_ROOTS_IN_BLOCK_SLOT and CURRENT_NUMBER_OF_ROOTS_IN_BLOCK_BYTE variables misleadingly suggest that they represent the number of roots, whereas they actually track the number of blocks processed. This discrepancy between the variable names and their actual purpose can lead to misunderstanding about the contract's functionality. Similarly, the INTEROP_BLOCKS_BEGIN_SLOT and INTEROP_BLOCKS_BEGIN_BYTE variables actually count the number of roots per block. Apart from this, the interopRootStartSlot variable could be renamed to interopRootStartByte to maintain slot vs. byte naming consistency.
In MessageRoot.sol, the Preimage event, triggered upon the generation of a new chainTree root (chainRoot) and the corresponding update of its leaf in sharedTree, includes two parameters named chainRoot and preimage. However, the naming inaccurately implies a direct cryptographic relationship between the two parameters, which is not the case. Moreover, the event name and its parameters do not convey the full context needed to understand the update, as the sharedTree leaf's hash requires both the chainRoot and the chainId of the associated chain for complete clarity.
For bootloader.yul, consider renaming the identified variables to more accurately reflect their purpose. For MessageRoot.sol, consider renaming the Preimage event to better describe its meaning and the relationship between its parameters. These changes aim to enhance code readability and clarity.
Update: Resolved in pull request #1506 at commit 7303766. All the points raised were addressed.
INTEROP_ROOTS Reading Is Vulnerable to Buffer OverflowWhen setting new interop roots, the bootloader is designed to read data in the INTEROP_ROOTS array from index CURRENT_INTEROP_ROOT to index CURRENT_INTEROP_ROOT + INTEROP_BLOCKS[CURRENT_NUMBER_OF_ROOTS_IN_BLOCK] - 2. However, the INTEROP_ROOTS array can hold up to 100 roots. This indicates that these memory reads are vulnerable to buffer overflows. For example, if CURRENT_INTEROP_ROOT or INTEROP_BLOCKS[i] are too large, the bootloader would read from memory beyond the INTEROP_ROOTS array, which is the area storing the COMPRESSED_BYTECODES.
While the impact of such an overflow is unclear, consider nonetheless ensuring that the bootloader does not read roots from outside the array bounds to prevent any potential issues.
Update: Resolved in pull request #1507 at commit 13140a1. The code now reverts on attempts to read from outside the interop roots memory buffer.
This unused code increases the complexity of the codebase and poses a maintenance overhead, potentially obscuring the logic and functionality of the contracts.
Throughout the codebase, multiple instances of unused and redundant code were identified:
bootloader.yul, the initial assignment of the interopRootStartSlot variable within the setInteropRootForBlock function is never utilized in subsequent operations.BridgeHub.sol, the code related to pause functionality during upgrades is obsolete since the new ChainAssetHandler is responsible for it. Consider deprecating the associated state variable and deleting the associated modifier and functions.Admin.sol, Mailbox.sol, Getters.sol, and ZKChainBase.sol). These references are misleading and do not reflect the current architecture, potentially leading to confusion and errors in understanding or extending the codebase.Consider removing all instances of unnecessary code for enhanced code clarity and maintainability.
Update: Resolved in pull request #1514. The Matter Labs team stated:
We will deprecate the upgrade functionality in
BrigdeHubin the next version, since we will still need to pause migration before the upgrade, do the upgrade, and then unpause it.
INTEROP_ROOT_SLOT_SIZE Return ValueIn the bootloader contract, the storage of each interopRoot is designed to occupy 5 memory slots, which include the block number for interopRoot processing, the chain ID, the dependency block, the sides length, and the sides. However, the INTEROP_ROOT_SLOT_SIZE function erroneously returns a value of 6 instead of 5.
This discrepancy does not currently affect the system's operations, as the extra, unutilized memory slot does not compromise memory consistency and is essentially ignored. Nonetheless, this misalignment between the intended and actual slot size allocation could lead to unnecessary memory fragmentation. More importantly, it introduces a potential source of errors for future development efforts, where the incorrect slot size assumption could lead to more significant issues.
Consider fixing the INTEROP_ROOT_SLOT_SIZE function to return the accurate number of slots an interopRoot actually occupies. This adjustment will help enhance code clarity and maintainability, reducing the risk of errors in subsequent development phases.
Update: Resolved in pull request #1521 at commit abacb3c.
addInteropRoot Function Argument TypeThe addInteropRoot function of the L2InteropRootStorage contract is designed to accept a third argument as a bytes32 array. However, in its corresponding interface definition, the third parameter is specified as a singular bytes32 value instead of an array. While this interface is not currently utilized within the existing codebase, the presence of such a mismatch introduces a latent risk. This inconsistency between the contract implementation and its interface definition is inherently error-prone and could lead to integration issues or failures in future developments or updates.
To mitigate the risk of future errors and enhance code maintainability and clarity, consider aligning the addInteropRoot function's parameter type in both the contract implementation and its interface.
Update: Resolved in pull request #1509 at commit 271380d. The unused function was removed from the interface.
The verification of cross-chain messages on ZKChains is managed by the L2MessageVerification contract. However, the _proveL2LeafInclusion function allows the correct batch root to be zero when verifying a leaf. This allows for verifying that the leaf (bytes32(0)) was sent in a non-existent batch number by providing an empty log leaf proof with finalProofNode = true.
Consider validating that correctBatchRoot is non-zero when comparing it with the reconstructed root.
Update: Resolved in pull request #1510 at commit 25c3046.
setInteropRootForBlock Call at the End of bootloaderAt the end of bootloader execution on a batch, the setInteropRootForBlock(MAXIMUM_L2_BLOCK_NUMBER()) function is called. This call is documented as "setting all remaining interop roots, even the ones in the fictive block". However, following a discussion with the Matter Labs team, it was clarified that there should not be interop roots in virtual blocks.
Consider removing the aforementioned code to reduce the attack surface.
Update: Resolved in pull request #1515 at commit 73be8f2.
bytes32(0) on ZKChainsThe Executor and L2InteropRootStorage contracts allow the operator to add dummy interop roots with the value bytes32(0) for any blockOrBatchNumber for which a root is not already defined. While this does not cause immediate security concerns, it represents a risky pattern and emits an event that could be misleading.
Consider validating that interop roots added on ZKChains cannot be zero.
Update: Resolved in pull request #1516 at commit 9beb04e. Checks were added on L1 and L2 to ensure the interop root is nonzero.
Misleading documentation or comments can confuse developers or lead to incorrect assumptions about the functionality of the code.
Throughout the codebase, multiple instances of misleading documentation or comments were identified:
In bootloader.yul:
CURRENT_NUMBER_OF_ROOTS_IN_BLOCK_SLOT inaccurately suggests that it returns the number of interopRoots in a block. However, it provides the storage slot for the count of blocks processed so far. Similarly, the description for CURRENT_NUMBER_OF_ROOTS_IN_BLOCK_BYTE in line 320 follows the same misleading pattern.INTEROP_BLOCKS_BEGIN_SLOT and INTEROP_BLOCKS_BEGIN_BYTE are described as being the starting point at which interopRoots are stored, whereas they return the starting slot and byte, respectively, of interopRoot counts per block.In MessageHashing.sol:
getProofData function states that the function hashes the proof. However, the actual functionality involves parsing the proof and returning the final proof data, not hashing it.In IExecutor.sol:
todo comment for the StoredBatchInfo struct can be removed as the action item has already been implemented. Consider removing it.ShardId, isService, and txNumberInBatch)" or something similar.Consider revising the documentation to accurately reflect the functionality of each function and variable. Accurate documentation significantly improves the clarity and maintainability of the codebase.
Update: Resolved in pull request #1511 at commit 678273e. All the instances identified were addressed.
Throughout the codebase, multiple opportunities for code improvements were identified:
getChainRoot function could validate that _chainId is registered before returning a root. Otherwise, an underflow error would occur for unregistered chains.MismatchNumberOfLayer1Txs error are inverted compared to its definition.SystemLogKey is defined differently in Constants.sol compared to IExecutor.sol. Specifically, the MESSAGE_ROOT_ROLLING_HASH_KEY field is missing.MessageVerification.sol, the check verifying the log hash against the default leaf hash within the _proveL2LogInclusion function should be moved into the _proveL2LeafInclusion function in order to be effective in all cases where a log is being proved.bootloader.yul, consider defining a constant for the currently hardcoded value of the maximum number of processed blocks within a batch. This would help semantically distinguish it from the maximum number of INTEROP_ROOTS per batch, which carries the same value at the moment.Consider addressing the identified instances to improve the clarity and maintainability of the codebase.
Update: Resolved in pull request #1517 at commit 5116186 and pull request #1522 at commit 4566b98. All the points identified were addressed. Regarding the last point the number of transactions, which is an upper bound on the number of blocks in the batch, was used instead.
Throughout the codebase, multiple opportunities for gas optimization were identified:
addChainBatchRoot and updateFullTree functions, updates are done to the shared tree through functions that return the new root. This return value is ignored and sharedTree.root() is called instead to get the updated root. Consider using the returned value to save gas.updateFullTree function, the chainIndexToId[i] value is read from a storage mapping twice and does not appear to be automatically cached by the compiler. Consider caching it manually in a local variable to save gas on each loop.Consider implementing the aforementioned suggestions to improve the gas efficiency of the codebase.
Update: Resolved in pull request #1512 at commit 081796d.
Throughout the codebase, multiple instances of typographical errors were identified:
L2InteropRootStorage.sol, the NatSpec line starts with "//" instead of "///". Consider fixing it so that any tool relying on documentation parsing can successfully extract it.bootloader.yul, "The slot starting from the L2 block [...]" should be "The slot starting from which the L2 block [...]".To improve the clarity and readability of the codebase, consider addressing the above-listed instances of typographical errors.
Update: Resolved in pull request #1513 at commit 741eaf7.
Throughout the codebase, multiple opportunities for code improvement were identified:
ValidatorTimelock contract and its interface, the hasRoleForChainId function could be marked as view._calculatePrecommitmentRollingHash function of the Executor contract, the assembly block could be marked as memory-safe as it only accesses the scratch space and memory within the bounds of the _packedTxPrecommitments memory array.bootloader contract, the comment in line 195 states that only L2 transactions are stored under the CURRENT_L2_TX_HASHES memory slot. However, this is not true since priority transactions are also stored at this location in the current version of the code.IValidatorTimelock.sol interface, the parameters of the proveBatchesSharedBridge function could be renamed to _chainAddress, _processBatchFrom, _processBatchTo, and _proofData, respectively, in order to be consistent with the implementation.ValidatorTimelock contract, the _l2BatchNumber parameter of the revertBatchesSharedBridge function could be renamed to _newLastBatch.Executor contract, some parameter types [1] [2] [3] have been changed from uint256 to address. However, the "// _chainId" comments were not updated accordingly. In these comments, the parameters should be renamed to _chainAddress.ValidatorTimelock contract, some comments [1] [2] appear to have been copy-pasted and, as a result, are irrelevant given the context. Consider removing these comments.Consider implementing the above-listed code quality improvement suggestions to enhance the clarity and maintainability of the codebase.
Update: Resolved in pull request #1530 at commit 03031ec. All the instances raised were addressed.
In the ChainTypeManager contract, the order of the arguments being supplied to the OutdatedProtocolVersion error appears to be inverted compared to the error's definition.
Consider fixing the argument order of this instance of the OutdatedProtocolVersion error to improve code clarity and avoid confusion.
Update: Resolved in pull request #1535 at commit b6b26b3.
The sendToL1 function of the L1Messenger contract defines the data location of the bytes message argument as calldata. However, the IL2ToL1Messenger sets the argument's location as memory.
Consider fixing the IL2ToL1Messenger interface so that the specified data location for the sendToL1 function argument is consistent with its implementation.
Update: Resolved in pull request #1536 at commit c20b5f4.
Throughout the codebase, multiple instances of typographical errors were identified:
ServerNotifier.sol, "the chain initiating migration to the ZK gateway" should be "the chain initiating migration from the ZK gateway".bootloader.yul, "The output has size of 32" should be "The output has size of 64".SystemContractHelper.sol, the word "validate" is missing from the comment.To improve the clarity and readability of the codebase, consider addressing the above-listed instances of typographical errors.
Update: Resolved in pull request #1537 at commit a8e76e7.
IExecutor Interface Change Prevents Chain SettlementWhen upgrading a ZK Chain, the high-level execution path is as follows:
ChainTypeManager.executeUpgrade, which calls diamondCut on the ZK Chain's diamond proxy.Executor contract), and then delegate calls to the L1V29Upgrade contract which optionally reassigns the validator on the ZK Chain's diamond proxy.commitBatchesSharedBridge to upgrade the ZK Chain's system contracts.However, the IExecutor interface was changed in the codebase, while the ValidatorTimelock implementation sits behind a TransparentUpgradeableProxy and is upgraded independently from the above flow. Additionally, the ValidatorTimelock is shared between all the ZK Chains, which are not required to upgrade at the same time. Because the ValidatorTimelock would not be compatible with both old and new IExecutor interfaces, this means that it would no longer be able to call some functions (part of a now unsupported IExecutor interface) required to settle some ZK Chains.
Update: Resolved in pull request #1571 at commit ed3d351. The ChainTypeManager contract now supports two versions of the ValidatorTimelock, which will be deployed separately. ZK Chains will change the supported validator when upgrading so as to support a ValidatorTimelock which is compatible with their IExecutor interface.
The changes under audit mainly introduce two new features: reduced interoperability, allowing for message passing between ZKChains settling on Gateway, and fast finality, allowing third-parties to verify the finality of ZKChain batches by monitoring pre-commitments on L1. Several other smaller code changes were also audited, mostly related to fixes and optimizations.
The main issues found were related to bootloader logic. Overall, the codebase was found to be well written and comprehensively documented. The Matter Labs team is appreciated for being highly responsive throughout the audit period, providing detailed explanations whenever needed along with valuable insights regarding the reasoning about certain design choices.