We audited the zksync-association/zk-governance repository at commit 70011bb.
In scope were the following files:
contracts
├── interfaces/
│ ├── IAADistributorPaymaster.sol
│ ├── IPaymaster.sol
│ ├── IPaymasterFlow.sol
│ ├── ISignatureBasedPaymaster.sol
│ └── IZkMerkleDistributor.sol
├── AADistributorPaymaster.sol
├── Constants.sol
└── SignatureBasedPaymaster.sol
The ZKsync Association team is conducting a token airdrop and intends to cover the gas costs associated with claiming and delegating these tokens. This improves the user experience by eliminating the need for participants to pay their own fees or to have ZKsync ETH in their wallets at the time of claiming. There are two expected use cases and the code under review contains a supporting paymaster contract for each one.
In the first case, the ZKsync Association team will run an off-chain service to collect user signatures which will allow them to claim and delegate tokens on behalf of the users. In this way, ZKsync Association will submit the ZKsync transactions and pay the associated gas costs. However, for security and convenience reasons, these transactions will be executed in parallel from multiple different Matter Labs "sender" accounts. The SignatureBasedPaymaster contract can be used to identify and sponsor any transaction sent from an authorized sender account. It uses a hierarchical key architecture so that more secure keys can authorize and revoke the operational senders.
However, some airdrop recipients may be account abstraction wallets that cannot support the off-chain use case, either because they do not fully conform to ERC-1271 (which would be necessary for a ZKsync Association sender account to act on their behalf) or their frontend user interface does not expose this functionality. This brings us to the second case. In this scenario, they can use the AADistributorPaymaster contract which will sponsor the gas costs of arbitrary transactions, provided:
The signature-based paymaster and account abstraction paymaster have different trust assumptions and privileged roles:
SignatureBasedPaymasterSignatureBasedPaymaster has an owner and signer address.AADistributorPaymasterAADistributorPaymaster has an owner address.If the owner or signer calls cancelNonce but the spender's transaction that uses the active nonce is confirmed first, this call will incorrectly cancel the next nonce.
Consider providing the target nonce to the cancelNonce function and reverting whenever it is not the expected value.
Update: Resolved at commit 79e005e.
The second NonceCanceled event parameter is named newNonce but it corresponds to the nonce that is canceled.
Consider renaming it to describe the relevant nonce.
Update: Resolved at commit 1dcf101.
Throughout the codebase, there are multiple instances of incomplete docstrings. For instance, the postTransaction function in IPaymaster.sol, and the _txHash and _suggestedSignedHash parameters are not documented. In addition, for the domainSeparator function in SignatureBasedPaymaster.sol, the return value is not 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 e002b6e.
The following typographical errors were identified in the codebase. Consider correcting them.
Update: Resolved at commit f6a605c.
The try-catch logic is only used to choose between two scenarios: either a new signature needs to be processed or the sender is relying on a previous signature. The intermediate case, where a provided buffer cannot be decoded correctly and is therefore ignored, does not appear to be a valid use case. As such, instead of executing the catch block in this scenario, it would be simpler and more natural for the function to revert.
Consider only attempting to decode and process the signature when the innerInputs buffer is not empty. In either case, the approvedSenders mapping can be checked afterwards (outside the if statement).
Update: Resolved at commit a67e188.
The uint256 casts for the transaction from and to parameters are redundant since they already have type uint256.
For simplicity, consider removing the redundant cast.
Update: Resolved at commit 1f9c0f1.
immutableIf a variable is only ever assigned a value from within the constructor of a contract, then it could be declared as immutable.
Within AADistributorPaymaster.sol, there are variables that could be immutable:
To better convey the intended use of variables and to potentially save gas, consider adding the immutable keyword to variables that are only set in the constructor.
Update: Resolved not an issue. The ZKsync Association team stated:
Acknowledged. In ZKsync using immutable is a bit more expensive than direct storage access. Due to this reason, we decided to leave the current approach.
Throughout the codebase, we identified instances where require statements are used instead of revert messages with custom errors.
Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed.
For conciseness and gas savings, consider replacing require and revert messages with custom errors.
Update: Acknowledged, not resolved. The ZKsync Association team stated:
Acknowledged.
Throughout the codebase, there are various functions with unnecessarily permissive visibility:
setMaxPaidTransactionsPerAccount function in AADistributorPaymaster.sol with PUBLIC visibility could be limited to EXTERNAL.setMaxSponsoredEth function in AADistributorPaymaster.sol with PUBLIC visibility could be limited to EXTERNAL.withdraw function in AADistributorPaymaster.sol with PUBLIC visibility could be limited to EXTERNAL.withdraw function in SignatureBasedPaymaster.sol with PUBLIC visibility could be limited to EXTERNAL.To better convey the intended use of functions and to potentially realize some additional gas savings, consider changing a function's visibility to be only as permissive as required.
Update: Resolved at commit ba8d36a.
While audits help in identifying potential security risks, the Matter Labs team is encouraged to also incorporate automated monitoring of on-chain contract activity into their operations. Ongoing monitoring of deployed contracts helps in identifying potential threats and issues affecting the production environment.
setMaxPaidTransactionsPerAccount such that genuine users would never run out of transactions while minimizing the harm done by malicious users who may be taking advantage of the gas sponsorship in AADistributorPaymaster.setMaxSponsoredEth is suitable for the current gas prices.AADistributorPaymaster does not assume many behaviors from account abstraction wallets. However, these wallets may still have unexpected properties which either do not comply with the paymaster's expectations or can make claiming through the wallet user interface counterintuitive. It is recommended to investigate reports of users having difficulty claiming their airdrop, especially from account abstraction wallets.The codebase allows ZKsync to cover the gas costs associated with airdrop claiming for both EOAs and account abstraction wallets. The code was of a high quality and only fixes for minor issues were suggested to further improve the codebase. We appreciate the ZKsync Association team for providing us with detailed documentation and being very responsive in answering any questions we had about the project.