We audited the zksync-sso-clave-contracts repository at commit fc0af34.
In scope were the following files:
src
├── auth
│ ├── Auth.sol
│ ├── BootloaderAuth.sol
│ ├── HookAuth.sol
│ └── SelfAuth.sol
├── helpers
│ ├── TimestampAsserterLocator.sol
│ ├── TokenCallbackHandler.sol
│ └── VerifierCaller.sol
├── interfaces
│ ├── IHookManager.sol
│ ├── IHook.sol
│ ├── IModule.sol
│ ├── IModuleValidator.sol
│ ├── IOwnerManager.sol
│ ├── ISsoAccount.sol
│ ├── ITimestampAsserter.sol
│ └── IValidatorManager.sol
├── libraries
│ ├── Errors.sol
│ ├── SessionLib.sol
│ ├── SignatureDecoder.sol
│ └── SsoStorage.sol
├── managers
│ ├── HookManager.sol
│ ├── OwnerManager.sol
│ └── ValidatorManager.sol
├── validators
| ├── SessionKeyValidator.sol
| └── WebAuthValidator.sol
├── AAFactory.sol
├── AccountProxy.sol
├── SsoAccount.sol
├── SsoBeacon.sol
├── EfficientProxy.sol
├── TransparentProxy.sol
├── batch/BatchCaller.sol
└── handlers/ERC1271Handler.sol
The code under review introduces the SsoAccount contract, a more customizable smart account that plugs into the existing bootloader Account Abstraction (AA) framework. As such, it is invoked identically to the DefaultAccount, following these steps:
bootloader calls validateTransaction to decide if the transaction should be executed or not.bootloader calls prepareForPaymaster. Otherwise, the SsoAccount contract funds the transaction itself through payForTransaction.bootloader calls executeTransaction, which initiates an arbitrary call with the SsoAccount as msg.sender. The SsoAccount inherits the BatchCall functionality, creating the opportunity for complex execution flows with only one necessary validation.A transaction could be valid if paired with a 65-byte long ECDSA signature based on the secp256k1 curve, which, when recovered, corresponds to one of the accounts' k1Owners. In addition, the SsoAccount contract has the ability to attach other functionalities to itself. These functionalities can be:
validationHooks).executionHooks).Besides the SsoAccount contract, the code under review implements two contracts that can serve as custom transaction validators: SessionKeyValidator and WebAuthValidator.
SessionKeyValidatorThe SessionKeyValidator contract is a module that allows an SsoAccount contract to create granular execution sessions for custom signers. Each session has an expiration timestamp and specifies a signer of the session, the amount that can be send to the bootloader contract as transaction fees, the amount that can be send with transfers or function calls during the execution step, as well as function selector and parameter constraints for arbitrary calls. A transaction meant to be validated by the SessionKeyValidator contract should be accompanied by information that links it to a particular session, as well as a valid ECDSA signature that recovers to the session owner's address.
The current implementation of the validator does not require any validation hooks to process the transaction. Furthermore, the session information is not persistent in storage, meaning it must be passed along with the transaction. Only the session hash (together with usage tracking information) is persisted in storage. If the transaction intends to call functionality that was not whitelisted, the transaction will revert when looping over all the defined policies for the session. Most of the functionality is implemented in the SessionLib library, complementary to the aforementioned contract.
WebAuthValidatorThe WebAuthValidator contract is a module that allows an SsoAccount contract to validate a transaction through WebAuthn passkeys. The account can add public keys corresponding to multiple signing domains. A transaction is considered valid if the accompanying data can be parsed to a JSON that has the necessary WebAuthn verification fields, the challenge field matches the transaction's hash, and the transaction signature recovers to the public key of the specified signing domain.
SsoAccount DeploymentNew SsoAccount contracts can be deployed through the AAFactory contract.
It is important to note that each SsoAccount is upgradeable and loads its' implementation from the SsoBeacon contract, which is a more efficient BeaconProxy. Hence, the implementation of different accounts can not be upgraded independently, and upgrading one will impact all of them.
In turn, the SessionKeyValidator, WebAuthValidator and AAFactory contracts are also upgradeable and will sit behind TransparentProxy instances, which are more efficient TransparentUpgradeableProxies.
Several design limitations were identified throughout the audit. While not necessarily a security risk, they should be taken into consideration as they limit functionality or could hinder user experience:
SsoAccount contract can grant arbitrary signers access to granular execution sessions, all transactions will flow through the same account and, hence, will share the same nonce within ZKsync. As a result, race conditions can appear where two session owners submit a transaction and only one of them succeeds while the other one reverts.SsoAccount contract is not sponsored by a paymaster, it will supply the transaction fee by itself and be refunded any remaining gas at the end of execution. However, when tracking and enforcing transaction fee spending limits, the SessionKeyValidator contract does not take into account refunds and always decreases the spending allowance by the full transaction fee. Thus, it is expected that a session owner will be able to spend fewer funds on transaction fees than initially intended.SessionKeyValidator contract requires the presence and correct linking of a TimestampAsserter contract. Since the current implementation of the TimestampAsserterLocator contract hardcodes the addresses of the TimestampAsserter within ZKsync, the TimestampAsserterLocator will require changes when deploying to any other ZK chain.SessionKeyValidator contract's expired sessions to be automatically deleted. These expired sessions can accumulate unless regularly cleared by each SsoAccount contract.SessionKeyValidator reverts if there are still active sessions, while the WebAuthValidator has no such checks.k1Owners or validators. If all transaction validations methods of an account are removed, the account would be locked, without the possibility of validating and executing any transactions.ERC1271Handler isValidSignature function does not run the validation hooks. Hence, a signature that was considered invalid by the SsoAccount contract might pass validation through the ERC1271 flow, or vice-versa. Additionally, the SessionKeyValidator contract does not support this signature validation method, and hence all transactions that leverage it will be invalid within the ERC1271 flow.WebAuthValidator transaction validation flow, it is possible that the supplied clientDataJSON contains multiple occurrences of the same WebAuthn key. Due to implementation details within the JSONParserLib library, using the at function will return the last encountered occurrence of a key. Hence, if a clientDataJSON object with both a valid and an invalid challenge value is supplied, transaction validation will depend on which challenge value appears first.WebAuthValidator transaction validation flow, the same public key can correspond to multiple origin domains.SsoAccount contract, the onInstall function of the hook or validator is called. It is important to note that the initData passed as parameter is not enforced to be non-empty ([1], [2]). This detail should be considered when implementing a hook or a validator, and revert in case empty initialization data is not wanted.removeHook and unlinkHook functions remove a hook from the corresponding set and then call the onUninstall function. Any hooks that need to callback during the onUninstall function call and check that they are attached to the account might revert because the hook's address will have been removed and will no longer appear as attached.AAFactory contract, the initialize function will not require any validators nor k1Owners to pass, which would result in a locked account.The WebAuthValidator contract transaction validation flow uses Solady's JSONParserLib library. It is important to note that JSONParserLib library can revert, or not, the transaction if the JSON is malformed or does not contain fields that were requested. In several method, the output validation is delegated to the contract that uses the library. Such cases were not explicitly handled by the codebase under review, although no attack pattern was uncovered.
The SsoAccount contract can attach to other validation or execution hooks which it will change the dynamic of the transaction workflow. As the protocol does not limit nor defines how those should behave at a bare minimum, hooks can be improperly implemented. Available validators can give some notions for them but there are no hook contracts that can be used as a role model.
Continuing with the standardization, a few calls do not present a defined guideline, such as the onInstall or onUninstall functions. Sensitive functionalities tied to hook and validation management can possibly be called when they are not supposed to, potentially causing issues in the underlying hook and validation contracts that assume certain management workflow.
Throughout the audit, the following trust assumptions were made:
A new SsoAccount can be deployed by anyone, at which time the initial validators and k1Owners will be set.
Only the bootloader can call an SsoAccount's validateTransaction, payForTransaction, prepareForPaymaster, and executeTransaction functions.
Upon the correct validation of a transaction, the SsoAccount contract will execute the intended action, either on itself or on arbitrary targets. Only the account itself can trigger BatchCall functionality.
The account can add or remove hooks, k1Owners, as well as validators.
A hook attached to an account can perform actions before the transaction validation or before and/or after execution.
Within the SessionKeyValidator contract, an account can create and grant a session, as well as revoke any session previously granted. Due to the complexity of the SessionSpec structure, the creator of a session is trusted to supply valid session information, with reasonable transfer and function call limits, as well as constraint indices set to the right offsets of the targeted function parameters. Moreover, it is crucial that the session does not have multiple policies for the same target address or target + function selector combination, as this can cause unexpected behaviour when tracking spending limits. Moreover, sessions are not fully validated on-chain and only a few items are checked.
Within the WebAuthValidator contract, an account can store public keys for custom signing domains, enabling transaction validation through signature recovery to the correct public key.
The owners of the SessionKeyValidator, WebAuthValidator, and AAFactory contracts' TransparentProxy are allowed to change the underlying implementations. The owner of the SsoBeacon is able to change the implementation of the SsoAccount contract's proxies. They are trusted to act in the protocol's best interest and to not perform any malicious activities. Each account's owner is trusted to thoroughly verify each validation mechanism it attaches to its account. This includes k1Owner and/or custom validators and hooks, as these actors can enable the correct validation of malicious transactions, or on the contrary, have such strict transaction validation logic that the system becomes locked and devoid of the capacity to remove these actors.
Throughout the audit, several areas for improvement were identified that could further enhance the quality and security of the codebase:
SsoAccount contract works as intended when plugged into the bootloader and the encompassing ZKsync system.SsoAccount contract, developers should pay an extra attention to its design choices and limitations as well as ensure that a thorough testing is performed, validating the entire transaction flow.SsoAccountThroughout the codebase, the functionalities calling the add and the remove methods of the EnumerableSet library do not check the returned value and incorrectly assume that the code would revert in case of an error. This makes the following scenarios possible:
isValidation flag. The code will attempt to remove the execution hook from the incorrect list, fail silently in doing so, yet proceed with calling the onUninstall function in the hook's contract. Depending on the hook's implementation, it is possible that subsequent transaction validation or execution will revert, since from the hook's standpoint, it is no longer attached to the SsoAccount contract or has cleared the necessary state. This can lead to the indefinite lockup of the account. Consider validating the return value of the add or remove calls to prevent silent failures. In addition, the current implementation allows a hook to be both a validation and execution hook, and since both interfaces have the onInstall and onUninstall functions, adding or removing a hook from both lists could cause unexpected behavior. Thus, consider enforcing that a hook contract is used for either validation or execution.k1Owner addresses, it is possible that the caller attempts to add a duplicate k1Owner address or remove an address that did not have this role. While the _k1Owners set would remain unchanged, the K1AddOwner and K1RemoveOwner events would nonetheless be emitted, which could affect off-chain indexers that track them. Consider validating the return value of the add or remove calls, and revert when the calls fail.onInstall and onUninstall functions would be called, which would produce unexpected behavior depending on the implementation. Consider validating the return value of the add or remove calls and reverting when the calls fail.Update: Resolved in pull request #260 and pull request #311. The pull request also introduces the ability for an execution hook to return bytes32 context from the preExecutionHook call, and pass it to the postExecutionHook call.
The SessionKeyValidator contract provides an SsoAccount with the functionality to create sessions with custom transfer and call policies. The policies can enforce a limit of funds that the session owner is allowed to send through a transfer or a function call. The limits are specified either for the entire lifetime of the session or as a per-period allowance, allowing the session owner to only spend amount of funds every period seconds. When sending a transaction, a user will attach additional calldata to their call, which decodes to the specifications of the session they intend to use and the periodIds they want to spend funds from. Depending on the remaining allowances for the session, the transaction can fail validation.
It is important to note that the periodIds is not included in a transaction's hash since it is appended at the end of the transaction.signature field and this field is not used during the encoding process. Hence, it can be modified without the user's consent. If validation fails for a transaction where the session owner intended to only spend funds from a period with periodId 10, a malicious actor could observe the transaction in the mempool and resend it with different periodIds, potentially achieving execution and spending allowances from unintended periods. A similar rationale and attack pattern can be applied to any information held within transaction.signature which is not the signature itself but auxiliary data to be used through the validation process, such as the validator field.
Consider adding a mechanism to protect against different entities submitting the same transaction with different parameters intended for validation.
Update: Acknowledged, will resolve. The Matter Labs team stated:
periodIdsare not meant to be semantically interpreted the same way as e.g. uniswap's trade deadline. They merely serve as auxiliary data that enables calculatingblock.timestamp / periodduring validation. If it was possible to useblock.timestampdirectly, there would be no need in supplyingperiodIds, and hence there would be no such issue.periodIdsare not signed and thus should not be treated by the users/developers as guarantees that transaction will be executed strictly within provided periods. If the users/developers care that a failed transaction can be replayed with differentperiodIds, they should simply send another transaction to increase the nonce and prevent a potential replay. That being said, the point thatvalidatorandvalidatorDatain general is not signed is completely valid and might become an attack vector in future modules. Currently, there is no way to allow signing arbitrary auxiliary data within any of the allowed transaction types on ZKsync. We will make sure to clearly reflect this in the developer documentation to prevent potentialvalidatorDatamisuse in future modules.
Through the runExecutionHooks modifier, each execution hook attached to an SsoAccount contract will run logic both before and after transaction execution. If the transaction adds or removes an execution hook, the current implementation might not function accordingly:
totalHooks variable is not updated and the postExecutionHook for loop will run out of bounds. In practice, this results in an inability to remove execution hooks. Moreover, since the OpenZeppelin EnumerableSet library does not have ordering guarantees after updating the set, it is possible that the postExecutionHooks are run in a different order.postExecutionHook function is executed, while an older hook will be neglected.Consider reviewing the execution hooks flow and determining what is the expected behavior in case a hook is added or removed in between pre- and post-execution. If only the same hooks are expected to run, consider caching both the totalHooks variable and the list of hooks.
Update: Resolved in pull request #281.
error Keyword Used For Variable NamingIn the ERC1271Handler and the SsoAccount contracts, the error keyword is being used to name a variable that holds the error returned from a call to ECDSA tryRecover.
However, the error keyword is similar to Error, which is a reserved keyword used to to define custom errors in Solidity. This can lead to confusion and potential errors when maintaining the codebase. Consider renaming the variable to something different that still reflects its purpose.
Update: Resolved in pull request #292.
The SsoStorage library defines the storage structure of all the different variables used by the SsoAccount contract. The structure is stored at a pre-calculated slot to prevent collisions and reduce the attack surface. However, the slot used corresponds to the Clave protocol.
Consider changing the storage slot to a different one that is specific to the ZKsync SSO Account project. Moreover, consider documenting how that hash was calculated as the Clave protocol has done.
Update: Resolved in pull request #269 at commit 52bc7f9 and in pull request #306.
Throughout the codebase, multiple opportunities for gas optimization were identified:
signature parameter of the isValidSignature function from the ERC1271Handler contract and validateSignature function from the WebAuthValidator contract could be declared as calldata instead of memory. Consider using calldata when appropriate.SsoStorage library, the __gap_0, __gap_2, and __gap_4 variables follow a non-sequential nomenclature and are redundant. It is possible to combine them under a single __gap variable that is cheaper yet still protects against future upgrades. Consider using a single __gap variable instead or documenting the reason for such a decision.SessionKeyValidator contract, the status is marked as Closed, preventing the validation of a transaction or the recreation of the session. Simultaneously, the rest of the SessionStorage structure becomes unreachable through the existing implementation. Consider deleting the unnecessary state for the particular account to recover gas upon closing a session.webAuthVerify function of the WebAuthValidator contract, the rs[0] and rs[1] bytes32 variables cannot be smaller than 0. For the current implementation, consider requiring them to be zero instead of equal or less than zero.When performing these changes, aim to reach an optimal trade-off between gas optimization and readability. Having a codebase that is easy to understand reduces the chance of errors in the future and improves transparency for the community.
Update: Partially resolved in pull request #294. The Matter Labs team stated:
About the third point, to delete session state other than the status, we have to pass the
StorageSpecintorevokeKey, since there is no way to otherwise know the targets and selectors of the policies, the states of which we want to delete. Requiring to passStorageSpecintorevokeKeycan negatively impact other system components, which would make revoking sessions harder for the users. Hence we decided not to change our approach there.
The Auth contract inherits access control modifiers from the BootloaderAuth, SelfAuth, and HookAuth contracts. Auth is then inherited by the HookManager, ValidatorManager, and OwnerManager contracts. However, the HookAuth functionality is unused as the ValidatorManager contract only uses the onlySelf modifier and the SsoAccount contract only uses the onlyBootloader modifier. This means that each contract inheriting from Auth will only be using one particular feature from it, resulting in inheriting redundant code which increases maintenance effort and reduces readability.
In order to improve the maintainability and clarity of the codebase, consider deleting Auth and modifying each contract that requires authentication to only inherit the necessary authentication modifiers, as it is being done with the BatchCaller contract with the SelfAuth contract.
Update: Resolved in pull request #275.
It is not an uncommon practice to use abi.encodeWithSignature or abi.encodeWithSelector to generate calldata for a low-level call. However, the first option is not typo-safe and the second option is not type-safe. The result is that both of these methods are error-prone and should be considered unsafe.
Throughout the codebase, multiple instances of unsafe ABI encoding were identified:
abi.encodeWithSelector within the HookManager.solabi.encodeWithSelector within the SessionLib.solabi.encodeWithSelector within the ValidatorManager.solConsider replacing all the occurrences of unsafe ABI encoding with abi.encodeCall which checks whether the supplied values actually match the types expected by the called function and also avoids errors caused by typos.
Update: Resolved in pull request #269 at commit dd96b2e and commit 69c614a.
Throughout the codebase, multiple instances of missing or incomplete documentation were identified:
validateSignature function of the SessionKeyValidator contract does not detail why this particular validator should not be used for signature validation.beacon storage variable of the AAFactory contract is not documented, consider mentioning that it will point to an instance of the SsoBeacon contract.verifier address of the VerifierCaller contract, detailing where the precompile code can be found for analysis.WebAuthValidator contract. For example, details about why the validation checks against r and s are important, what is the meaning of the 33rd byte of authenticatorData, why the first and third least significant flag bits are special, and detailing why only the type, challenge, origin, and crossOrigin fields are important and why the rest are not relevant for the on-chain part of the implementation.WebAuthValidator contract, the at method states that "duplicated keys, the last item with the key WILL be returned". This means that if there are multiple challenges in the JSON, if the last one is successful, then the process will pass. However, if the successful one is before the last challenge, then the outcome will be different. Consider documenting this behavior to users or preventing the possibility of having a non-deterministic outcome due to repeated types.Consider reviewing the whole codebase for missing docstrings and missing documentation around each step of the implementation. In addition, consider documenting any hidden code assumptions or expected behaviors of system components.
Update: Partially resolved in pull request #285 and pull request #290. The Matter Labs team stated:
About the third point: "Verifier caller is verifier-agnostic, could be a precompile or a contract, so doesn't make sense to add it there"
JSON Parsing behavior: Duplicate keys in JSON is explicitly implementation-defined behavior in the JSON RFC (https://datatracker.ietf.org/doc/html/rfc8259#section-4). While the previous behavior of throwing for ambiguous JSON might improve strictness, in practice this JSON is generated from browsers without the developer's intervention so duplicate keys would be a violation on the client side's standard. See https://www.w3.org/TR/webauthn-2/#dictdef-collectedclientdata for the expected format from the client.
SsoAccount Can Call updateAccountVersion and updateNonceOrderingWhen the SsoAccount executes a call to the DEPLOYER_SYSTEM_CONTRACT through the batchCall functionality, the function selectors are not validated similar to how they are in the DefaultAccount contract or in the _executeCall function of the SsoAccount. This would allow accounts to call updateAccountVersion or updateNonceOrdering, which could lead to arbitrary nonce integration issues.
Consider adding the above-mentioned check to prevent any unexpected behavior.
Update: Resolved in pull request #293 and pull request #305.
Throughout the codebase, multiple instances of unused or redundant code were identified:
HookAuth contract is unused and can be entirely removed. The NOT_FROM_HOOK error should be removed after following the previous recommendation.Errors library is unused in Auth.sol, OwnerManager, and SignatureDecoder.sol.IERC777Recipient interface import in the ISsoAccount interface.Transaction structure import in the ERC1271Handler contract.To improve the clarity and quality of the codebase, consider reviewing the above instances and removing any unused or redundant code.
Update: Resolved in pull request #276.
When one of the batch call fails and allowFailure is set to true, the revert is not propagated and the execution continues.
However, no information is emitted about which calls reverted. Moreover, the returned data from failed calls is dismissed, and it might be important for debugging. Consider emitting an event that specifies the index of the reverted call and the returned data.
Update: Resolved in pull request #293 and pull request #313. It is important to note that the getReturnData function updates the free memory pointer to a location which is not aligned to a multiple of 32 bytes. While this does not currently pose a security risk, issues could arise if the code is ever modified in such a way where data is stored starting at the misaligned location, and then read from an aligned location, or vice versa.
SsoAccount Contract Deployment Can Be FrontrunAny user can deploy a new SsoAccount contract through the AAFactory contract. As inputs, the function takes a _salt and a _uniqueAccountId to use for a create2 call.
However, any other user could observe the transaction in the mempool and frontrun, performing a deployment while blocking the original one. This serves as both a denial-of-service and loss-of-funds attack vector: a user could pair the deployment with a transfer of funds to the predicted address, and unintentionally send funds to an SsoAccount they do not control.
Consider mitigating this situation by adding a storage variable that serves as uniqueAccountId, and is incremented with each successful deployment. Additionally, consider using the msg.sender as part of the salt.
Update: Resolved in pull request #295 and pull request #309.
In the _validateTransaction function of the SsoAccount contract, the decoding of the _transaction.signature field is handled by the decodeSignature function of the SignatureDecoder library. However, since the validatorData output is not needed in this particular case, consider using the decodeSignatureNoHookData function from the SignatureDecoder library instead.
Update: Resolved in pull request #269 at commit 9b4591a.
Throughout the codebase, multiple opportunities for improved naming were identified:
keyExists variable of the WebAuthValidator contract will be assigned true if the key is new and false if it is updated. Hence, consider changing the variable's name to keyIsNew to prevent the misunderstanding of such output.OwnerManager contract are named as k1<...>Owner (k1AddOwner, k1IsOwner, K1AddedOwner, and others). Since they refer to an entity called k1Owner, consider changing the names to adhere to the k1Owner<...> and <...>K1Owner formats. This would also be more consistent with the rest of the codebase (HookAdded, addModuleValidator).Update: Resolved in pull request #269 at commit a39df4d and in pull request #308.
Throughout the codebase, multiple instances of dependencies being imported indirectly from other packages were identified:
INonceHolder and IContractDeployer interfaces are imported from Constants.sol.IERC165 interface is imported from TokenCallbackHandler.sol.Indirect imports can create confusion and potentially cause issues with the version management of the underlying files. In order to improve the readability of the codebase and reduce the possibility of introducing bugs when maintaining the code, consider resolving the instances mentioned above and reviewing the codebase for other occurrences.
Update: Resolved in pull request #267.
Throughout the codebase, multiple instances of typographical errors were identified:
IHookManager, "it's" should be "its".ISsoAccount, "are contract" should be "are contracts".Consider correcting any instances of typographical errors to improve the clarity and readability of the codebase.
Update: Resolved in pull request #269 at commit 2c6f44c.
The interface and implementation of the ISsoAccount.initialize function mismatch due to having a different name for the "owners" function parameter: k1Owners and initialK1Owners, respectively.
Consider matching the two names to improve the function's clarity.
Update: Resolved in pull request #277.
The IModule.sol file is lacking an SPDX license identifier.
To avoid legal issues regarding copyright and follow best practices, consider adding SPDX license identifiers to files as suggested by the Solidity documentation.
Update: Resolved in pull request #262. Additionally, a pragma solidity ^0.8.24; directive was added in pull request #304.
Throughout the codebase, multiple instances of functions with unnecessarily permissive visibility were identified:
_addHook and _removeHook functions in HookManager.sol with internal visibility could be limited to private._k1RemoveOwner function in OwnerManager.sol with internal visibility could be limited to private._addValidationKey function in SessionKeyValidator.sol with internal visibility could be limited to private.checkCallPolicy and remainingLimit functions in SessionLib.sol with internal visibility could be limited to private._validateTransaction, _incrementNonce, _executeCall, and _safeCastToAddress functions in SsoAccount.sol with internal visibility could be limited to private._removeModuleValidator function in ValidatorManager.sol with internal visibility could be limited to private.supportsInterface function in WebAuthValidator.sol with public visibility could be limited to external.To better convey the intended use of functions, consider changing a function's visibility to be only as permissive as required.
Update: Resolved in pull request #278.
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.
Throughout the codebase, multiple instances of mappings without named parameters were identified:
accountMappings state variable in the AAFactory contractsessionCounter and sessions state variables in the SessionKeyValidator contractConsider adding named parameters to mappings in order to improve the readability and maintainability of the codebase.
Update: Resolved in pull request #279.
Throughout the codebase, multiple instances of misleading documentation were identified:
k1AddOwner and k1RemoveOwner functions from the IOwnerManager interface imply that the functions can be called by whitelisted modules, which is not the case.HookManager and OwnerManager contracts imply that the addresses are stored inside linked lists, whereas they are stored inside enumerable sets.hooks for initialization, whereas only moduleValidators and k1Owners are set.Consider addressing the above instances of misleading documentation to improve the quality and clarity of the codebase.
Update: Resolved in pull request #287.
Since Solidity version 0.8.4, custom errors provide a cleaner and more cost-efficient way to explain to users why an operation failed. Even though there are cases where they are being used, it is recommended to use them consistently.
The AAFactory.sol, SessionKeyValidator.sol, SessionLib.sol, TimestampAsserterLocator.sol, and WebAuthValidator.sol files use require messages. Consider updating them to use custom errors consistently.
Update: Resolved in pull request #298 at commit a955d59.
During development, having well-described TODO comments will make the process of tracking and solving them easier. However, if not addressed in time, these comments might age and important information for the security of the system might be forgotten by the time it is released to production. Thus, such comments should be tracked in the project's issue backlog and resolved before the system is deployed.
Throughout the codebase, multiple instances of TODO comments were identified:
TODO comment in line 247 of SessionLib.solTODO comment in line 75 of SsoAccount.solMoreover, there are places in the codebase, such as in the CallSpec struct of the SessionLib library, that have questions about the current implementation and whether further changes should be made to it. Ideally, all TODO comments and open questions about the codebase should be implemented and resolved before reaching production.
Consider removing all instances of TODO comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO to the corresponding issues backlog entry.
Update: Resolved. The Matter Labs team stated:
The issues are already tracked in the backlog, however, the comments are still left in the codebase for additional context.
The HookManager contract and the IValidationHook and IExecutionHook interfaces have been changed compared to the original code forked from Clave. However, they still reference Clave as the author.
In modified contracts, consider stating that while Clave was the original author, extensive modifications have been made since then.
Update: Resolved in pull request #288.
In the WebAuthValidator, an implicit cast is performed to compare the bytes32 values of r and s to an explicit uint256 zero.
To improve the readability of the code, consider explicitly casting the bytes32 to uint256, as done by OpenZeppelin's P256 library.
Update: Resolved in pull request #286 and pull request #310.
rawVerify Function Could Be FacilitatedThe rawVerify function of the WebAuthValidator contract is used to strictly test the validity of a transaction signature, without going through the other transaction validation steps. Note that compared to the validateTransaction function, the rawVerify function does not receive the authenticatorData, clientDataJSON and rs as parameters but rather the already signed message. This creates an inconsistency that complicates user experience and creates room for error due to the different inputs expected by the two functions. Since the _createMessage function is private, a user would have to implement their own means of obtaining the message.
In order to reduce the probability of errors during off-chain message creation, consider updating the rawVerify function to receive a fat signature as well, which it should first deconstruct to authenticatorData, clientDataJSON, and rs before creating the message and feeding it to callVerifier.
Update: Resolved in pull request #312 at commit 4004ec0. The function used for only testing purposes has been removed. The Matter Labs team stated:
Removed to reduce confusion. We might come back to new tests later (larger change) that cover provided suggestions instead of the standalone precompile flow comparison.
SessionKeyValidator's validateTransaction Function Disregards Signature ParameterThe validateTransaction function of the SessionKeyValidator contract decodes the transaction.signature field instead of decoding the supplied signature parameter. While this does not pose a security risk, it is inconsistent with how WebAuthValidator is implemented.
Consider making code behavior consistent by decoding the supplied signature field in all cases.
Update: Resolved in pull request #307.
Throughout the codebase, multiple instances of contracts having an inconsistent order of functions were identified:
SessionKeyValidator, SsoAccount, and WebAuthValidator contracts interlace functions with different visibilities. Consider either consistently ordering by logical sense or grouping the functions by visibility.SessionLib library interlaces enums and structs.WebAuthValidator contract interlaces storage variables with events.To improve the project's overall legibility, consider standardizing ordering throughout the codebase as recommended by the Solidity Style Guide (Order of Functions). Alternatively, consider ordering by logical sense.
Update: Acknowledged, will resolve. The Matter Labs team stated:
It was decided to postpone reordering due to it being potentially disruptive to ongoing development (as it introduces a lot of conflicts).
The IModule interface states that a call to the onInstall function "MUST revert on error (e.g., if the module is already enabled)". However, the current implementation allows the WebAuthValidator and the SessionKeyValidator contracts to call such function multiple times when calling it directly from the SsoAccount contract with a different data input after the validators have been enabled.
In favor of keeping a consistent codebase, consider either restricting the calls to already enabled validators or redefining the specifications.
Update: Resolved in pull request #298 at commit f13d0bd.
During the session creation in the SessionKeyValidator contract, the expiresAt parameter is used to set the minuteBeforeExpiration variable to either 0 or one minute before the supplied expiration. If minuteBeforeExpiration is 0, the transaction will revert within the assertTimestampInRange function of the TimestampAsserter contract.
Consider reverting earlier and explicitly, by adding an if statement that reverts the transaction if sessionSpec.expiresAt is smaller than 60.
Update: Resolved in pull request #298 at commit a955d59.
The code under review introduces an innovative and modular smart contract account design that supports customization with flexible transaction validation methods and execution hooks. These accounts can be created and initialized without restrictions, integrating seamlessly into the ZKsync account abstraction flow, just like the existing DefaultAccount.
The project offers significant flexibility when it comes to implementing and attaching hooks or validators to an SsoAccount contract, the only requirement being the presence of the onInstall and onUninstall methods. While convenient as it supports creative development, this freedom of implementation can cause errors when integrating with the SsoAccount contract. Hence, addresses with privileged rights over an SsoAccount contract should research and do due diligence before attaching a new hook or custom validator.
In addition, while the existing test suite covers a broad range of scenarios, there is room for further improvement. Expanding test coverage and strengthening the suite would help ensure compliance with specifications while identifying potential errors and edge cases in the implementation.
We encourage the Matter Labs team to implement the fixes suggested in this audit. These include enhancing the test suite with extensive unit, integration, and backward-compatibility testing, and resolving outstanding backlog items. Nonetheless, the codebase feels quite robust, with direct flows and helpful documentation. The Matter Labs team has demonstrated exceptional responsiveness and collaboration throughout this process, promptly addressing questions and offering valuable insights. The technical documentation accompanying the codebase has been instrumental in understanding the high-level architecture of the ZKsync SSO components and their upgradeability.