March 9, 2023
This security assessment was prepared by OpenZeppelin.
We audited the matter-labs/zksync-2-contracts repository at the 9f3c6944e6320166edd96ef6586a9dd4548a27f2 commit. In scope were the following contracts:
├── ethereum
│ └── contracts
│ └── bridge
│ ├── L1ERC20Bridge.sol
│ └── interfaces
│ ├── IL1Bridge.sol
│ └── IL2Bridge.sol
└── zksync
└── contracts
└── bridge
├── L2ERC20Bridge.sol
├── L2StandardERC20.sol
└── interfaces
├── IL1Bridge.sol
├── IL2Bridge.sol
└── IL2StandardToken.sol
In addition, we audited the matter-labs/system-contracts repository at the 191246a878f1493a5ed5f0fa6d79af4ce3e7eb8f commit of pull request #170. The contracts marked as diff were previously audited and hence only the latest changes were reviewed. All other contracts were fully audited. In scope were the following contracts:
├── bootloader
│ └── bootloader.yul (diff)
└── contracts
├── AccountCodeStorage.sol
├── ContractDeployer.sol
├── ImmutableSimulator.sol
├── KnownCodesStorage.sol
├── L2EthToken.sol (diff)
├── MsgValueSimulator.sol (diff)
├── NonceHolder.sol
└── interfaces
├── IAccountCodeStorage.sol
├── IContractDeployer.sol
├── IEthToken.sol (diff)
├── IImmutableSimulator.sol
├── IKnownCodesStorage.sol
├── IL2StandardToken.sol
└── INonceHolder.sol
This audit focuses on the latest changes to the bootloader, described in our previous audit, the functionality of the L1/L2 (Layer 1 / Layer 2) Bridges, and changes to the system contracts. We found the system to be modular and well-documented.
The bootloader now supports the fee model, which is the main change over the previous version. For a given L1 gas price, the bootloader calculates the basefee for the zkSync L2. Users are generally required to make significant upfront payments when conducting zkSync transactions, and are later refunded for the unused ergs, the zkSync-equivalent gas unit. The fee includes an overhead amount of ergs, which compensates for the calculation of the proof as well as committing and verifying the block on L1. The bootloader is ensuring that users are not overcharged for their transactions by making these calculations itself and checking against the operator inputs. As such, the bootloader plays a crucial role in the fair processing of transactions. However, the operator can provide a higher refund to the user than what was calculated by the bootloader. In short, the bootloader calculates how much of the block’s overhead should be covered by each transaction. It also acts as an ETH bridge now, which was previously done through dedicated bridge contracts.
We identified one high-severity issue which allows a malicious operator to minimize the refund for the user, thereby effectively stealing funds. Further, we raised a medium-severity issue about instances of overflows and underflows as part of the fee calculation, one of which allows users to pay no fees.
The new token bridge exclusively handles transfers of ERC-20 tokens between the two domains. It is worth noting that although it does not support ETH transfers, it is possible to send an arbitrary amount of ETH along with any token deposit.
All ERC-20 tokens are supported, as long as they are defined on L1. For every token deposited in the L1 bridge, a new token is minted on L2. The L2 token will copy the L1 token’s optional metadata (i.e. the name, symbol and decimals values) but it is otherwise a standard ERC-20 token. Any non-standard functionality (e.g., rebasing or transfer restrictions) is not replicated on the L2 token. When the token is returned to the L2 bridge, it is then burned and the corresponding L1 tokens are released.
The codebase defines some system contracts on L2 that help replicate (and extend) the EVM’s behavior in the zkEVM. Below is a summary of the system contracts that have been audited, with an overview of their respective functionality.
The zkEVM supports native account abstraction, which means it recognizes some contracts as “accounts” that can originate transactions. To prevent replay attacks, the bootloader ensures that every transaction originating from an account has a unique nonce. It also ensures that account nonces are consumed sequentially (so all transactions have a well-defined order), although each account can optionally relax this restriction.
In addition, all contracts (including accounts) use a sequential deployment nonce to ensure every contract they deploy with the CREATE opcode has a unique address. If they use the CREATE2 opcode, the salt parameter should provide uniqueness, although the deployment nonce is incremented anyway so it still tracks the number of contracts that have been deployed.
The NonceHolder contract manages these nonces for all contracts and provides utility functions to ensure uniqueness.
To guarantee data availability, all L2 transactions are published on L1. In addition, the bytecode of all L2 contracts should also be published on L1. This ensures that the entire state of the zkEVM can always be reconstructed from publicly available information.
The KnownCodeStorage contract simply tracks which L2 contracts (technically, which bytecode hashes) have been published. To achieve this, L2 transactions can include optional bytecode hashes to signal to the bootloader that the preimages should be published. Any new values are marked off in the KnownCodeStorage contract once it guarantees that the preimage is known (either because the transaction originated on L1 or because the KnownCodeStorage explicitly sends a message to L1 that can only be resolved by publishing the preimage). It also ensures that any new bytecode conforms to the zkEVM length requirements.
The AccountCodeStorage contract stores the code hashes associated with all L2 contracts (not just accounts). This facilitates querying the code hash and code size of all contracts. In contrast to the EVM, L2 contracts don’t have separate initialization code, and instead include the constructor as part of their run-time bytecode. The AccountCodeStorage contract also manages the isConstructing flag associated with each contract to help recreate the expected constructor behavior (ie. it is called exactly once during deployment and never again).
Since the zkEVM does not distinguish between initialization code and run-time code, the contract’s code needs to be registered before the constructor is executed. This means that in contrast to the EVM, the constructor cannot modify the run-time code to include any immutable values.
Instead, the constructor code simply returns all of the immutable values, which are saved in the ImmutableSimulator contract for whenever they are needed.
Instead of using the CREATE and CREATE2 opcodes, contracts on the zkEVM are deployed using the ContractDeployer system contract. This contract manages the deployment nonces and ensures all new bytecode is known (see the KnownCodeStorage section).
It also distinguishes between “accounts” and regular contracts to ensure accounts can choose their account abstraction implementation (only version 1 is currently supported) and can manage their transaction nonce ordering.
The security of the system in scope depends on the individual governors of the respective contracts, which are analyzed for the bridge and the system contracts.
The L1 ERC-20 bridge as well as the L2 ERC-20 token implementation are both upgradeable. Any ERC-20 token that is bridged into zkSync will be locked into this L1 bridge contract, and a malicious upgrade to this contract would let an attacker steal the locked funds. The L2 token implementation could be similarly affected (by stealing the tokens through a malicious version on L2 and then bridging them back to L1).
The L1 ERC-20 bridge implements the AllowList access control. This mechanism enables restriction for functions and callers. Hence, a function can be fully inaccessible, or only accessible to specific addresses. The finalizeWithdrawal function is affected by this mechanism, which is a necessary step to withdraw ERC-20 tokens from L2 to L1. Thus, users could be prevented from bridging their assets back to L1.
The system contracts in scope play an integral role in the system’s operations. These contracts can be upgraded through a force-deploy through a requested L2 transaction from L1. This upgrade mechanism is tied to proposals made to the DiamondCut facet and managed by this respective governor and the security council.
It is strongly advised that the governors and owners of these contracts be extensively secured through cold wallets and multisigs. Further, it is assumed that these actors will act in the best interest of the users.
The zkSync system requires a dedicated operator role to coordinate and bundle all L2 transactions. Currently, the operator will be centralized through Matter Labs with the goal to decentralize it in the medium-term future. The operator has the privilege to include or exclude any transaction they choose. The priority mode censoring protection mechanism is still to be implemented. Furthermore, as part of the fee model at the current stage of the protocol, the operator is trusted to pick a fair L1 gas price, which influences the L2 fees paid by the user.
It is worth noting that due to the current system design, some interactions work differently from the EVM. For instance, to deploy a contract or custom account, a user must call the respective system contract along with specific parameters, which is not the case for an EVM deployment transaction. Prior to that, the bytecodes (factory dependencies) need to be explicitly shared with the operator and published on L1. The zkSync documentation provides additional context.
In addition, as seen in the Potential EIP-1052 Deviation issue, it is possible that opcodes deviate from EVM behavior, which still needs to be explored for other opcodes. For more differences – including unsupported opcodes – visit the EVM compatibility section of the zkSync documentation.
Due to the complex nature of the system and several subtle deviations from the EVM, we believe this audit would have benefitted from more complete testing coverage, particularly around the bridge messages and new fee mechanics.
While insufficient testing is not necessarily a vulnerability, it implies a high probability of additional hidden vulnerabilities and bugs. Given the complexity of this codebase and the numerous interrelated risk factors, this probability is further increased. Testing provides a full implicit specification along with the exact expected behaviors of the codebase, which is especially important when adding novel functionalities. A lack thereof increases the chances that correctness issues will be missed. It also results in more effort to establish basic correctness and reduces the effort spent exploring edge cases, thereby increasing the chances of missing complex issues.
Moreover, the lack of repeated automated testing of the full specification increases the chances of introducing breaking changes and new vulnerabilities. This applies to both previously audited code and future changes to current code. This is particularly true in this project due to the pace, extent, and complexity of ongoing and planned changes across all parts of the stack (L1, L2, bootloader and system contracts, compiler and zkEVM). Underspecified interfaces and assumptions increase the risk of subtle integration issues, which testing could reduce by enforcing an exhaustive specification.
To address these issues, we recommend implementing a comprehensive multi-level test suite before the next expected audits. Such a test suite should comprise contract-level tests with >90% coverage, per-layer deployment and integration tests that test the deployment scripts as well as the system as a whole, per-layer fork tests for planned upgrades, and cross-chain full integration tests of the entire system. Crucially, the test suite should be documented in a way so that a reviewer can set up and run all these test layers independently of the development team. Some existing examples of such setups can be suggested for use as reference in a follow-up conversation. Implementing such a test suite should be a very high priority to ensure the system’s robustness and reduce the risk of vulnerabilities and bugs.
In the refundCurrentL2Transaction function, the user gets refunded for overpaying in ergs for their L2 transaction. The ETH amount to refund is dependent on the ergs amount and the ergs price. The ergs amount is the maximum value of the operator-provided value and the one calculated by the bootloader. The ergs amount multiplied by the ergs price determines the refund in ETH.
A malicious operator can provide a very large amount of ergs that should be refunded, which will therefore be the chosen refundInErgs value. However, when multiplied by the ergs price, ethToRefund can overflow and lead to a very low refund, effectively stealing funds from the user.
Consider calculating the refund based on the bootloader and operator values individually and then picking the higher amount to protect the fee model against a malicious operator.
Update: Resolved in pull request #208 at commit 57ba2b1.
L1ERC20Bridge Implementation ContractThe L1ERC20Bridge is the implementation contract that is intended to be used with a proxy, so it is good practice to restrict how it can be invoked directly. When the contract is constructed, the reentrancyGuardInitializer modifier is executed and the immutable _mailbox address is written to the bytecode that is deployed. During this invocation of reentrancyGuardInitializer, the reentrancy status is set to _NOT_ENTERED, which locks the initialize function from being called, but simultaneously allows functions with the nonReentrant modifier to be called.
Specifically, the deposit function of the implementation contract is callable. However, in the implementation contract itself, many variables are not initialized, such as the l2Bridge variable, so it holds the zero-address. Therefore, the deposit function would request an L2 transaction that attempts to finalize the withdrawal by calling the zero-address, thereby triggering the non-reverting fallback function of the EmptyContract. Since this L2 call does not fail, the deposited tokens are locked and irrecoverable, as a call to claimFailedDeposit cannot be proven.
Consider implementing a stricter mechanism that prohibits direct calls to the contract if all or some of its variables were not properly initialized. In addition, consider preventing the initialization of the implementation contract more directly, rather than relying on the implicit behavior of reentrancyGuardInitializer, which lacks visibility.
Update: Acknowledged, not resolved. The Matter Labs team stated:
While we appreciate your insights and suggestions, we do not believe the issue carries a significant security risk. As the contract is intended to be used through a proxy, direct calls to the implementation contract are not recommended. Users could also call any other scam contract.
__gap VariableThe L1ERC20Bridge and L2StandardERC20 contract are intended to be used as logic contracts with a proxy, but do not have a __gap variable. This would become problematic if a subsequent version was to inherit one of these contracts. If the derived version were to have storage variables itself and additional storage variables were subsequently added to the inherited contract, a storage collision would occur.
Consider appending a __gap variable as the last storage variable to these upgradeable contracts, such that the storage slots sum up to a fixed amount (e.g. 50). This will proof any future storage layout changes to the base contract. Note that the __gap variable space will need to be adjusted accordingly as subsequent versions include more storage variables, in order to maintain the fixed amount of slots (e.g. 50).
Update: Acknowledged, not resolved. The Matter Labs team stated:
While we appreciate your insights and suggestions, we do not believe the issue has a significant security risk. Specified contracts are not expected to be inherited, since they are complete logical contracts.
ceilDiv Function Can OverflowThe ceilDiv function of the bootloader is used throughout the fee model formulas. This function can overflow as the numerator and denominator are added together before they are divided. Although this was not identified as a threat in the current codebase, an issue could be introduced by using this function with unvalidated inputs.
Consider adapting the formula to be safe from overflows.
Update: Resolved in pull request #236 at commit 56b3231.
The bootloader changes in scope involve a few formulas as part of the fee model, and there were a few instances where calculations were performed on user or operator-provided inputs. Unchecked arithmetic (without overflow protection) using these values is generally dangerous and prone to exploits.
One example is the calculated ergs price. If the maxPriorityFeePerErg is sufficiently large and the maxFeePerErg value is increased to match, the maxFeeThatOperatorCouldTake calculation could overflow, resulting in a zero ergs price. The transaction will still be executed but the amount the user pays would be zero. A savvy operator could recognize this scenario and discard the operation, but it does make the system unnecessarily fragile.
We also identified the following cases where potential overflows with user-provided values appear to be unmitigated, although the consequences are limited:
intrinsicOverhead calculation in the getErgsLimitForTx functiongetBlockOverheadErgs functionoverheadForCircuits, overheadForLength, and overheadForPubdata calculations in the getTransactionUpfrontOverhead functionLastly, the following functions appear to allow overflows, but they are protected by validations in other parts of the codebase:
getBaseFee, a large l1GasPrice could cause pubdataBytePriceETH to overflow but the validateOperatorProvidedPrices checks prevent this.processL1Tx, toRefundRecipient would negative overflow if the value was too large, but the check on line 1306 prevents this.getErgsLimitForTx, ergsLimitForTx would negative overflow if the operatorOverheadForTransaction was too large, but the check on line 1253 prevents this.Consider applying additional checks to these operations, explicitly documenting where the checks are or why they would not be necessary. Whenever a potential overflow is not mitigated in the same function, consider documenting where the relevant validation can be found. It is advised to check the rest of the bootloader for more potential overflows and underflows. Lastly, it is recommended to validate all changes with proper dynamic testing.
Update: Resolved in pull request #211 at commit 448932e.
When the L1ERC20Bridge is initialized, the bytecode of the L2 bridge and token proxy are both provided as factory dependencies. This ensures the code is known when the L2 Bridge is deployed, or a new token is created. However, the L2 bridge initialization also deploys the token implementation and proxy beacon. Since neither contract was listed as a factory dependency, they may not be marked as known. Unless they were previously mentioned in an unrelated transaction, the bridge initialization will fail.
Consider including the L2StandardERC20 and the UpgradeableBeacon contracts in the factory dependencies of the bridge initialization transaction.
Update: Acknowledged, will resolve. The Matter Labs team stated:
Acknowledged. The problem can only be encountered at the initialization stage, so we prefer not to change the deployment scripts at the moment. This has been included in the backlog as a refactoring task.
require and revert StatementsThroughout the bridge and system contracts codebases there are require and revert statements that lack error messages:
require statement on line 96 of L2StandardERC20.solrevert statement on line 116 of L2StandardERC20.solrevert statement on line 122 of L2StandardERC20.solrevert statement on line 128 of L2StandardERC20.solrequire statement on line 25 of AccountCodeStorage.solrequire statement on line 36 of AccountCodeStorage.solrequire statement on line 50 of AccountCodeStorage.solrequire statement on line 39 of ContractDeployer.solrequire statement on line 34 of ImmutableSimulator.solConsider including specific, informative error messages in require and revert statements to improve overall code clarity and facilitate troubleshooting whenever a requirement is not satisfied. In addition, for statements that do contain a message, consider using longer strings instead of a few characters to describe the error.
Update: Resolved in pull request #212 at commit cb93637 and pull request #63 at commit 77e64b5. The Matter Labs team stated:
We did not add error messages to reverts from name/symbol/decimals functions because if the token does not implement that method it should behave exactly the same way as if the function was not declared.
L2ERC20Bridge Is Not UpgradeableThe L1ERC20Bridge and L2ERC20Bridge contracts manage the ERC-20 token bridge for the Layer 1 and Layer 2 sides respectively. These two bridge contracts are intertwined, as L1 tokens are locked into the L1ERC20Bridge in order to mint the corresponding L2 tokens. Currently, these L1 tokens can only be unlocked in the event of a withdrawal that is initialized through L2ERC20Bridge. In the event of an undiscovered bug in the L2 bridge, the only way to upgrade it would be to redeploy a new L1 bridge (since it is not upgradeable).
The new L1ERC20Bridge contract would then be intertwined with a new L2ERC20Bridge. One drawback of this approach is the inconvenience of re-deploying a new L1 contract every time there is a desire to change the L2 contract. In addition, each ERC-20 token minted on a different L2 bridge would generate a new L2 address, creating multiple copies of the same L1 token in L2. Consider making the L2ERC20Bridge upgradable by making it an implementation contract behind a proxy. However, note that even without this upgradeability of the L2 bridge, the upgradeability of the L1 bridge would still prevent L1 tokens from being locked forever.
Update: Resolved at commit b51d4c3.
On zkSync, the EXTCODEHASH opcode is realized by using the getCodeHash function of the AccountCodeStorage contract. This returns the registered bytecode hashes for deployed contracts. The EIP-1052 standard dictates that the return of precompile contracts should be either 0 or c5d246... (the hash of the empty string). However, precompile code hashes can differ from those values by setting them to their real code hash or to an arbitrary value during a forced deployment, thereby causing an inconsistency with EIP-1052.
Consider conforming to EIP-1052 by adding additional checks to ensure that the precompile contracts return either of the values defined in the specification.
Update: Resolved in pull request #213 at commit 98cb968. The Matter Labs team stated:
We also added the correct codehash/codesize for the zero-address.
In ContractDeployer.sol, there is no event emitted when an AccountAbstractionVersion or an [AccountNonceOrdering] (https://github.com/matter-labs/system-contracts/blob/191246a878f1493a5ed5f0fa6d79af4ce3e7eb8f/contracts/ContractDeployer.sol#L81) are updated, which makes it challenging to monitor whether an account has updated either of these values.
In addition, the DepositInitiated event should emit the L2 transaction hash, because it may be needed to claim a failed deposit.
Consider emitting and updating these events to facilitate monitoring.
Update: Resolved in pull request #214 at commit d398cce and pull request #63 at commit d0ce4d4.
setValueUnderNonce Value Is MutableIn the NonceHolder contract, the setValueUnderNonce function sets a specific _value associated with a particular _key (nonce). While setting it to 1 would be enough to simply convey if a specific nonce has already been used, it is actually a uint256 value. According to the internal documentation shared with us, it “could be used to store some valuable information.” This could correspond to details about the particular transaction that consumed this nonce (e.g., the amount of wei that was sent in the transaction).
While a user cannot reset the _value to 0, they can change it to any other non-zero quantity. This can be misleading for external entities that may be relying on it for particular information about the transaction. More importantly, since no event is emitted, an external party may not know that it has been changed.
Consider requiring that the _value for a particular _key is settable only once. Alternatively, if it is intended to be mutable, consider emitting an event every time it is set.
Update: Resolved in pull request #215 at commit d6fd17d. The Matter Labs team stated:
It is mutable in case users want to store some valuable data under the respective nonce. For example, a user might want to store a mapping such as
(uniqueNonce => someInternalConstant).
The L1ERC20Bridge contract will deploy the Layer 2 token bridge by passing a request to the Mailbox. However, it will not send any ETH. This is acceptable while the stub fee calculation uses a zero fee, but when the calculation is updated, messages that do not pay fees will be rejected.
In the interest of predictability, consider allowing ETH to be sent with the L2 bridge deployment request.
Update: Resolved in pull request #65 at commit 15b3433.
Throughout the bridge and the system contracts codebases, the version of Solidity used is ^0.8.0. This version indicates that any compiler version after 0.8.0 can be used to compile the source code. However, the code will not compile when using version 0.8.7 or earlier since there are functions which override interface functions without using the keyword override. This is only permitted in Solidity 0.8.8 and above. In addition, there is the usage of abi.encodeCall, which was not introduced until Solidity 0.8.11. However, it is recommended to use Solidity 0.8.13 or above since there was a bug detected regarding fixed-length bytes literals. While this bug does not currently affect the codebase, using an updated version will remove the possibility of future errors.
Consider upgrading all contracts to Solidity version 0.8.13 at a minimum, but ideally to the latest version.
Update: Resolved in pull request #66 at commit e422c7f. Regarding the floating point Solidity version, the Matter Labs team stated:
System contracts are exposing the System API to smart contract developers, so the code they write will have to adhere to the same compiler version’s limitations. Similarly to OpenZeppelin Smart Contracts libraries, our System Contracts enforce the minimum Solidity version to ensure that they can be properly built, but do not pin a specific version, so we do not prevent the developers from using newer features.
When depositing ERC-20 tokens, the L1ERC20Bridge contract queries the token’s metadata, and sends the results to the L2ERC20Bridge contract. Any errors are still encoded on L1 and discarded on the L2 bridge. This pattern seems unnecessarily complex, and couples the processing on both sides. Consider detecting errors on L1 and only sending the relevant values over the bridge.
Update: Acknowledged, will resolve. The Matter Labs team stated:
We will take this change into account when further refactoring is done.
The isNonceUsed function is defined in the NonceHolder contract, but it is missing in the INonceHolder interface.
Consider aligning the interface with the contract to fully represent its features.
Update: Resolved in pull request #216 at commit c9f1e3c.
Throughout the bridge and system contracts codebases, there are state variables, constants, and immutables that lack an explicitly declared visibility:
allowList in L1ERC20Bridge.solzkSyncMailbox in L1ERC20Bridge.solDEPLOY_L2_BRIDGE_COUNTERPART_ERGS_LIMIT in L1ERC20Bridge.solDEFAULT_ERGS_PRICE_PER_PUBDATA in L1ERC20Bridge.soll2TokenProxyBytecodeHash in L2ERC20Bridge.solavailableGetters in L2StandardERC20.solEMPTY_STRING_KECCAK in AccountCodeStorage.sol__DEPRECATED_l2Bridge in L2EthToken.solDEPLOY_NONCE_MULTIPLIER in NonceHolder.solMAXIMAL_MIN_NONCE_INCREMENT in NonceHolder.solFor clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.
Update: Acknowledged, will resolve. The Matter Labs team stated:
We will take this change into account when further refactoring is done.
There are two instances in the codebase where redundant computations are performed:
ContractDeployer contract, there is some redundancy in the logic of the create{2}{Account} functions. Consider highlighting their differences and reusing existing logic by having one function call the other.getErgsLimitForTx function of the bootloader, the ergsLimitForTx value is computed a second time after being computed as part of the getVerifiedOperatorOverheadForTx function. This particularly hinders the visibility of the underflow check.Consider applying the above changes to improve the code’s clarity.
Update: Resolved in pull request #218 at commit 5828dc1.
In the L2EthToken contract, if the user attempts to transfer more funds than owned, the function reverts with a Solidity underflow error. This can result in a confusing user experience. Consider explicitly checking this condition and giving the user a proper revert reason if the transfer fails.
Update: Resolved in pull request #217 at commit d4e019f.
Throughout the codebase there are several files that state an SPDX license identifier of “MIT OR Apache-2.0”.
Consider agreeing on one license per file to prevent confusion on how these files can be used.
Update: Resolved in pull request #226 at commit d7c89a5.
The L2StandardERC20 contract has the BridgeInitialization event defined in the contract, while its other events BridgeMint and BridgeBurn are defined in the IL2StandardToken interface.
Consider defining events in the same place for better visibility.
Update: Resolved in pull request #67 at commit 1de79d9.
The following opportunities for gas optimizations were identified:
L2StandardERC20 contract, the bridgeInitialize function checks that the l1Address is not the zero address. This is to guarantee that the contract has not been initialized before. However, the contract already has the initializer modifier, so the require statement is redundant.l2TokenProxyBytecodeHash variable can be made immutable since it is only set once in the constructor and there is no functionality to change it. The purpose of this variable is solely to determine the address of the L2 Token._splitRawNonce function a division by a power of two is made. Instead, a right shift by the exponent would be cheaper.Consider making the above changes to reduce gas consumption.
Update: Partially resolved in pull request #68 at commit b37a19a. The Matter Labs team stated:
We did not implement the second and third suggestions, since
immutableis more expensive in L2 contracts (seeImmutableSimulatorcontract) and the arithmetic operations are well-optimized by our compiler (LLVM backend!).
The L2StandardERC20 contract is used as the token logic contract for the bridge. Hence, each token is a beacon proxy instance which refers to the logic of the L2StandardERC20 contract, which is initializable. The actual logic contract is initialized by having the initializer modifier on the constructor, to prevent an attacker from initializing it maliciously.
Instead of using the modifier, consider calling the _disableInitializers function of the Initializable contract explicitly in favor of readability and clarity.
Update: Resolved in pull request #69 at commit 5db383b.
In the L1ERC20Bridge contract, while most functions use named return variables, the _depositFunds and l2TokenAddress functions have a return statement instead. On the other hand, the L2ERC20Bridge contract mostly uses unnamed return statements, except for the _getCreate2Salt function. In the NonceHolder contract, there are both a named return variable and a return statement in the getDeploymentNonce function.
Consider applying one return style for better readability.
Update: Acknowledged, not resolved. The Matter Labs team stated:
It makes sense to follow one style standard, but in practice, we see that different return methods are convenient in different cases, so for now we will stay with what we have. However, we will rethink this when we begin refactoring the codebase.
In the bootloader contract, there is an inconsistency in the way memory offsets are declared, with some being expressed in decimals and most others being expressed in hexadecimals. This deviation from a consistent notation can be confusing and make it difficult to understand the purpose and usage of these sizes. For instance:
add(txPtr, 32) in line 470lt(returnlen, 96) in line 759sub(returnlen, 0x40) in line 785returndatacopy(PAYMASTER_CONTEXT_BEGIN_BYTE(), 64, effectiveContextLen) in line 796Consider using a consistent notation for expressing memory offsets throughout the codebase.
Update: Partially resolved in pull request #219 at commit c022f89. There are more instances similar to the examples mentioned above that can be changed with further refactoring.
To favor explicitness and readability, there are several locations in the contracts that may benefit from better naming:
L1ERC20Bridge.sol:
txHash name is inconsistent with _l2TxHash. Consider renaming it to l2TxHash for consistency.l2TokenFactory is not the factory, but rather the beacon. In fact, the factory is L2ERC20Bridge. Consider renaming it to l2TokenBeacon.l2ProxyTokenBytecodeHash name is inconsistent with l2TokenProxyBytecodeHash. Consider renaming it to l2TokenProxyBytecodeHash for consistency.L2ERC20Bridge.sol:
l2TokenFactory is not the factory, but rather the beacon. In fact, the factory is L2ERC20Bridge. Consider renaming it to l2TokenBeacon.L2StandardERC20.sol:
BridgeInitialization event is inconsistent with the other events named BridgeMint and BridgeBurn. Consider renaming it to BridgeInitialize.AccountCodeStorage.sol:
Utils.isContractConsructing is spelled incorrectly. Consider renaming it to isContractConstructing.Update: Resolved in pull request #220 at commit d5bf7da and pull request #70 at commit 0cb750b.
Throughout the codebase imports on the following lines are unused and could be removed:
IAccountCodeStorage of ContractDeployer.solIKnownCodesStorage of ContractDeployer.solINonceHolder of ContractDeployer.solIL2StandardToken of L2EthToken.solIMailbox, L2Log, and L2Message of IL1Bridge.solConsider removing unused imports to avoid confusion that could reduce the overall clarity and readability of the codebase.
Update: Resolved in pull request #221 at commit 49dbcf5 and pull request #71 at commit 53beb18.
The forceDeployOnAddresses function attempts to send the specified amount of L2 ETH with each deployment, after collecting the aggregate amount. However, the value sent with each call is implicitly truncated to the least significant 128 bits (henceforth, “lower half”). This means the most significant 128 bits (“upper half”) of every value parameter is ignored, which has two implications:
ContractDeployer contract.FORCE_DEPLOYER address can manipulate the upper half of the aggregate amount by setting large values that will be truncated. It can also cause the result to overflow, since the summation occurs inside an unchecked block. If the actual aggregated amount exceeds 2128 Wei and the FORCE_DEPLOYER address manipulates the summation to clear the upper half, they will only need to pay for the lower half of the amount. The rest will come out of the ContractDeployer contract’s own balance.In practice, the total supply will be less than 2128 for the foreseeable future, so neither scenario should be possible. In addition, the FORCE_DEPLOYER is a trusted address that would not be expected to manipulate the deployment configurations. Nevertheless, all assumptions should be enforced wherever possible. In the interest of predictability and local reasoning, consider restricting the ForceDeployment.value parameter to 128 bits. Alternatively, consider requiring the msg.value to be less than 2128 Wei instead of truncating it, which would remove the possible inconsistency for all use cases.
Update: Resolved in pull request #222 at commit 9600e74. The Matter Labs team stated:
The
msg.valueis not implicitly truncated (if so we would have a larger issue). The compiler checks if the value is less than 2^128, and panics otherwise. ThesumOfValuesoverflow was resolved.
The deposit function of the L1ERC20Bridge contract interacts with an untrusted token before updating its own state. Although it is protected by the nonReentrant modifier and there are no reentrancy possibilities in the current codebase, as a matter of good practice, consider moving the token transfer to the end of the function to follow the checks-effects-interactions pattern.
Update: Acknowledge, not resolved. The Matter Labs team stated:
We agree that in general it is more appropriate to use patterns wherever possible. However, in our case, to follow the checks-effects-interactions we would need to remove the calculation of the amount of deposited money, as the balance differs before and after. On other hand, it is difficult for us to imagine a reentrancy attack on this contract and we are already defending against this possibility with a
nonReentrantmodifier. As a result, we decided not to implement the fix.
The NonceHolder contract uses a single storage slot to pack each account’s deployment nonce and minimum transaction nonce.
Instead of manually splitting and recombining them, consider defining a struct that holds them both. That struct can then be saved in the rawNonces mapping and the individual nonces can be accessed and updated more easily.
Update: Acknowledged, will resolve. The Matter Labs team stated:
We will take this change into account when further refactoring is done.
Throughout the codebase we identified the following typographical errors:
that used → that is used in L1ERC20Bridge.solthe transferring funds → the transferring of funds in L1ERC20Bridge.solwhich do not → which it does not in L2StandardERC20.solstore → stores in L2ERC20Bridge.solinitiate → initiated in L2ERC20Bridge.solDeploys → Deploy in L2ERC20Bridge.soleiher → either in NonceHolder.solvalue value → value in NonceHolder.solserver → serve in NonceHolder.solrather rather → rather in NonceHolder.solmsg.sneder → msg.sender in NonceHolder.solmethodd → method in NonceHolder.solThat is means → That means in bootloader.yulwe need to the ability to → we need the ability to in bootloader.yultransfered → transferred in bootloader.yulthat → than in bootloader.yulprovides provides → provides in bootloader.yulNo colission is not possible and No colission is possible → No collision is possible in ContractDeployer.solConsider correcting these typographical errors.
Update: Resolved in pull request #223 at commit 7042d1f, pull request #228 at commit 5398a91, and pull request #72 at commit f0c0544.
The line that extracts the isSystemCall flag has two sets of redundant brackets that do not clarify the order of operations.
Consider grouping the bit AND operation before the logical != operation, or removing the brackets entirely.
Update: Resolved in pull request #224 at commit ad13199.
The following parts of the codebase are lacking documentation:
_l2TxErgsLimit parameter of the deposit function is undocumented.@param statements:
L2StandardERC20 contract.resultPtr parameter of the processL1Tx function._aaVersion parameter of the createAccount and create2Account functions._sender parameter of the _performDeployOnAddress function.The following documentation is misleading:
_l1Token address says “Always should be equal to zero”, which does not make sense._l2TokenFactory address says “Pre-calculated address of L2 token beacon proxy”. However, the address is actually that of the UpgradeableBeacon contract.L2EthToken contract is missing that the bootloader also interacts with it._hash parameter should state that the account is “constructing” and is not “constructed”.KnownCodeStorage contract is wrong since the implementation of the bytecode hash matches this comment.mint function states to be only callable by the ETH bridge. This is obsolete and should refer to the bootloader now.withdraw function states that funds are claimable through finalizeWithdrawal, but the function is actually called finalizeEthWithdrawal.updateNonceOrdering function ignores the fact that the ordering can only be changed from sequential to arbitrary.createAccount function claims to accept a “nonce as one of its parameters”, however, salt is meant.MSG_VALUE_SIMULATOR_IS_SYSTEM_BIT constant addresses the second extraAbi parameter, while it is actually the first.NEW_CODE_HASHES_START_PTR and MAX_NEW_CODE_HASHES does not match any code.getNewAddressCreate function refers to collision resistance with “Ethereum’s CREATE2” but it should be “Ethereum’s CREATE”.Consider adding more documentation to the codebase to enhance its clarity. In addition, consider rephrasing misleading comments to match the intention of the code.
Update: Resolved in pull request #225 at commit 3a28b44, pull request #218 at commit 5828dc1, and pull request #73 at commit 275d73e.
This audit was conducted over the course of 4 weeks. Through an in-depth review of the new fee model, we uncovered a high-severity issue and additional attack surfaces. We also identified several medium and lower-severity issues in the system and the bridging contracts. The Matter Labs team provided us with dedicated documentation again, which was very helpful to understand this complex system.
While audits help in identifying code-level issues in the current implementation and potentially the code deployed in production, we encourage the Matter Labs team to consider incorporating monitoring activities in the production environment. Ongoing monitoring of deployed contracts helps in identifying potential threats and issues affecting the production environment. Hence, with the goal of providing a complete security assessment, we want to raise several actions addressing trust assumptions and out-of-scope components that can benefit from on-chain monitoring.
Critical: There are multiple privileged actions with serious security implications:
AllowList contract controls access to the token bridge functionality, and has the ability to disable withdrawals.FORCE_DEPLOYER address can upgrade any of the system contracts to radically change the behavior of the zkEVM.Consider monitoring triggers of these administrator functions to ensure all changes are expected.
Medium: Consider monitoring the size, cadence and token type of bridge transfers during normal operations to establish a baseline of healthy properties. Any large deviation, such as an unexpectedly large withdrawal, may indicate unusual behavior of the contracts or an ongoing attack.
High: All L1 deposits should correspond to tokens minted on L2. Similarly, all tokens burned on L2 should correspond to released tokens on L1. This means every L1 token’s bridge balance should match the corresponding L2 token’s total supply, with some tolerance to account for the time delay of bridge transfers. Consider monitoring that these two values remain acceptably close to each other (for example, within 5%).
Low: Withdrawals on L1 are not triggered automatically, and must instead be initiated by L1 users. Any withdrawal that takes too long to finalize may indicate a problem with the withdrawal proof. Consider monitoring for withdrawals that remain in transit for significantly longer than the median withdrawal duration.