We audited pull request #258 of the matterlabs/system-contracts repository up to the ce6c3458f0f16d20ac335071de161a986a4ecb0d commit.
In scope were the following contracts:
contracts
├── KnownCodesStorage.sol
├── L1Messenger.sol
├── MsgValueSimulator.sol
└── libraries
└── SystemContractHelper.sol
We also audited the matter-labs/zksync-2-contracts repository at the c44852f89f443bbe8120a7c6abcc6984b394eb29 commit.
contracts
├── ethereum
│ └── contracts
│ ├── bridge
│ │ ├── L1ERC20Bridge.sol
│ │ ├── interfaces
│ │ │ └── IL2ERC20Bridge.sol
│ │ └── libraries
│ │ └── BridgeInitializationHelper.sol
│ └── zksync
│ └── facets
│ └── Mailbox.sol
└── zksync
└── contracts
├── L2ContractHelper.sol
└── bridge
├── L2ERC20Bridge.sol
└── L2Weth.sol
The first scope introduces changes to the way gas is handled in Layer 2 (L2) calls. Most importantly, whenever the MsgValueSimulator is invoked, the contract now explicitly withholds the gas reserved for decommitting the called contract as well as any gas that was reserved for the ETH transfer operations but was not consumed. In this way, the gas provided to the called contract can be closely (but not perfectly) controlled. This is needed to ensure .transfer and .send operations receive a predictable gas stipend.
It's worth noting that while the smart contract updates were reviewed, the code that assigns the gas limit to the MsgValueSimulator call is contained in the VM and was not in scope. The Matter Labs team is encouraged to introduce tests for the possible use cases to ensure they behave as expected. These should include the .transfer and .send operations as well as regular value-bearing calls.
The pull request also introduces a minor optimization where precompiles that are only used to burn gas do not validate that there is sufficient gas, since no actual work will be completed anyway.
The second scope introduces a new L2Weth contract, which functions like the Layer 1 (L1) version except:
IL2StandardToken interface.It also makes some incremental improvements to the token bridge:
The basic trust assumptions of the system have not been changed by these modifications. In particular:
Weth contract.In addition to configuring and managing the system, these powers imply that the governor currently has access to all the funds in the system.
The L1ERC20Bridge contract prevents deposits over a configurable limit for some tokens. This is achieved by recording deposits for deposit-limited tokens, rewinding the update if the deposit failed, and ignoring tokens that are not limited. This introduces an edge case that could lead to loss of funds:
Consider simply clearing the deposit record if the amount to claim exceeds the recorded value. Alternatively, consider recording deposits for all tokens, whether or not they have a deposit limit.
Update: Acknowledged, not resolved. The Matter Labs team stated:
The deposit limiting feature is currently turned off in the network. We will revisit this issue if we decide to turn on the feature.
It is not an uncommon practice to use abi.encodeWithSignature or abi.encodeWithSelector to generate calldata for a low-level call. However, the first option is not typo-safe and the second option is not type-safe. The result is that both of these methods are error-prone and should be considered unsafe.
On line 151 of L2ERC20Bridge.sol an unsafe ABI encoding is used.
Consider replacing this occurrence with abi.encodeCall, which checks whether the supplied values actually match the types expected by the called function and also avoids errors caused by typos. Note that this will require a version greater than 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.
Update: Resolved in pull request #132 at commit fb2ab16.
require StatementsThroughout the codebase there are require statements that lack error messages:
require statement on line 46 of L1Messenger.solrequire statement on line 168 of SystemContractHelper.solConsider including specific, informative error messages in require statements to improve overall code clarity and facilitate troubleshooting whenever a requirement is not satisfied.
Update: Resolved in pull request #265 at commit 589d582.
When users transfer ERC-20 tokens over the bridge, they consume part of their deposit limit, which is never replaced. This introduces the possibility that an address can reach the limit and be unable to use the bridge until the limit is increased or removed for everybody. It is likely that the limits will be configured to exceed most legitimate use cases.
Nevertheless, in the interest of flexibility, consider introducing a mechanism for the administrator to reset or replenish a user's allowance.
Update: Acknowledged, not resolved. The Matter Labs team stated:
The deposit limiting feature is currently turned off in the network. We will revisit this issue if we decide to turn on the feature.
The BridgeInitializationHelper contract sets the message sender as the refund recipient for its cross-domain messages. This pattern assumes that the message sender will independently recover the funds if necessary. However, in this case, the message sender is the L1ERC20Bridge contract, which has no mechanism to initiate the withdrawal. Therefore, if either the implementation or proxy deployment transaction fails, the deployment fee will be trapped on Layer 2.
Consider using the governor address as the refund recipient instead.
Update: Resolved in pull request #136 at commit 0437f63.
The following parts of the codebase are lacking documentation:
BridgeInitializationHelper contract:
requestDeployTransaction function is missing the @param statement for the _zkSync parameter.L1ERC20Bridge contract:
initialize function is missing the @param statements for the fee parameters.Mailbox contract:
l2TransactionBaseCost function is missing all @param statements._deriveL2GasPrice function is missing the @return statement._getOverheadForTransaction function is missing the @param statements for two parameters.L2ContractHelper.sol file:
The following documentation is misleading:
L2Weth contract:
initialize function's documentation claims it stores the L1 address of the bridge, but it does not.bridgeMint function's documentation references the StandardToken interface instead of the IL2StandardToken interface. It should also indicate that the function always reverts.bridgeMint function parameter is annotated with "_to" but the parameter is named _account.bridgeBurn function claims to burn tokens from "_from" WETH contract instead of the "_from" address.L1ERC20Bridge contract:
l2TokenBeacon variable is described as a factory, but it is actually a beacon.Consider including thorough and correct docstrings and inline explanations with references to relevant source files or documentation, allowing readers or maintainers to verify the implementations and their correct usage.
Update: Resolved in pull request #135 at commit c415ce5.
Throughout the codebase there are state variables that lack an explicitly declared visibility:
allowList in L1ERC20Bridge.solzkSync in L1ERC20Bridge.soll2TokenProxyBytecodeHash in L2ERC20Bridge.solFor clarity, consider always explicitly declaring the visibility of variables, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #140 at commit 459918a.
Throughout the codebase there are imports that are unused and could be removed:
MAX_MSG_VALUE of MsgValueSimulator.solMSG_VALUE_SYSTEM_CONTRACT of SystemContractHelper.solUtils of SystemContractHelper.solConsider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #269 at commit b868f4a.
Within SystemContractHelper.sol, both the SystemContractHelper library and the abstract contract ISystemContract are declared.
Consider separating the contracts into their own files to make the codebase easier to understand for developers and reviewers.
Update: Resolved in pull request #270 at commit 5a48644.
Throughout the codebase we identified the following typographical errors:
zkSync v2.0 → zkSync Era in L1ERC20Bridge.soldo → does in L1ERC20Bridge.solfinalisation → finalization in L1ERC20Bridge.sol and in Mailbox.sol for consistencycan not → cannot in L2Weth.solConsider correcting these typographical errors.
Update: Resolved in pull request #138 at commit 4f984e6.
To favor explicitness and readability, there are two locations in the contracts that may benefit from better naming:
requestDeployTransaction is internal and thus could be prepended with an underscore. Consider renaming the function to _requestDeployTransaction.unspendGas could be renamed to unspentGas.Consider renaming these variables to be more consistent.
Update: Resolved in pull request #267 at commit 2382fa2. The Matter Labs team stated:
Our convention is not to prefix internal methods with an underscore in libraries, so we will not change the naming of
requestDeployTransaction.
assert for Inviolable ConditionsWhen finalizing deposits on Layer 2, the L2ERC20Bridge contract checks one of two consistency conditions. Since these should never fail for any user input, consider using assert statements instead of require statements. This indicates that the conditions should be inviolable, and is better suited for analysis tools that attempt to prove this claim or identify counterexamples.
Update: Acknowledged, not resolved. The Matter Labs team stated:
Although
assertmight logically seem like a better option,requireis easier to debug. We will leave therequirestatement in place for now.
When constructing Layer 1 (L1) to Layer 2 (L2) messages, the Mailbox contract aliases the refund recipient if it corresponds to a deployed L1 smart contract. This makes sense when the address is known to be associated with that smart contract, such as when it is initialized to the message sender. However, if it is chosen by the user to be an L2 address, applying the alias could bypass the user's intention.
In particular, users will be unable to choose an L2 refund recipient that has the same address as an existing L1 smart contract. If a user inputs this address as the refund recipient, it will be aliased and the intended L2 contract will be unable to control the funds. Instead, the funds would be controlled by the L1 smart contract, which the user may not even control.
In the current deployment, this scenario should be impossible because L1 and L2 addresses are derived differently, but this decision is not settled. In the interest of predictability, consider leaving the refund recipient unaliased if it is explicitly chosen by the user to be an L2 address.
Update: Acknowledged, not resolved. The Matter Labs team stated:
We will take this into account when and if we change the address derivation rule.
This audit was conducted over the course of a week, and we uncovered a handful of medium and lower-severity issues.
While audits help in identifying code-level issues in the current implementation and potentially the code deployed in production, the Matter Labs team is encouraged 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. Below is a new recommendation to augment the ones provided in previous audits.
Medium: It was previously recommended to monitor 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. In addition, consider identifying whenever a user approaches or reaches the new deposit limit for any token. This could also indicate unusual behavior or an ongoing attack, or it could imply that the token deposit limit is too restrictive.