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.
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 the IModuleValidator interface definition, has been added.GuardianRecoveryValidator.sol: A new implementation of the guardian validator has been added.WebAuthValidator.sol: An adaptation of the original WebAuthValidator contract has been added to allow multipass keys, primarily due to the introduction of a credentialId into the structure of the storage.GuardianRecoveryValidatorThe 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:
SsoAccount contract to the WebAuthValidator and GuardianRecoveryValidator contracts by adding them as module validators.proposeValidationKey function of the GuardianRecoveryValidator contract to propose a guardian throughout the SsoAccount contract's transaction execution workflow.addValidationKey function of the GuardianRecoveryValidator 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.
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.
The guardians assigned to an account can:
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.
SsoAccount contract mimics the Bootloader flow of the Default account, it does not mimic the EOA behavior provided by the Default account. This means it could cause a reversion when calling functions protected by the onlyBootloader modifier or when invoking unimplemented functionalities, instead of returning empty data. Protocols that previously used the DefaultAccount contract might encounter issues when switching to the new SsoAccount contract if these behaviors are not properly handled.originDomains and credentialIds make use of the hashed versions when submitting the recovery request to obfuscate their values.data input which will not remove/unlink any guardian-related data from the contract. Furthermore, there is no restriction for sending partial data (this means, not the full list of hashedOriginDomain).WebAuthValidator contract to be attached to the respective SsoAccount contract, but there is no enforcement nor validation that this is being done.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.
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.
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:
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.
pendingRecoveryData in onUninstall Allows Immediate Account Recovery Upon ReconnectionThe 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.
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'sonInstallto requireWebAuthValidatorto be installed in theSSOAccount. A more comprehensive solution, as suggested by the audit, would include extendingValidatorManagerfunctionality to allow grouping validators.
In the current implementation, if an account passes the entire array of hashedOriginDomains 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.
SsoAccountIn 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.
_executeCallIn 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.
pendingRecoveryData Does Not Return rawPublicKeyThe 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.
addValidationKeyThe 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.
Throughout the codebase, multiple instances of deviations from specifications were identified:
GuardianRecoveryValidator.sol, there is a comment regarding the functionality that purports to remove all past guardians when disabling the validator. However, if all hashedOriginDomains are not passed as inputs to the onUninstall 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.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 the SsoAccount contract, there is a call to the runValidationHooks function, which uses the _call function. Since _call leverages the call opcode, it could modify state or violate the first two restrictions. In such cases, consider using the staticcall 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.
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:
_webAuthValidator operationnewGuardian operationaccountToGuard operationaccountToRecover operationLikewise, 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.
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 the solidity ^0.8.24 floating pragma directive.IGuardianRecoveryValidator.sol has the solidity ^0.8.24 floating pragma directive.TransparentProxy.sol has the solidity ^0.8.24 floating pragma directive.WebAuthValidator.sol has the solidity ^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.
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.createflow that is already validated in the client and replicate that to validate in the same manner as we do for thewebauthn.getflow withwebAuthVerify.A follow-up issue was created here: https://github.com/matter-labs/zksync-sso-clave-contracts/issues/340.
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.
SSO_STORAGE_SLOTThe 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.
In the GuardianRecoveryValidator contract, several custom errors are declared but never used:
PasskeyNotMatchedCooldownPeriodNotPassedExpiredRequestAdditionally, 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.
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.
publicKeys in WebAuthValidatorIn 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.
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:
proposeValidationKey and removeValidationKey functions have mismatched parameter names (externalAccount instead of newGuardian, and externalAccount instead of guardianToRemove, respectively).initRecovery function.discardRecovery, guardiansFor, guardianOf, and getPendingRecoveryData 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 DevelopmentIn 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.
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.
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.
GuardianRecoveryValidatorThe 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.
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.
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:
accountGuardians state variableguardedAccounts state variablependingRecoveryData state variableaccountGuardianData state variableIf 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.
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.
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.
Throughout the codebase, multiple instances of contracts having an inconsistent ordering of functions were identified:
In the GuardianRecoveryValidator contract in GuardianRecoveryValidator.sol, the onlyGuardianOf modifier is implemented between functions and the functions are not sorted by its visibility.
In the WebAuthValidator contract in WebAuthValidator.sol, the PasskeyId 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.
Throughout the codebase, multiple instances of functions with unnecessarily permissive visibility were identified:
The initialize function in GuardianRecoveryValidator.sol with public visibility could be limited to external.
The finishRecovery function in GuardianRecoveryValidator.sol with internal visibility could be limited to private.
The _discardRecovery function in GuardianRecoveryValidator.sol with internal visibility could be limited to private.
The guardiansFor function in GuardianRecoveryValidator.sol with public visibility could be limited to external.
The guardianOf function in GuardianRecoveryValidator.sol with public visibility could be limited to external.
The getPendingRecoveryData function in GuardianRecoveryValidator.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 in pull request #359 at commit 924d857.
calldata Instead of memoryWhen 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:
rawPublicKey parameterConsider 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.
ERC-165 Interface Check ImplementationThe _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.
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.
webAuthVerifyIn 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.
GuardianRecoveryValidatorSeveral areas within the GuardianRecoveryValidator contract can be improved for consistency and maintainability:
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.onUninstall function duplicates the functionality found in the removeValidationKey function with minor differences. Consider making the removeValidationKey function a public function and calling it from the onUninstall function to avoid redundancy. Note that this approach will require small adjustments to the current logic within the removeValidationKey function.onlyGuardianOf modifier is used only once in the initRecovery function. Consider embedding this modifier's logic directly into the initRecovery function to simplify the contract's structure.finishRecovery function contains minimal logic and is called only once. Consider integrating its functionality directly into the validateTransaction function to simplify code review and avoid unnecessary runtime bytecode overhead.validateTransaction function, the storedData 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.proposeValidationKey function, the newGuardian is added to a Guardian struct and stored within a mapping using the key of newGuardian 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.
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.