Table of Contents
Summary
- Type
- Bridge
- Timeline
- From 2023-07-17
- To 2023-07-21
- Languages
- Solidity
- Total Issues
- 7 (6 resolved, 1 partially resolved)
- Critical Severity Issues
- 0 (0 resolved)
- High Severity Issues
- 0 (0 resolved)
- Medium Severity Issues
- 1 (1 resolved)
- Low Severity Issues
- 0 (0 resolved)
- Notes & Additional Information
- 6 (5 resolved, 1 partially resolved)
- Client Reported Issues
- 0 (0 resolved)
Scope
We audited the across-protocol/contracts-v2 repository at the 2f649b1fecb0b32aa500373a8b8b0804e0c98cd2 commit.
In scope were the following contracts:
contracts
├── ZkSync_SpokePool.sol
└── chain-adapters
└── ZkSync_Adapter.sol
System Overview
The two in-scope contracts allow the Across protocol to operate on zkSync Era.
The ZkSync_SpokePool contract extends the functionality of the SpokePool contract, specifically designed to facilitate deposits, refunds, relays, and slow relays on the zkSync Era's side. Its main modifications to the SpokePool behavior include wrapping ETH to WETH during refunds and slow relays, implementing address aliasing within the onlyAdmin modifier, and enabling the ability to change the zkSync bridge address.
The ZkSync_Adapter contract allows HubPool to relay messages and tokens to ZkSync_SpokePool. It uses zkSync ERC-20 bridge for ERC-20 token transfers except for WETH and zkSync messaging for WETH transfers and messaging.
For a more detailed description of the protocol, refer to our previous audit report for Across V2.
Security Model and Trust Assumptions
The HubPool contract is supposed to administer the ZkSync_SpokePool contract.
Privileged Roles
The admin of the ZkSync_SpokePool contract can change the zkSync bridge address.
Medium Severity
HubPool Might Stop Executing Bundles if Deposit Limits Are Enabled for ZkSync
If zkSync enables deposit limit and the HubPool contract hits the limit then the Across protocol will partially stop working because the root bundle could not be executed. The Across protocol assumes that the limit can be bypassed by splitting a deposit into multiple chunks.
However, this is not the case as the limit specifies the total amount of tokens bridged but not the per-deposit amount. Thus if the limit is hit it will not be possible to bypass it by splitting the deposit into smaller chunks. Furthermore, the attacker can trigger this scenario by increasing the total amount deposited from Across to zkSync by choosing zkSync as the repayment chain.
Consider changing how the limit hit is handled by the Across protocol.
Update: Resolved in pull request #328 at commit fd6c17b.
Notes & Additional Information
Constants Not Using UPPER_CASE Format
In ZkSync_Adapter.sol there are constants not using UPPER_CASE format. For instance:
- The
l2GasLimitconstant declared on line 75 - The
l1GasToL2GasPerPubDataLimitconstant declared on line 80 - The
l2RefundAddressconstant declared on line 85 - The
zkSyncconstant declared on line 91 - The
zkErc20Bridgeconstant declared on line 93
According to the Solidity Style Guide, constants should be named with all capital letters with underscores separating words. For better readability, consider following this convention.
Update: Partially resolved in pull request #322 at commit ce6e2e1. The names of the constants that are actually interfaces were not changed because the camel case better reflects their purpose.
Incomplete Event Emissions
The SetZkBridge event should emit both the old bridge address and the new bridge address.
When making modifications to state variables, it is essential to enhance the functionality of off-chain services in searching for and filtering specific events. To achieve this, consider consistently emitting the old and new values during state variable changes. By doing so, off-chain services can have better visibility and track the evolution of these variables over time.
Furthermore, it is recommended to adopt this pattern in the scope of this audit and other contracts beyond its scope. This approach promotes consistency and standardization throughout the codebase.
Update: Resolved in pull request #327 at commit fa3b55b.
Lack of Documentation
The l2RefundAddress address is used to receive refunds from L2 transactions. However, it is not clear what its role is and how it is managed.
Consider documenting the address's purpose and how it is managed.
Update: Resolved in pull request #326 at commit ea0a27f.
Misleading Comments
The comment in the ZkSync_SpokePool contract states that ETH on zkSync is an ERC-20. However, that is not completely true. While ETH on zkSync has balanceOf, decimals, and some other ERC-20 functions it does not provide functions such as transfer, approve, and transferFrom.
Consider adjusting the comment.
Update: Resolved in pull request #323 at commit d6c610e.
Style Inconsistencies
Throughout the codebase, events are emitted at the end of functions. However, the ZkSyncTokensBridged event does not follow this pattern and is emitted at the beginning of the function.
Consider emitting it at the end of the functions to improve the codebase's consistency.
Update: Resolved in pull request #324 at commit 7cce6fb.
TODO Comments
The following instances of TODO comments were found in the codebase.
These comments should be tracked in the project's issue backlog and resolved before the system is deployed:
- The
TODOcomment on line 15 in ZkSync_SpokePool.sol - The
TODOcomment on line 95 in ZkSync_Adapter.sol
During development, having well-described TODO comments will make the process of tracking and solving them easier. Without this information, these comments might age and important information for the security of the system might be forgotten by the time it is released to production.
Consider removing all instances of TODO comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO to the corresponding issues backlog entry.
Update: Resolved in pull request #325 at commit b2abd5d.
Conclusion
One medium-severity issue was identified in this audit, along with some notes to improve the clarity and consistency of the codebase. Some changes were proposed to ensure smart contract security best practices are followed.
