Table of Contents
- Table of Contents
- Summary
- Scope
- System Overview
- Security Model and Trust Assumptions
- Design Choices, Limitations, and Integration Issues
- Medium Severity
- Potential Denial-of-Service While Iterating Hooks
- Front-Running Scenarios During Key Registration
- Guardian Can Overwrite Recovery Process and Render It Useless
- Failure to Clear pendingRecoveryData in onUninstall Allows Immediate Account Recovery Upon Reconnection
- Incomplete Recovery Process Due to Missing Validator Attachment
- Uninstall Process Might Revert Due to Pending Guardian Acceptance
- Low Severity
- Possible Initialization of an Unusable SsoAccount
- Potential Panic During Data Slicing in _executeCall
- Builtin Getter For pendingRecoveryData Does Not Return rawPublicKey
- Misleading Error Message in addValidationKey
- Deviation From Specifications
- Missing Checks for Function Arguments
- Floating Pragma
- Lack of Public Key Validation
- Missing or Incomplete Documentation
- Notes & Additional Information
- Non-Standardized Storage Location for SSO_STORAGE_SLOT
- Unused Code
- Redundant Storage Access
- Redundant Getter for publicKeys in WebAuthValidator
- Inconsistencies Between Interface and Implementation
- IGuardianRecoveryValidator Is Not Optimized For Development
- Refactor Opportunity Over Address Casting
- Inconsistent Types for Time-Related Variables
- Misleading Function Naming in GuardianRecoveryValidator
- Use Custom Errors
- Missing Named Parameters in Mappings
- Lack of Security Contact
- Prefix Increment Operator Can Save Gas in Loops
- Inconsistent Order Within Contracts
- Function Visibility Overly Permissive
- Use calldata Instead of memory
- Suboptimal ERC-165 Interface Check Implementation
- Typographical Error
- Redundant Hashing Operations in webAuthVerify
- Recommendations
- Conclusion
Summary
- Type
- Account Abstraction
- Timeline
- From 2025-03-05
- To 2025-03-17
- Languages
- Solidity
- Total Issues
- 34 (28 resolved, 3 partially resolved)
- Critical Severity Issues
- 0 (0 resolved)
- High Severity Issues
- 0 (0 resolved)
- Medium Severity Issues
- 6 (2 resolved, 3 partially resolved)
- Low Severity Issues
- 9 (7 resolved)
- Notes & Additional Information
- 19 (19 resolved)
Scope
We audited the matter-labs/zksync-sso-clave-contracts repository at commit c7714c0.
In scope were the following files:
src
├── TransparentProxy.sol
├── interfaces
│ └── IGuardianRecoveryValidator.sol
└── validators
├── GuardianRecoveryValidator.sol
└── WebAuthValidator.sol
A few issues in out-of-scope files of the repository were also identified.
System Overview
The code under review introduces changes to the SsoAccount
contract, a more customizable smart account that plugs into the existing bootloader
Account Abstraction (AA) framework. Under review are the incremental changes made since the last audit of the protocol:
TransparentProxy.sol
: The functionality to pass data during the construction stage, which will then be used alongside the implementation's code, has been added.IGuardianRecoveryValidator.sol
: A new interface meant for the new guardian validator module, respecting theIModuleValidator
interface definition, has been added.GuardianRecoveryValidator.sol
: A new implementation of the guardian validator has been added.WebAuthValidator.sol
: An adaptation of the originalWebAuthValidator
contract has been added to allow multipass keys, primarily due to the introduction of acredentialId
into the structure of the storage.
GuardianRecoveryValidator
The validator is in charge of providing a mechanism to recover access to an SsoAccount
in case any other validation method has been lost. The procedure to set up an account and use it as a safety measure consists of the following steps:
- Attach the
SsoAccount
contract to theWebAuthValidator
andGuardianRecoveryValidator
contracts by adding them as module validators. - Invoke the
proposeValidationKey
function of theGuardianRecoveryValidator
contract to propose a guardian throughout theSsoAccount
contract's transaction execution workflow. - The guardian must accept the proposal from their account by invoking the
addValidationKey
function of theGuardianRecoveryValidator
contract.
Then, in case of the need for a recovery, the guardian for that particular SsoAccount
is in charge of initiating a recovery by passing the respective account, and the hashed originDomain
and credentialId
values alongside the rawPublicKey
that will be added into the WebAuthValidator
contract. The hashed version is used in an effort to obfuscate the data with a commit-and-reveal pattern. At this point, the 24-hour waiting period begins, during which the account being recovered can cancel the recovery process.
Once the recovery window is reached (24 to 72 hours after initiating recovery), anyone can create a Transaction
that calls the addValidationKey
method on the WebAuthValidator
contract. If different transaction data is provided, the execution will revert at the Bootloader
layer. Parameters used for this call (credentialId
, originDomain
, and rawPublicKey
) must exactly match those provided when initiating the recovery. If these conditions are met, the GuardianRecoveryValidator
contract will successfully validate the transaction, allowing the execution to proceed on behalf of the SsoAccount
contract.
When a recovery request is used, it is automatically deleted, preventing it from being executed again. However, expired yet unused recovery requests are not automatically removed. Since executing an expired recovery request is not possible, it must be overridden with a new one, which also requires waiting for a 24-hour cancellation period. The SsoAccount
contract attached to this validator does have the option to modify and remove guardians at will, but only through a validatable Transaction
.
Security Model and Trust Assumptions
The owners of the SsoAccount
contract trust the good will of the guardians, in that they will choose to initiate the recovery process. Failing to do so can seriously affect the capability to recover access to such an account, even if there are honest guardians registered for that account.
Furthermore, the process to recover the accounts relies on the complementary use of another validator, the WebAuthValidator
contract, which must be functional and attached to the account. Otherwise, the guardians and their validator will not be able to successfully complete their endeavor.
Privileged Roles
The guardians assigned to an account can:
- Accept their role once the account has proposed them as guardians.
- Initiate/update a recovery process for a particular account that they are the guardians for.
After a recovery has been initiated, anyone can make the Transaction
using the GuardianRecoveryValidator
as the validator for it, and crafting the parameters to include the passed publicKey
to the WebAuthValidator
contract.
Design Choices, Limitations, and Integration Issues
- Even though the
SsoAccount
contract mimics theBootloader
flow of theDefault
account, it does not mimic the EOA behavior provided by theDefault
account. This means it could cause a reversion when calling functions protected by theonlyBootloader
modifier or when invoking unimplemented functionalities, instead of returning empty data. Protocols that previously used theDefaultAccount
contract might encounter issues when switching to the newSsoAccount
contract if these behaviors are not properly handled. - Both
originDomain
s andcredentialId
s make use of the hashed versions when submitting the recovery request to obfuscate their values. - The uninstallation process can be called with an empty
data
input which will not remove/unlink any guardian-related data from the contract. Furthermore, there is no restriction for sending partialdata
(this means, not the full list ofhashedOriginDomain
). - The procedure expects the
WebAuthValidator
contract to be attached to the respectiveSsoAccount
contract, but there is no enforcement nor validation that this is being done.
Medium Severity
Potential Denial-of-Service While Iterating Hooks
The runExecutionHooks
modifier and the runValidationHooks
function both iterate over all elements in a set, potentially leading to excessive gas consumption as the set grows. The runExecutionHooks
modifier contains a call to EnumerableSet.values()
, which retrieves all elements at once, an operation with an unbounded gas cost. Similarly, runValidationHooks
directly iterates over all validation hooks, making transaction validation increasingly expensive and potentially unusable if the number of hooks becomes too large.
Using EnumerableSet.values()
within state-changing functions and iterating over large sets during validation is risky because retrieving or processing all elements in one go can exceed the gas limits of a single block. If the set grows beyond a certain size, these operations may render transaction execution or validation infeasible.
Consider explicitly documenting that owners should limit the number of execution and validation hooks added to the set or imposing a reasonable upper bound on the number of hooks allowed.
Update: Partially resolved in pull request #372 at commit dff9e12. The Matter Labs team stated:
This is a known issue with the design of global hooks. Adding a small note to the hook interface is currently the best solution without overhauling how hooks work.
Front-Running Scenarios During Key Registration
The WebAuthValidator
contract utilizes two mappings for key registration: publicKeys
for associating keys with the accountAddress
, and registeredAddress
for linking keys with a combination of originDomain
and credentialId
. This setup is designed to ensure the uniqueness of each originDomain
and credentialId
pair within the system. However, the implementation exposes the problem of front-running attacks that could be exploited by malicious actors.
The problem arises from the contract's check to prevent the registration of duplicate originDomain
and credentialId
combinations. Malicious users can monitor the mempool for pending transactions aimed at registering a new key. Upon detecting such a transaction, they can execute a call to register a key with the same originDomain
and credentialId
, but with a different, arbitrarily chosen publicKey
. This preempts the legitimate registration attempt by occupying the intended slot.
The result culminates when the legitimate user's transaction reaches the onInstall
function, which includes a verification step to ensure that the key being added has not been registered under a different account. If an attacker has already claimed the slot, this verification fails, causing the legitimate transaction to revert and preventing the user from attaching their validator to their account.
Furthermore, the GuardianRecoveryValidator
contract utilizes a commit-reveal pattern over the originDomain
and credentialId
to obfuscate their actual values before they are needed. However, a malicious user could index the past values for all the SsoAccount
contracts attached to the WebAuthValidator
contract, hash their values, and might be able to match (in some cases) the obfuscated values to their actual unhashed values. This would allow them to preemptively start planning an attack.
To mitigate this front-running scenario, consider introducing additional verification mechanisms that securely associate the registration attempt with the initiating account, thus preventing unauthorized preemption of key registration slots. In addition, to improve the commit-reveal pattern, consider utilizing data from the account and/or guardian during the hashing and submitting the commitment in the WebAuthValidator
contract instead of in the GuardianRecoveryValidator
contract.
Update: Acknowledged, not resolved. The Matter Labs team stated:
Given that no elastic chain operators have public mem-pools or shared sequencers this is at-best a possible future issue. The two design issues at play here are trying to make this information accessible and separating recovery concerns from authentication concerns. In the future we might consider updating the check to support a list of accounts per credential id, which would significantly complicate the UX but would prevent this kind of front-running.
Guardian Can Overwrite Recovery Process and Render It Useless
The initRecovery
function of the GuardianRecoveryValidator
contract lacks a crucial validation step, allowing for the overwriting of existing recovery data without any checks for ongoing recovery processes. This oversight permits a guardian to initiate a new recovery using incorrect data or to refresh the timestamp, thereby obstructing or postponing the intended recovery process.
This vulnerability is particularly problematic in scenarios where an account is protected by multiple guardians. In such cases, a single guardian acting maliciously can indefinitely disrupt the recovery process by:
- Overwriting existing recovery data with malicious or incorrect information.
- Forcing well-intentioned guardians to overwrite this malicious data with the correct information, only for the cycle to repeat ad infinitum.
This cycle not only stalls the recovery process but also leaves no recourse for removing the malicious guardian and regaining control of the account, as the necessary Transaction
cannot be executed by any party.
To address these vulnerabilities, consider implementing a consensus mechanism among the guardians for recovery processes involving multiple guardians. Such a mechanism would only allow a recovery to proceed once a majority of guardians have concurred on the submitted parameters. Additionally, for accounts with a single guardian, consider enhancing the initRecovery
function by incorporating a preliminary check to ascertain when a recovery process is still active. These measures will significantly mitigate the risk of overwrites and delays, thereby bolstering the security and efficacy of the recovery process.
Update: Partially resolved in pull request #379 at commit f2b6bbf. The Matter Labs team stated:
We decided not to include an N-out-of-M design in this iteration; instead, we added a recommendation to the user-facing documentation to use a Multisig as a guardian to replicate this behavior.
Failure to Clear pendingRecoveryData
in onUninstall
Allows Immediate Account Recovery Upon Reconnection
The onUninstall
function can clear all data associated with guardians of a specific account, but it cannot clear the pendingRecoveryData
mapping's requests. This omission allows an account to re-connect the GuardianRecoveryValidator
contract and execute pending recovery requests within the permitted time window. This behavior contradicts the intended logic, whereby a newly reconnected account should not be able to perform actions such as calling the addValidationKey
function from the WebAuthValidator
contract without waiting for the required delay, as the state is expected to be cleared.
Consider explicitly clearing the pendingRecoveryData
mapping's requests during the onUninstall
function to prevent unauthorized immediate account recovery based on the old state of the pendingRecoveryData
mapping.
Update: Resolved in pull request #342 at commit 69f4c36.
Incomplete Recovery Process Due to Missing Validator Attachment
The SsoAccount
contract allows for the configuration of a guardian through the GuardianRecoveryValidator
contract. This setup enables the guardian to initiate a recovery process aimed at regaining control of the account by adding a new public key via the WebAuthValidator
contract. However, in the current implementation, there is an absence of a requirement for the SsoAccount
contract to be pre-attached to the WebAuthValidator
contract. Consequently, while a guardian may be correctly set up and linked to the GuardianRecoveryValidator
contract, attempting to validate a subsequent transaction through the WebAuthValidator
contract will fail if the SsoAccount
contract does not recognize the WebAuthValidator
as a module validator within the system, as indicated by this check.
This oversight renders the recovery process ineffective, as the successful addition of a public key does not guarantee the ability to validate future transactions through the WebAuthValidator
contract. The implication is that both the GuardianRecoveryValidator
and WebAuthValidator
contracts must be correctly set up in advance for the recovery flow to function as intended.
Furthermore, the SsoAccount
contract treats all validators as independent modules without providing a mechanism to require their collaborative operation. To address this issue and guarantee the successful completion of the recovery process, consider implementing checks during the attachment of the SsoAccount
contract to the GuardianRecoveryValidator
contract. These checks should ensure that the SsoAccount
contract is also adequately attached to the WebAuthValidator
contract, thereby securing the recovery flow's functionality. For a more comprehensive solution, consider introducing a validation hook that would enable validators to be designated as collaborative entities, rejecting transactions that attempt to disrupt this collaboration (e.g., by only adding the GuardianRecoveryValidator
contract or by removing the WebAuthValidator
contract after both have been attached). In addition, this hook could incorporate a whitelist of approved module validators, enhancing security for any account utilizing them.
Update: Partially resolved in pull request #343 at commit 9368f6b. The account attempting to connect to GuardianRecoveryValidator
now needs to be linked to webAuthValidator
. While this addresses the initial concern of the account not being connected to webAuthValidator
, it does not fully resolve the issue. In the future, the user could uninstall webAuthValidator
after connecting to GuardianRecoveryValidator
, which would still leave the account unrecoverable in that scenario. The Matter Labs team stated:
We will implement a check in
GuardianRecoveryValidator
'sonInstall
to requireWebAuthValidator
to be installed in theSSOAccount
. A more comprehensive solution, as suggested by the audit, would include extendingValidatorManager
functionality to allow grouping validators.
Uninstall Process Might Revert Due to Pending Guardian Acceptance
In the current implementation, if an account passes the entire array of hashedOriginDomain
s that it has used (for both accepted and pending guardians), the account might fail to uninstall the GuardianRecoveryValidator
contract from an SsoAccount
contract using the removeModuleValidator
function. This happens because the onUninstall
function does not verify whether the guardian has accepted the guardian role (i.e., the isReady
flag is set to true
) before attempting to remove the account from the guardian's set of guarded accounts. Consequently, when a guardian has a pending request and has not confirmed its acceptance, the user's account is not included in that guardian's set of guarded accounts. Attempting a removal by passing all the hashedOriginDomain
values, including those for pending guardians, triggers the remove
function to return false
, causing a call revert with the AccountNotGuardedByAddress
custom error.
As a result, currently, users cannot fully uninstall the validator while any guardian requests remain pending, as the data associated with these pending guardians cannot be entirely cleared. Although users may attempt to exclude pending guardians from the uninstallation process by omitting the relevant hashedOriginDomain
entries, this approach introduces risks. Specifically, pending guardians may subsequently accept their roles through the addValidationKey
function without the account owner's knowledge. Since the user has disabled the validator module and may not actively monitor such events, newly accepted guardians could initiate account recovery processes undetected.
If the user later reinstalls the validator module, they might unexpectedly discover new guardians already in place, potentially in the midst of adding recovery keys to WebAuthValidator
contract. Such a scenario could escalate to an SsoAccount
contract takeover, enabling the execution of unauthorized transactions due to the user's inability to reject the addition of new public keys to their account.
Consider verifying that a guardian has confirmed their role by checking the isReady
flag within the accountGuardianData
structure before removing the account from the guardian's guardedAccounts
mapping, similar to the verification implemented in the removeValidationKey
function.
Update: Resolved in pull request #345 at commit c36e31a.
Low Severity
Possible Initialization of an Unusable SsoAccount
In the initialize
function of the SsoAccount
contract, no validation is performed to ensure that the array arguments are non-empty. This omission allows the contract to be initialized in a state where it cannot perform any operations, rendering the account unusable.
Consider adding a validation check to ensure that at least one of the initialValidators
or initialK1Owners
arrays has a non-zero length before initialization. This will prevent the creation of non-functional accounts.
Update: Resolved in pull request #371 at commit 8454fce.
Potential Panic During Data Slicing in _executeCall
In the _executeCall
function of the SsoAccount
contract, there is a slice operation performed on the _data
parameter when handling a DEPLOYER_SYSTEM_CONTRACT
call. The purpose of this slice operation is to retrieve the function selector, which requires at least 4 bytes of data. However, the current implementation does not validate whether the input data has sufficient length. If the provided data is shorter than 4 bytes, the slicing operation will cause a panic, abruptly terminating the execution.
Consider verifying the length of the _data
parameter prior to slicing, and explicitly reverting with a meaningful error message if the length is insufficient.
Update: Resolved in pull request #369 at commit f476fb1.
Builtin Getter For pendingRecoveryData
Does Not Return rawPublicKey
The pendingRecoveryData
variable is declared with a public
visibility modifier, causing the Solidity compiler to automatically generate a public
getter function. However, this getter will only return the struct’s simple members (hashedCredentialId
and timestamp
), and it will omit complex data structures such as rawPublicKey
. This happens because automatically-generated getters for public
state variables do not return array or mapping members within structs, even when nested. For more information, see the Solidity documentation.
Since the contract already implements the getPendingRecoveryData
getter, consider removing the automatic getter (that does not retrieve all the data) by reducing the variable's visibility.
Update: Resolved in pull request #346 at commit 832372a.
Misleading Error Message in addValidationKey
The addValidationKey
function of the WebAuthValidator
contract is designed to add a new validation key. If the function returns false
, then the onInstall
function will emit the WEBAUTHN_KEY_EXISTS
error, indicating that the key already exists. However, this error message can be misleading due to other conditions under which the function may return false
, such as receiving an empty input for the rawPublicKey
. This lack of specificity in error reporting could lead to confusion, as users may incorrectly infer that a key exists when, in fact, the function failed due to different validation issues.
However, this error message can be misleading due to other conditions under which the function may return false
, such as receiving an empty input for the rawPublicKey
. This lack of specificity in error reporting could lead to confusion, as users may incorrectly infer that a key exists when, in fact, the function failed due to different validation issues.
To enhance clarity and improve error handling, consider differentiating the error messages based on the failure condition. This could involve introducing a new error code for cases where the input validation fails and reserving the WEBAUTHN_KEY_EXISTS
error strictly for scenarios where an attempt is made to add a duplicated key. By doing so, users and developers can more accurately diagnose issues and understand the contract's behavior, leading to a more robust and user-friendly experience.
Update: Resolved in pull request #333 at commit 5f89af4.
Deviation From Specifications
Throughout the codebase, multiple instances of deviations from specifications were identified:
- In line 83 of
GuardianRecoveryValidator.sol
, there is a comment regarding the functionality that purports to remove all past guardians when disabling the validator. However, if allhashedOriginDomains
are not passed as inputs to theonUninstall
function, there might still be leftovers associated with the account. Moreover, the contract lacks a mechanism to verify if all guardians associated with an account, such as a counter, have indeed been removed. Consider adding a counter that would aim to remove all the linkage between the account and the guardians. - In the ZkSync docs, there is a set of limitations that must be followed during transaction validation. These are: accounts can only interact with their own slots, context variables (e.g.,
block.number
) are not allowed in account logic, and accounts must increment the nonce by 1 to prevent hash collisions. However, in the_validateTransaction
function of theSsoAccount
contract, there is a call to therunValidationHooks
function, which uses the_call
function. Since_call
leverages thecall
opcode, it could modify state or violate the first two restrictions. In such cases, consider using thestaticcall
call for validation hooks to ensure compliance with these constraints.
To improve the clarity and maintainability of the codebase, consider resolving the aforementioned instances of deviations from specifications.
Update: Resolved in pull request #347 at commit 10efdf2.
Missing Checks for Function Arguments
When operations with address
parameters are performed, it is crucial to ensure that the address is not set to zero. Setting an address to zero is problematic because it has special burn/renounce semantics. Thus, this action should be handled by a separate function to prevent accidental loss of access during value or ownership transfers.
Within GuardianRecoveryValidator
, multiple instances of missing zero address checks were identified:
- The
_webAuthValidator
operation - The
newGuardian
operation - The
accountToGuard
operation - The
accountToRecover
operation
Likewise, in the addValidationKey
function of WebAuthValidator
, there is no validation for credentialId
and originDomain
, allowing them to be empty or have arbitrary lengths. Furthermore, the constructor of AAFactory
currently lacks validation checks for its input parameters. This absence of validation may cause unexpected contract reverts during operation. For instance, if _beaconProxyBytecodeHash
is set to an empty value, the ContractDeployer
will consistently revert, rendering the factory unusable and requiring redeployment.
Consider adding appropriate validation checks for the arguments before assigning them to a state variable.
Update: Resolved in pull request #349 at commit b65d4cf and pull request #333 at commit c9c8e79.
Floating Pragma
Pragma directives should be fixed to clearly identify the Solidity version with which the contracts will be compiled.
Throughout the codebase, multiple instances of floating pragma directives were identified:
GuardianRecoveryValidator.sol
has thesolidity ^0.8.24
floating pragma directive.IGuardianRecoveryValidator.sol
has thesolidity ^0.8.24
floating pragma directive.TransparentProxy.sol
has thesolidity ^0.8.24
floating pragma directive.WebAuthValidator.sol
has thesolidity ^0.8.24
floating pragma directive.
Consider using fixed pragma directives.
Update: Acknowledged, not resolved. The Matter Labs team stated:
We use floating pragmas to allow easy use of these base contracts with future compiler versions and other future packages that rely on these contracts. These future versions can provide gas optimizations or security enhancements. Updating just these 4 files would be inconsistent with the rest of the project and other published zksync system contracts.
Lack of Public Key Validation
In the addValidationKey
function, the public key is currently only checked to ensure that it is non-zero. However, this does not guarantee that the key is a valid point on the elliptic curve or that it can later be used as it should. The absence of explicit validation makes it possible to add an invalid public key, which may result in a situation where it is impossible to generate a valid signature. This could impact authentication mechanisms and may compromise system integrity.
Consider implementing a verification step to ensure that the provided public key is a valid point on the elliptic curve before adding it.
Update: Acknowledged, will resolve. The Matter Labs team stated:
There is also no validation that the provided k1 owner keys are valid keys, or that session k1 keys are valid. While we would accept this as a possible improvement, the additional gas required to truly validate this when adding keys would be non-trivial and would significantly change the interface of adding keys to this module.
The idea would be to use the
webauth.create
flow that is already validated in the client and replicate that to validate in the same manner as we do for thewebauthn.get
flow withwebAuthVerify
.A follow-up issue was created here: https://github.com/matter-labs/zksync-sso-clave-contracts/issues/340.
Missing or Incomplete Documentation
Docstrings provide essential context for contracts, functions, events, and state variables within smart contracts. They clearly explain intended behavior, usage, and relevant parameters, greatly enhancing readability, maintainability, and ease of security audits.
Multiple instances of incomplete or missing docstrings were identified in GuardianRecoveryValidator.sol
, IGuardianRecoveryValidator.sol
, and WebAuthValidator.sol
. For example, in the GuardianRecoveryValidator.sol
file, functions such as onInstall
and onUninstall
lack proper documentation for their parameters. This makes it challenging for developers or auditors to clearly understand the functionality and implications of these components.
Consider thoroughly documenting all contracts, functions, events, and relevant state variables according to the Ethereum Natural Specification Format (NatSpec). This should include clear, concise explanations for all parameters and return values associated with public APIs to ensure clarity and ease of review.
Update: Resolved in pull request #380 at commit fd8fdc9.
Notes & Additional Information
Non-Standardized Storage Location for SSO_STORAGE_SLOT
The SsoAccount
contract currently sets the storage offset for SSO_STORAGE_SLOT
using the keccak256('zksync-sso.contracts.SsoStorage') - 1
formula. Although functional, this approach deviates from the standardized method proposed by EIP-7201, specifically created to minimize the risk of storage collisions with default Solidity storage slots. Adopting this standard can also facilitate optimization opportunities in future protocol upgrades, such as those involving the Verkle state tree migration (if applicable to ZKsync).
Consider aligning the storage offset calculation for SSO_STORAGE_SLOT
with the standardized approach outlined in EIP-7201 to ensure safer storage management.
Update: Resolved in pull request #369 at commit 45360b4.
Unused Code
In the GuardianRecoveryValidator
contract, several custom errors are declared but never used:
PasskeyNotMatched
CooldownPeriodNotPassed
ExpiredRequest
Additionally, in the SsoAccount
contract, the signature
variable is defined but never utilized. Furthermore, the GuardianRecoveryValidator.sol
file includes imports for BatchCaller
and Call
, which are not used.
Consider removing the aforementioned unused errors, the signature
variable, and unnecessary imports to improve code clarity, maintainability, and readability.
Update: Resolved in pull request #350 at commits 84162ef and 66c2731, and pull request #377 at commit 37fe8d7.
Redundant Storage Access
The validateTransaction
function of the SsoAccount
contract calls _transaction.totalRequiredBalance()
twice: once to check if the account has sufficient balance and again when throwing an error if the balance is insufficient. This redundant invocation adds unnecessary computational overhead.
Since _transaction.totalRequiredBalance()
returns the same result within this scope, repeatedly calling it is inefficient. Instead, storing the value in a local variable and reusing it would improve performance and readability. A similar inefficiency exists in the WebAuthValidator
contract, where an unused code fragment is present here.
Consider caching the result of _transaction.totalRequiredBalance()
in a local variable and similarly caching the registeredAddress
in the WebAuthValidator
contract.
Update: Resolved in pull request #369 at commit b12ebf9.
Redundant Getter for publicKeys
in WebAuthValidator
In Solidity, when a state variable is declared as public
, the compiler automatically generates a getter function for it. This getter provides access to the variable's data but may have limitations depending on the variable type. In the case of arrays or mappings, Solidity-generated getters can only return one element at a time, which can lead to there being redundant custom getter functions in the contract.
The publicKeys
mapping variable is declared with public
visibility, prompting the compiler to generate a default getter for the bytes32[2] publicKey
array. However, this automatically generated getter returns only one element at a time, which is insufficient for retrieving the full key. As a result, the contract also defines a separate getAccountKey
function that returns both elements of the array. This results in redundant functionality, as one of these access methods is unnecessary.
Consider changing the visibility of the publicKeys
mapping to a more restrictive one, such as internal
or private
, to prevent the compiler from generating an automatic getter. This will eliminate the redundancy and ensure that only the explicitly defined getAccountKey
function is used for retrieving keys.
Update: Resolved in pull request #333 at commit 78b8bb0. The Matter Labs team stated:
We added comments and re-ordered the layout.
Inconsistencies Between Interface and Implementation
The IGuardianRecoveryValidator
interface and its implementation present inconsistencies in parameter naming, function ordering, and missing functions, which could hinder usability and developer experience. In particular:
- The
proposeValidationKey
andremoveValidationKey
functions have mismatched parameter names (externalAccount
instead ofnewGuardian
, andexternalAccount
instead ofguardianToRemove
, respectively). - The order of functions in the interface does not align with their implementation, such as with the
initRecovery
function. - The interface lacks critical functions found in the implementation, including the
discardRecovery
,guardiansFor
,guardianOf
, andgetPendingRecoveryData
functions.
Addressing these points will streamline the IGuardianRecoveryValidator
interface, enhancing clarity and facilitating a better development experience. Consider fixing the aforementioned instances.
Update: Resolved in pull request #361 at commit 1be0586.
IGuardianRecoveryValidator
Is Not Optimized For Development
In the IGuardianRecoveryValidator
interface, the current implementation could be optimized for better developer experience and protocol interoperability. Specifically, the events, errors, and structs defined within the implementation could be moved to the interface level. This adjustment would not only streamline the contract's structure but also enhance its usability in diverse development scenarios.
Moving these definitions to the interface would facilitate other contracts or developers who aim to interact with, decode, or handle these specific events and errors more efficiently. It would also simplify the process for those looking to pass structs as inputs or outputs in their implementations, such as for the guardiansFor
function. Given that IGuardianRecoveryValidator
is the primary interface interacting with these constructs, integrating them directly into the interface could significantly improve clarity and reduce the complexity for developers interfacing with the protocol.
Consider restructuring the interface to include these definitions directly. This approach not only adheres to best practices in contract design but also significantly enhances the developer experience by providing a more intuitive and accessible interface for interacting with the contract's functionalities.
Update: Resolved in pull request #361 at commit 1be0586.
Refactor Opportunity Over Address Casting
The GuardianRecoveryValidator
contract employs a direct type casting approach to convert uint256
to an address.
As the safeCastToAddress
method is already present in the Utils
library, consider using the safeCastToAddress
method in the validateTransaction
function to reduce the maintenance effort.
Update: Resolved in pull request #351 at 934d446 and e735710 commits.
Inconsistent Types for Time-Related Variables
In the GuardianRecoveryValidator
contract, there is an inconsistency in the types of variables used for time measurements, particularly for the addedAt
variable in the Guardian
struct and the timestamp
variable in the RecoveryRequest
struct. This variation in variable types for similar time-related data points could lead to confusion and conflicts within, or outside, the contract's operations.
To improve the readability and consistency of the code, consider keeping the same variable types for similar units. Alternatively, if this resulted from a gas optimization, consider documenting the design choice over the respective struct
.
Update: Resolved in pull request #357 at commit 70b0601.
Misleading Function Naming in GuardianRecoveryValidator
The GuardianRecoveryValidator
contract uses function names such as proposeValidationKey
, removeValidationKey
, and addValidationKey
that suggest a similarity to functions in other validators, such as those found in the WebAuthValidator
. However, the functionality of the functions in GuardianRecoveryValidator
is significantly different, as they are designed to manage guardians for an account rather than validation keys.
This discrepancy can lead to confusion and errors, as the naming convention does not accurately reflect the functions' purposes. Furthermore, the interface definitions IModule
and IModuleValidator
do not mandate such naming conventions, increasing the risk of misuse or misunderstanding. In addition, the parameters for these functions do not align with those of their namesakes in other validators, resulting in different function selectors. This further compounds the potential for confusion and errors among developers and auditors.
To mitigate these issues and enhance clarity, it is recommended to rename these functions to more accurately describe their specific roles in managing guardians rather than validation keys. Adopting clear and descriptive naming conventions will improve code readability, reduce the risk of errors, and facilitate a better understanding of the contract's functionality.
Update: Resolved in pull request #353 at commit ca53654.
Use 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.
Multiple instances of revert
and/or require
messages were identified within GuardianRecoveryValidator.sol
:
-
The
require(transaction.data.length >= 4, "Only function calls are supported")
statement -
The
require(transaction.to <= type(uint160).max, "Overflow")
statement
For conciseness and gas savings, consider replacing require
and revert
messages with custom errors.
Update: Resolved in pull request #354 at commit c09d2ff.
Missing Named Parameters in Mappings
Since Solidity 0.8.18, developers can utilize named parameters in mappings. This means mappings can take the form of mapping(KeyType KeyName? => ValueType ValueName?)
. This updated syntax provides a more transparent representation of a mapping's purpose.
Within GuardianRecoveryValidator.sol
, multiple instances of mappings with missing named parameters were identified:
- The
accountGuardians
state variable - The
guardedAccounts
state variable - The
pendingRecoveryData
state variable - The
accountGuardianData
state variable
If the last named parameter omission is a design choice, consider keeping a consistent coding style throughout the codebase, as there are cases such as the publicKey
parameter in the publicKeys
mapping from the WebAuthValidator
contract that do have the name. Otherwise, consider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #358 at commit a1bbeba.
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 is quite 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, multiple instances of contracts that do not have a security contact were identified:
Consider adding a NatSpec comment containing a security contact above each contract definition. Using the @custom:security-contact
convention is recommended as it has been adopted by the OpenZeppelin Wizard and the ethereum-lists.
Update: Resolved in pull request #355 at commit ec565fc.
Prefix Increment Operator Can Save Gas in Loops
Throughout the codebase, multiple opportunities for loop iteration optimization were identified:
-
The j++ in
GuardianRecoveryValidator.sol
-
The i++ in
GuardianRecoveryValidator.sol
-
The i++ in
GuardianRecoveryValidator.sol
-
The i++ in
WebAuthValidator.sol
Consider using the prefix increment operator (++i
) instead of the postfix increment operator (i++
) in order to save gas. This optimization skips storing the value before the incremental operation, as the return value of the expression is ignored.
Update: Resolved in pull request #356 at commits 9125cd9 and d6e705b, and pull request #378 at commit 66c1c37.
Inconsistent Order Within Contracts
Throughout the codebase, multiple instances of contracts having an inconsistent ordering of functions were identified:
-
In the
GuardianRecoveryValidator
contract inGuardianRecoveryValidator.sol
, theonlyGuardianOf
modifier is implemented between functions and the functions are not sorted by its visibility. -
In the
WebAuthValidator
contract inWebAuthValidator.sol
, thePasskeyId
struct comes after the functions and variables, and the function ordering is not correct.
To improve the project's overall legibility, consider standardizing ordering throughout the codebase as recommended by the Solidity Style Guide (Order of Functions).
Update: Resolved in pull request #362 at commit ae86f43, pull request #333 at commit 640d7c9.
Function Visibility Overly Permissive
Throughout the codebase, multiple instances of functions with unnecessarily permissive visibility were identified:
-
The
initialize
function inGuardianRecoveryValidator.sol
withpublic
visibility could be limited toexternal
. -
The
finishRecovery
function inGuardianRecoveryValidator.sol
withinternal
visibility could be limited toprivate
. -
The
_discardRecovery
function inGuardianRecoveryValidator.sol
withinternal
visibility could be limited toprivate
. -
The
guardiansFor
function inGuardianRecoveryValidator.sol
withpublic
visibility could be limited toexternal
. -
The
guardianOf
function inGuardianRecoveryValidator.sol
withpublic
visibility could be limited toexternal
. -
The
getPendingRecoveryData
function inGuardianRecoveryValidator.sol
withpublic
visibility could be limited toexternal
.
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 in pull request #359 at commit 924d857.
Use calldata
Instead of memory
When dealing with the parameters of external
functions, it is more gas-efficient to read their arguments directly from calldata
instead of storing them to memory
. calldata
is a read-only region of memory that contains the arguments of incoming external
function calls. This makes using calldata
as the data location for such parameters cheaper and more efficient compared to memory
. Thus, using calldata
in such situations will generally save gas and improve the performance of a smart contract.
Within GuardianRecoveryValidator.sol
, multiple instances where function parameters should use calldata
instead of memory
were identified:
- The
rawPublicKey
parameter - The dismissed parameter
Consider using calldata
as the data location for the parameters of external
functions to optimize gas usage.
Update: Resolved in pull request #360 at commit 79d3725.
Suboptimal ERC-165
Interface Check Implementation
The _supportsModuleValidator
function of the ValidatorManager
contract currently checks whether a given validator
account implements the IModuleValidator
and IModule
interfaces separately. Each interface check uses three external
calls via the supportsERC165InterfaceUnchecked
method, resulting in a total of six external
calls. However, the ERC165Checker
library provides a more efficient alternative, the supportsAllInterfaces
function, capable of verifying both interfaces simultaneously with only four external
calls. Utilizing this method will significantly reduce gas consumption during each initialization of SsoAccount
contract. The same applies to the _supportsHook
function in the HookManager
contract.
Consider replacing the separate interface checks with a single call to supportsAllInterfaces
, supplying both IModuleValidator
and IModule
selectors, to optimize gas usage within the initialize
function of the SsoAccount
contract.
Update: Resolved in pull request #369 at commit de51ab6.
Typographical Error
In the NatSpec comment of the initialize
function in the SsoAccount
contract, the abi.encode(validatorAddr,validationKey))
code snippet contains an extra closing parenthesis.
Consider correcting the aforementioned typographical error.
Update: Resolved in pull request #370 at commit aa313ab.
Redundant Hashing Operations in webAuthVerify
In the webAuthVerify
function of the WebAuthValidator
contract, comparisons are performed between certain string fields from clientDataJSON
and predefined constant string values. Currently, the function utilizes the Strings.equal
function which hashes both the provided and constant strings each time it compares them. Since these constant strings do not change, this implementation unnecessarily repeats hashing operations, increasing execution cost.
The function can be optimized by precomputing hashes for constant string values such as "webauthn.get"
and "false"
, storing these hashes directly in the contract. Thus, comparisons would only require hashing the input data once, avoiding redundant hashing operations.
Consider implementing this approach by storing precomputed hashes of constant values, thereby minimizing hashing operations and reducing the overall execution cost of the webAuthVerify
function.
Update: Resolved in pull request #333 at commit ce6f196.
Recommendations
Code Style in GuardianRecoveryValidator
Several areas within the GuardianRecoveryValidator
contract can be improved for consistency and maintainability:
- The
addValidationKey
function returns a boolean to indicate the call's outcome, whereas other functions in the contract do not follow this convention. Consider aligning the style of function signatures consistently across the contract. - The
onUninstall
function duplicates the functionality found in theremoveValidationKey
function with minor differences. Consider making theremoveValidationKey
function apublic
function and calling it from theonUninstall
function to avoid redundancy. Note that this approach will require small adjustments to the current logic within theremoveValidationKey
function. - The
onlyGuardianOf
modifier is used only once in theinitRecovery
function. Consider embedding this modifier's logic directly into theinitRecovery
function to simplify the contract's structure. - The
finishRecovery
function contains minimal logic and is called only once. Consider integrating its functionality directly into thevalidateTransaction
function to simplify code review and avoid unnecessary runtime bytecode overhead. - In the
validateTransaction
function, thestoredData
variable is currently accessed as a storage pointer, though its contents are only read and not modified. Since all struct elements are accessed, consider loading them into memory to optimize gas consumption. - In the
proposeValidationKey
function, thenewGuardian
is added to aGuardian
struct and stored within a mapping using the key ofnewGuardian
itself (accountGuardianData[hashedOriginDomain][msg.sender][newGuardian]
). This field may be redundant. Consider revising this storage logic to avoid tautological logic, when the value is a key for a mapping and also the struct member pointed to by the key.
Consider implementing the above recommendations to improve the clarity and maintainability of the codebase.
Conclusion
During the audit, the new guardian recovery validator contract and associated changes were reviewed. This new validator contract enables users to designate guardians for their SsoAccount
accounts, allowing a recovery process to take place if users lose control over them. Recovery functionality depends on integration with both the WebAuthValidator
and the GuardianRecoveryValidator
contracts, introducing a new way of social recovery to the ZKsync ecosystem.
The audit identified multiple medium-severity findings, along with several low- and note-severity issues. The GuardianRecoveryValidator
contract, being in its initial iteration, has room for improvement, such as having clearer code structure, reduction of duplicated code through component reuse, and optimizations to enhance readability and reduce execution costs. Additionally, providing more descriptive documentation, particularly for new functionality in the WebAuthValidator
contract, would enhance clarity and maintainability. Finally, a more comprehensive test suite could also improve the quality of the codebase, in particular by adding more negative case scenarios which could have found some of the issues presented in the report.
The Matter Labs team was responsive and cooperative throughout the audit process. We appreciate their professionalism and collaboration.