OpenZeppelin
Skip to content

zkSync Bridge and .transfer & .send Diff Audit

OpenZeppelin Security | January 29, 2024

Table of Contents

Summary

Type
Layer 2
Timeline
From 2023-04-24
To 2023-05-01
Languages
Solidity
Total Issues
13 (9 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
0 (0 resolved)
Medium Severity Issues
1 (0 resolved)
Low Severity Issues
5 (4 resolved)
Notes & Additional Information
7 (5 resolved)
Client Reported Issues
0 (0 resolved)

Scope

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

System Overview

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:

  • It follows the IL2StandardToken interface.
  • It does not have a silent fallback function that implements risky "phantom" functions, although it can still receive L2 ETH directly.
  • It includes native permit functionality.
  • It is upgradeable for now.

It also makes some incremental improvements to the token bridge:

  • It allows users to specify an L2 refund recipient address to receive the funds if the deposit fails.
  • There is an administrator-controlled limit on the total deposit amount per L1 token per user that can cross the bridge.

Security Model and Privileged Roles

The basic trust assumptions of the system have not been changed by these modifications. In particular:

  • The governor address is able to upgrade the bridge contracts, and the L2 token contracts.
  • The governor can also upgrade the new Weth contract.
  • The governor also controls the AllowList that determines:
    • Which addresses can use the bridge.
    • The deposit limit per L1 token per user.

In addition to configuring and managing the system, these powers imply that the governor currently has access to all the funds in the system.

 

Medium Severity

Fragile Deposit Limit

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:

  • A user initiates a deposit that will fail to execute on L2 using a token with no deposit limit. Therefore, the deposit is not recorded.
  • A deposit limit is introduced for the token.
  • The user will be unable to claim the failed deposit, because the attempt to rewind the (non-existent) deposit update will fail.

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.

Low Severity

Unsafe ABI Encoding

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.

Missing Error Messages in require Statements

Throughout the codebase there are require statements that lack error messages:

Consider 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.

Unreplenishable Deposit Limit

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.

Bridge Cannot Recover Funds

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.

Missing and Misleading Documentation

The following parts of the codebase are lacking documentation:

  • In the BridgeInitializationHelper contract:
    • The requestDeployTransaction function is missing the @param statement for the _zkSync parameter.
  • In the L1ERC20Bridge contract:
    • The initialize function is missing the @param statements for the fee parameters.
  • In the Mailbox contract:
  • In the L2ContractHelper.sol file:
    • The library functions, interfaces, and struct are missing documentation.

The following documentation is misleading:

  • In the L2Weth contract:
    • The initialize function's documentation claims it stores the L1 address of the bridge, but it does not.
    • The bridgeMint function's documentation references the StandardToken interface instead of the IL2StandardToken interface. It should also indicate that the function always reverts.
    • The first bridgeMint function parameter is annotated with "_to" but the parameter is named _account.
    • The bridgeBurn function claims to burn tokens from "_from" WETH contract instead of the "_from" address.
  • In the L1ERC20Bridge contract:
    • The 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.

Notes & Additional Information

State Variable Visibility Not Explicitly Declared

Throughout the codebase there are state variables that lack an explicitly declared visibility:

For 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.

Unused Imports

Throughout the codebase there are imports that are unused and could be removed:

Consider removing unused imports to improve the overall clarity and readability of the codebase.

Update: Resolved in pull request #269 at commit b868f4a.

Multiple Declarations per File

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.

Typographical Errors

Throughout the codebase we identified the following typographical errors:

Consider correcting these typographical errors.

Update: Resolved in pull request #138 at commit 4f984e6.

Naming Suggestions

To favor explicitness and readability, there are two locations in the contracts that may benefit from better naming:

  • The function requestDeployTransaction is internal and thus could be prepended with an underscore. Consider renaming the function to _requestDeployTransaction.
  • The variable 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.

Use assert for Inviolable Conditions

When 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 assert might logically seem like a better option, require is easier to debug. We will leave the require statement in place for now.

Unexpected Address Aliasing

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.

 

Conclusions

This audit was conducted over the course of a week, and we uncovered a handful of medium and lower-severity issues.

 

Appendix

Monitoring Recommendations

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.

Financial

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.

Related Posts