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
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.
The HubPool contract is supposed to administer the ZkSync_SpokePool contract.
The admin of the ZkSync_SpokePool contract can change the zkSync bridge address.
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.
UPPER_CASE FormatIn ZkSync_Adapter.sol there are constants not using UPPER_CASE format. For instance:
l2GasLimit constant declared on line 75l1GasToL2GasPerPubDataLimit constant declared on line 80l2RefundAddress constant declared on line 85zkSync constant declared on line 91zkErc20Bridge constant declared on line 93According 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.
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.
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.
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.
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 CommentsThe 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:
TODO comment on line 15 in ZkSync_SpokePool.solTODO comment on line 95 in ZkSync_Adapter.solDuring 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.
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.