Table of Contents
- Table of Contents
- Summary
- Scope
- System Overview
- Security Model and Trust Assumptions
- Low Severity
- The Domain of The Permit Functionality Is Not Initialized
- Claim Cap Is Not Enforced on Merkle Distributor
- Claim Can Be Prevented by Anyone if Front-Ran
- Not All Delegation Parameters Are Signed
- Voting Checkpoints Refer to Block Numbers Despite Unstable Block Production Frequency
- Problematic Edge Cases When Using delegateBySig
- Notes & Additional Information
- Recommendations
- Conclusion
Summary
- Type
- Token
- Timeline
- From 2024-03-11
- To 2024-03-22
- Languages
- Solidity
- Total Issues
- 13 (10 resolved, 1 partially resolved)
- Critical Severity Issues
- 0 (0 resolved)
- High Severity Issues
- 0 (0 resolved)
- Medium Severity Issues
- 0 (0 resolved)
- Low Severity Issues
- 6 (6 resolved)
- Notes & Additional Information
- 7 (4 resolved, 1 partially resolved)
Scope
We audited the zksync-association/zk-governance repository at commit 08ec4e7.
In scope were the following files:
src
├── ZkCappedMinter.sol
├── ZkMerkleDistributor.sol
└── ZkTokenV1.sol
Note that some issues might refer to the f37e3dc commit which was the audit's early commit until the final one was delivered.
System Overview
The ZkTokenV1
contract is the core of the system and is going to be the governance token of ZKsync Era. It is an ERC-20 token with permit functionality which incorporates a voting delegation system so that token holders can delegate their voting power to a trusted representative while preserving the actual token value. In order to activate their voting power, each token holder must set a delegatee even if they are delegating to their own account address. Upon a token transfer, the corresponding voting power is also automatically transferred from and to the corresponding delegatees, respectively.
The token contract is a transparent upgradeable proxy and also has an access control mechanism. The default admin role is allowed to create other access roles, assign or revoke these roles, and also set a separate and dedicated admin role for each role. By default, all defined roles are managed by the default admin role.
In order to bootstrap the token's circulation, a specific amount of tokens will be minted and distributed among Matter Labs, the Foundation, and the Association, with an airdrop taking place as well. The token distribution involves two more contracts, namely ZkCappedMinter
and ZkMerkleDistributor
. These contracts are going to be assigned the MINTER
role in the access control system of ZkTokenV1
and will control the token minting among the beneficiaries.
More specifically, two instances of the ZkCappedMinter
contract will be deployed: one for the Foundation and another for the Association. ZkCappedMinter
defines an immutable admin and an immutable cap value. The admin is able to mint tokens to any address at any frequency, up to the token mint cap.
The ZkMerkleDistributor
contract will be used to control token distribution among the beneficiaries of the public airdrop. An immutable Merkle root value will be set upon the contract's deployment, committing to all information about the qualifying addresses and amounts. Users will be allowed to claim their airdropped tokens within a specific time window. In order to claim successfully, users should provide a valid Merkle proof and a delegatee account address. When the specified claiming period is over, the contract admin is allowed to sweep any airdrop amount left unclaimed and transfer it to an address of their preference.
Security Model and Trust Assumptions
The system's security ultimately relies on the honest behavior of the initial admin of ZkTokenV1
. Upon the contract's initialization, the admin is granted the DEFAULT_ADMIN_ROLE
, the MINTER_ADMIN_ROLE
, and the BURNER_ADMIN_ROLE
roles, along with the ownership of the proxy. As a consequence, the admin is allowed to grant or revoke token minting and burning rights to any address. The admin also can upgrade the token by changing the implementation address.
The ZKsync Association team clarified that the initial admin will be the Association's 4-out-of-7 multisig with a security precaution of having one key per person, with each key being a hardware key and not being used for any other purpose. According to the plan, once the governance system has been successfully bootstrapped and the proposals procedure is functional, all three admin roles will be handed over to a governance contract and the Association will subsequently lose all three roles. During the audit, we considered that the Association will act honestly and in accordance with the interests of the community until the admin roles are handed over to the governance.
Regarding the distribution of the token's supply, an initial amount of funds will be minted upon contract initialization for Matter Labs. There will be two instances of the ZkCappedMinter
contract, one with the Association being the admin and the other with the Foundation being the admin. The ZkCappedMinter
contract will control the minting for these two entities, allowing them to mint up to a defined cap amount at the frequency and the recipient(s) of their preference. The airdrop distribution will be fully controlled by the ZkMerkleDistributor
contract, where an immutable Merkle root will be the only source of truth regarding the eligible accounts and the corresponding amounts. The ZKsync Association team shared their willingness to publish the data required to reconstruct the Merkle tree so that anyone can verify the correctness of the Merkle root.
Low Severity
The Domain of The Permit Functionality Is Not Initialized
The initialize
function of the ZkTokenV1
contract does not call the initializer of the ERC20PermitUpgradeable
base contract. This could impact user experience when approving using the permit functionality as there will be no readable name for the signing domain.
Consider initializing the ERC20PermitUpgradeable
base contract.
Update: Resolved at commit 63bdcc9.
Claim Cap Is Not Enforced on Merkle Distributor
The _claim
function is supposed to verify that the total claimed amount is below the maximum total claimable amount according to the specification. However, it checks a particular amount instead.
Consider checking the total claimed amount instead of a particular amount.
Update: Resolved at commit f9b1389.
Claim Can Be Prevented by Anyone if Front-Ran
Both the claim
and claimOnBehalf
functions delegate the claimed amount at the end of their execution. However, it is possible to extract the _delegateInfo
parameter and front-run this transaction by calling the delegateBySig
function on the token directly. This way, the nonce is incremented due to which the whole claim transaction reverts when the delegation is attempted.
Consider redesigning the requirement for delegation upon claim or calling the delegateBySig
function in a try-catch
block.
Update: Resolved at commit e2f3209.
Not All Delegation Parameters Are Signed
From all the DelegateInfo
parameters, only the delegatee
parameter is signed. As such, it is possible to front-run the transaction, change the signature to oneself while keeping the delegatee, and claim the airdrop but skip the original delegation. This way, the original transaction is fulfilled only partially because the airdrop is correctly claimed but the delegation is not correctly set.
Consider signing all the DelegateInfo
parameters.
Update: Resolved at commits d2116b3 and 36c6145. All the DelegateInfo
parameters are signed except for the expiry
which is ignored in favour of the claim signature's expiry.
Voting Checkpoints Refer to Block Numbers Despite Unstable Block Production Frequency
The ZkTokenV1
is the governance token that will be used for voting during an open governance proposal. For vote counting, the governor will read the users' voting power checkpoint with respect to the associated voting period. According to the current default, checkpoints are created per block numbers which will, in turn, enforce the proposals' voting period to be expressed in block numbers. However, in ZKsync Era, the block production time is not stable and it may be confusing for users to predict the voting checkpoint so as to vote in time.
In order to avoid confusion and enhance user experience, consider modifying the vote checkpoint unit to block timestamp.
Update: Resolved at commit a4a45fe.
Problematic Edge Cases When Using delegateBySig
To encourage governance participation through delegation, the delegateBySig
function is called together with a token claim. However, delegateBySig
works only for EOA signatures and does not support ERC-1271 nor account abstraction due to backwards compatibility reasons. Given that account abstraction is common on ZKsync Era, the feature might not work as expected for some accounts.
Consider choosing another way of encouraging governance participation that will support EOAs as well as smart contracts and account abstraction.
Update: Resolved at commit 4587190.
Notes & Additional Information
Initializer Is Not Disabled on Implementation
The ZkTokenV1
implementation is left uninitialized which allows anyone to initialize it with arbitrary values.
While this does not lead to security issues in this case, consider calling the _disableInitializers
function in ZkTokenV1
's constructor to disable the initializer.
Update: Acknowledged, not resolved. The ScopeLift team stated:
When deployed, the token contract will be initialized in the same transaction.
Same nonces
Mapping Is Used for Vote Delegation and Permit
The ERC20VotesUpgradeable
and ERC20PermitUpgradeable
base contracts of ZkTokenV1
use the same nonces
mapping. As a consequence, it would be invalid to include the same nonce in a delegate-by-signature message and in a subsequent permit message or vice versa.
Consider clearly documenting that the nonces produced for both delegation and permit actions should respect a single order so as to avoid failing transactions.
Update: Resolved at commit 604fdf9. Further docstrings were added to clarify how to use the nonces of both actions correctly.
Missing Input Validation
The _windowStart
timestamp must be less than _windowEnd
. Otherwise, the ZkMerkleDistributor
contract will not work according to the specification. However, this is not enforced in the contract.
Consider verifying that the start timestamp is less than the end timestamp in the constructor.
Update: Acknowledged, not resolved. The ScopeLift team stated:
If misconfigured, which is unlikely, we will deploy a new contract.
Code Clarity Suggestions
Throughout the codebase, a number of instances were identified where code clarity could be improved:
- Since the
BitMap
methods are bound to theclaimedBitMap
variable, it is possible to use a shorter syntax for these methods. For example,BitMaps.get(claimedBitMap, _index)
can beclaimedBitMap.get(_index)
. Otherwise, binding methods is not necessary. - The
toTypedDataHash
function can be used to get the typed data hash in theclaimOnBehalf
function for succinctness. - The
verify
function is used even though the proof is a calldata. Consider using theverifyCalldata
function.
Update: Partially resolved at commit 604fdf9. The claimAndDelegateOnBehalf
function does not use the toTypedDataHash
function.
Incomplete Docstrings
Within ZkMerkleDistributor.sol
, not all return values of the isClaimed function are documented.
Consider thoroughly documenting all functions/events (and their parameters or return values) that are part of a contract's public API. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
Update: Resolved at commit 604fdf9.
Floating Pragma
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Throughout the codebase, there are multiple floating pragma directives:
IMintable.sol
has thesolidity ^0.8.24
floating pragma directive.IMintableAndDelegatable.sol
has thesolidity ^0.8.24
floating pragma directive.
Consider using fixed pragma directives.
Update: Resolved at commit 604fdf9.
Lack of Security Contact
Providing a specific security contact (such as an email or ENS name) within a smart contract significantly simplifies the process for individuals to communicate if they identify a vulnerability in the code. This practice proves beneficial as it permits the code owners to dictate the communication channel for vulnerability disclosure, eliminating the risk of miscommunication or failure to report due to a lack of knowledge on how to do so. In addition, if the contract incorporates third-party libraries and a bug surfaces in those, it becomes easier for their maintainers to contact the appropriate person about the problem and provide mitigation instructions.
Throughout the codebase, there are contracts that do not have a security contact:
Consider adding a NatSpec comment containing a security contact above the contract definitions. Using the @custom:security-contact
convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Resolved at commit 604fdf9.
Recommendations
Deployment Suggestions
The system uses the OpenZeppelin/openzeppelin-foundry-upgrades library for deployments. This library was developed to be compatible with EVM chains but was not thoroughly tested for ZKsync Era deployments.
Given that ZKsync Era has some differences compared to Ethereum and uses its own versions of the Foundry toolkit and the Solidity compiler, consider thoroughly testing the deployment on ZKsync Era testnet before deploying to the ZKsync Era mainnet.
The ScopeLift team responded:
We modified our deployment scripts to use hardhat with ZKsync's plugins (at commit a26da8a). This change required us to downgrade to OZ Contracts v4.9.
As a result of the downgrade to OZ Contracts v4.9, OpenZeppelin assessed the degree of the changes and did not identify any issues. The code level changes of the library contracts used in this project are backwards compatible.
Conclusion
The audit only yielded low-severity issues which indicates an overall healthy design and implementation. Several fixes were suggested to address edge cases and improve the usability, robustness, and clarity of the codebase. The in-scope contracts were inspected, taking into consideration the integrated libraries and the specifics of the ZKsync Era ecosystem.
The ScopeLift team, who developed the codebase for ZKsync Association, provided a detailed specification of the system and sufficiently covered the codebase with tests. Both ZKsync Association and ScopeLift teams were very responsive and engaged in discussions with the audit team.