SSO Account OIDC Recovery Solidity Audit

OpenZeppelin Security | May 15, 2025

Table of Contents

Summary

Type
Account Abstraction
Timeline
From 2025-04-03
To 2025-04-11
Languages
Solidity
Total Issues
33 (31 resolved)
Critical Severity Issues
0 (0 resolved)
High Severity Issues
1 (1 resolved)
Medium Severity Issues
3 (3 resolved)
Low Severity Issues
12 (11 resolved)
Notes & Additional Information
17 (16 resolved)

Scope

OpenZeppelin audited the matter-labs/zksync-sso-clave-contracts repository at commit ed21d09.

In scope were the following files:

 src
├── OidcKeyRegistry.sol
├── validators
│   └── OidcRecoveryValidator.sol
└── handlers
    └── ERC1271Handler.sol (diff audit for changes made in 2c20eb0)

System Overview

The code under review introduces a new type of account recovery with OpenID Connect (OIDC) to the SsoAccount, using OIDC along with zero-knowledge proofs to recover account control that has been lost, in addition to the existing recovery validators. Also under review was the ERC1271Handler contract against the 2c20eb0 commit, where the ERC-712 logic was removed to simplify the protocol. For an overview of SSOAccount and other validators, please refer to the previous audit reports.

The new OIDC recovery is useful because it allows users to recover lost account access through external OpenID Providers (OPs), such as Google. For that purpose, two new contracts were introduced: OidcKeyRegistry and OidcRecoveryValidator.

OidcKeyRegistry

The OidcKeyRegistry stores OIDC keys of OPs, which can be added and deleted only by the contract owner. A circular buffer of eight keys is used for each issuer identifier (issHash). When a ninth key is added, it overrides the first key, maintaining the circular structure. Any key in the buffer can be removed by the owner. After removal, the remaining keys are compacted to occupy consecutive storage slots.

OidcRecoveryValidator

The OidcRecoveryValidator is the main contract used for this OIDC-based recovery flow. To use this contract for recovering access, a user must enable it beforehand on their SSOAccount and ensure that WebAuthValidator is also installed as a validator. While the user still has access to their account, they should call the addOidcAccount function. This function requires the hash of OIDC data (constructed by hashing the subject identifier, the audience for the ID Token, the issuer identifier, and a user-specific salt) along with the OIDC issuer as a second argument. The data from this call is stored and later used during the recovery.

Using the OIDC data stored in the contract, along with other parameters required by the zero-knowledge circuit, the user can generate a proof to demonstrate control of the specified account. They can use another account to call the startRecovery function, submitting the proof and other parameters, including the address to be recovered. If the data is valid, the account is set to a “ready to recover” status, and a pending passkey hash is recorded. Note that the proof includes a time limit, after which it can no longer be used for verification.

Once the recovery process begins, anyone can initiate a call from the account being recovered, triggering the validateTransaction function. Successful execution requires providing the valid preimage of the passkey hash. The only allowed contract call at this point is to WebAuthValidator via the addValidationKey function, which sets the validation key for the account and completes the recovery.

ERC1271Handler

The ERC1271Handler contract has been updated in commit 2c20eb0, removing the ERC-712 logic to simplify the flow. OpenZeppelin has been asked to validate the new version of this contract.

Security Model, Trust Assumptions, and Privileged Roles

During the audit, the following trust assumptions regarding the security model and privileged roles were made:

  • The owner of the OidcKeyRegistry is responsible for providing valid and up-to-date OIDC keys. Otherwise, users will be unable to recover access to their accounts.
  • The owner of the SSOAccount must not remove the WebAuthValidator from its validators. Otherwise, the OidcRecoveryValidator cannot be used to recover the account.

Design Choices, Limitations, and Integration Issues

During the assessment, potential concerns related to cross-account and multchain replay scenarios were discussed. While no immediate multichain risks were identified, the project's future roadmap includes building an ecosystem where additional chains may be deployed on top of the current infrastructure.

In this context, including the wallet address being recovered in the senderHash function and the unique nonce may serve as a preventative measure to avoid possible replay attacks across accounts that share auxiliary addresses. It is also worth noting that the current salt derivation process may be considered weak and should be strengthened accordingly. Lastly, the codebase could benefit from expanding the current test suite to cover more edge cases, including zk-proof verification flow.

 

High Severity

Potential Signature Replay Attack in ERC1271Handler

The isValidSignature function is commonly used to verify signatures in scenarios where an external account is not required to initiate a call. It ensures that the contract allows the execution of some logic based on the check of provided inputs.

In the isValidSignature function of the ERC1271Handler contract, insufficient checks may allow nearly any action if the contract calling isValidSignature does not fully validate the provided data or hash. For EOA signatures (65-byte signatures), there are no restrictions on how the hash is constructed, and the function only confirms the signature’s validity and its association with a k1owner. An attacker could reuse a historical transaction hash and signature and execute actions on behalf of the account. Additionally, EIP712 logic was removed in the alternative validation method, permitting cross-chain attacks and the reuse of signatures across different accounts. Previously, _hashTypedDataV4 included the chain ID and the verifier contract in the hash, safeguarding against such misuse.

Consider implementing the approach described in ERC7739, which proposes a defensive rehashing scheme specifically designed to address ERC1271 signature replay vulnerabilities—particularly across multiple smart accounts managed by the same EOA. This approach employs nested EIP-712 typed structures to maintain readability while effectively preventing signature replays. Additionally, consider protecting alternative validation methods, such as those using a validator, from signature replay attacks.

Update: Resolved in pull request #439 and commit 19747f1c. The fix leverages the ERC1271.sol contract from the Solady library. Our team did not conduct an independent security review of this dependency and instead relied on the audit report provided in the GitHub repository. However, the integration of the contract was thoroughly reviewed. The Matter Labs team stated:

We implemented ERC7739 to prevent signature replay attacks as well as ensure that the signed contents are fully visible to the user upon signing.

Medium Severity

Incorrect Key Ordering in _compactKeys Function Leads to Potential Overwriting of New Keys

The _compactKeys function of the OidcKeyRegistry contract manages a list of keys in a ring-like structure, rearranging elements after one is removed to maintain compactness and continuity. However, it handles the ordering process by starting from the current index pointer instead of resetting to zero, leading to unintentional key rearrangements and potential data integrity concerns.

For example, if the key array is [0, key1, key2, key3, key4, 0, 0, 0] with the current keyIndexes pointer at key4, and key3 is removed, the function reorders the keys from that pointer onward, producing [key4, key1, key2, 0, 0, 0, 0, 0]. After this operation, the insertion pointer incorrectly moves to index 2 (key2), causing the next new key to be inserted at index 3. This misalignment risks overwriting key4 first in future operations, contradicting the FIFO principle which dictates that key1 (the oldest key) should be replaced first. Consequently, newer keys could be overwritten prematurely, leading to potential data loss and unintended replacements.

Consider revising the _compactKeys function to preserve the original ordering after each compaction, regardless of the current insertion pointer’s position, ensuring that the FIFO sequence is maintained and preventing the overwriting of newer keys.

Update: Resolved in pull request #429 at commit 1829cf8 and in pull request #435 at commit 8071a0ec.

Insufficient Validation of RSA Moduli and Exponent in _validateKeyBatch

The _validateKeyBatch function of the OidcKeyRegistry contract uses _hasNonZeroExponent and _validateModulus to validate the exponent and modulus, respectively. The _validateModulus function is responsible for verifying RSA moduli in cryptographic operations, ensuring they meet basic structural requirements for secure usage.

The _validateModulus function checks each chunk for size and non-zero values but does not enforce a minimum bit length to prevent factoring attacks. It also does not ensure that the modulus is odd, which is typically required because RSA moduli are the product of two odd primes. In addition, the exponent is only checked against zero, leaving the possibility of smaller exponents that may be vulnerable to known attacks.

Consider implementing a check to ensure the modulus meets a secure minimum bit length (for example, 2048 bits), verifying that it is odd, and enforcing a minimum exponent threshold of at least 65537. These measures will help strengthen the cryptographic properties of the system and mitigate multiple potential attack vectors.

Update: Resolved in pull request #430 at commit 1999421. The Matter Labs team stated:

Regarding the recommendations: - The modulus is always interpreted as a 2048-bit number. The contract receives an array of 17 chunks of 121 bits each. All of them together are later interpreted by the circuit as a 2048-bit modulus. - The recommendation to verify that the modulus is odd has been addressed. We have implemented validation that takes into account the specific Circom formatting used for big numbers, where the modulus is encoded as 17 chunks and the least significant chunk it’s at the left (same order as little-endian for bytes). Our implementation ensures that the first chunk is not 0 mod 2, effectively validating that the modulus is odd as required for RSA security. - Regarding the exponent threshold (65537), the contract no longer handles variable exponents - this value is hardcoded in the circuit, eliminating the need for runtime validation.

Unauthorized Control via Manipulated pendingPasskeyHash in startRecovery Process

When both the OidcRecoveryValidator and WebAuthValidator contract are active for an account, and the account owner loses access, a recovery process can be initiated using a zero-knowledge (ZK) proof. This proof demonstrates ownership of the associated OIDC identity, as registered in the OidcRecoveryValidator contract. Once the proof and associated data are verified, any party can call the startRecovery function to begin account recovery.

Although most parameters in the startRecovery function are validated by both the ZK circuit and the contract, there is no verification of the data.pendingPasskeyHash before it is written to the account’s storage. This enables a scenario where the party initiating recovery (who has access to the valid ZK proof) could submit a malicious pendingPasskeyHash (one for which they possess the corresponding private key). As a result, they can complete the recovery through the WebAuthValidator contract, effectively taking control of the account, even if they were not the intended recipient of the recovery request.

Consider adding a public signal to the ZK proof to bind the pendingPasskeyHash parameter to the proof itself, and store this hash only after the proof has been verified.

Update: Resolved in pull request #431 at commit ba4967a. The Matter Labs team stated:

In order to address this issue we included the pendingPasskeyHash inside the content of the JWT nonce. The nonce content was previously calculated as:

 keccak256(abi.encode(msg.sender, oidcData.recoverNonce, data.timeLimit));

Now it’s calculated as: solidity keccak256(abi.encode(msg.sender, targetAccount, data.pendingPasskeyHash, oidcData.recoverNonce, data.timeLimit)); This ensures that the user actually wanted to use the given passkey, and also makes the pendingKeyHash part of the data being checked by the circuit.

Low Severity

Duplicate kid Values in OidcKeyRegistry Allow for Partial Key Deletion and Retrieval

In the OidcKeyRegistry contract, the addKeys function enables the addition of multiple keys simultaneously. Although this function performs several validations, it lacks a crucial check to prevent the insertion of multiple keys with identical kid values. This oversight may result in inconsistent states within the key registry, particularly when the deleteKey function is used. The deleteKey function is designed to remove a key based on its kid value. However, it only removes the first occurrence of a matching kid, leaving any additional duplicates intact.

For example, if an entity inadvertently adds multiple keys with the same kid and later attempts to revoke a compromised key by its kid, only a single instance—the first match—will be removed. The remaining duplicate keys with the same kid will persist in the registry, potentially leaving the system vulnerable. Furthermore, this issue impacts the getKey function which is intended for retrieving a key by its kid. Similar to the deleteKey function, the getKey function will only return the first key that matches the provided kid, ignoring any additional keys with the same kid that may have been erroneously added.

To mitigate this issue and preserve the integrity of the key management process, consider implementing a validation mechanism in both the addKeys and addKey functions to enforce the uniqueness of each kid value.

Update: Resolved in pull request #432 at commit bc71d5e.

Front-Running in addOidcAccount Account Registration

The OidcRecoveryValidator contract uses a digestIndex mapping to associate unique identifiers (oidcDigest) with user accounts. This mechanism ensures that each identifier is distinct within the system. However, the current implementation allows for a scenario in which a malicious user can perform a front-running attack.

Specifically, an attacker can monitor the pending registration transactions in the mempool. Upon detecting a legitimate user's attempt to register an oidcDigest, the attacker can submit their own transaction using the same oidcDigest, thereby claiming it first. As a result, when the legitimate transaction eventually reaches the addOidcAccount function, the verification step fails due to the oidcDigest already being registered, causing the transaction to revert and preventing the legitimate user from associating their validator with their account.

Note that given that no elastic chain operators have public mem-pools or shared sequencers, this is a possible future issue.

Consider implementing additional verification steps or securing transaction commitment mechanisms that bind the registration request directly to the originating account. This approach would effectively prevent attackers from preemptively claiming oidcDigest identifiers.

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 design rationale lies in avoiding the requirement for users to input their address during account recovery. In the future, we might consider removing the one-to-one OIDC-to-Google account restriction, which would significantly complicate the UX but would prevent this kind of front-running.

Builtin Getter for OIDCKeys Does Not Return n

The OIDCKeys 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 (issHash, kid, and e), and it will omit complex data structures such as n. 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 getKeys getter, consider removing the automatic getter (that does not retrieve all the data) by reducing the variable's visibility.

Update: Resolved in pull request #433 at commit cd57ff7.

Potential Loss of Ownership During Transfer

The OidcKeyRegistry contract implements a single-step ownership transfer process which may lead to unintended loss of ownership if the new owner address is incorrect or inaccessible. This pattern does not provide a safeguard to recover from such scenarios, and any mistake in the transfer can result in permanent loss of administrative control.

The issue lies in the absence of a confirmation step during the ownership transfer. In the current implementation, ownership is immediately reassigned once the transfer function is called, without requiring the new owner to accept the role. This increases the risk of errors during execution, particularly if an incorrect or unprepared address is specified.

Consider using the Ownable2StepUpgradeable contract from the OpenZeppelin library, which introduces a two-step transfer pattern. This approach requires the new owner to explicitly accept ownership, reducing the chance of accidental loss of control and improving operational safety.

Update: Resolved in pull request #422 at commit 77036be.

Inflexible Recovery Process Termination in OidcRecoveryValidator

The OidcRecoveryValidator contract, as currently implemented, lacks a method to halt an initiated recovery process without complete termination of the account's linkage to its issuer. Once the recovery process is initiated, if the subsequent transaction that triggers the validateTransaction function is not executed, the only way to abort the recovery process is by invoking the deleteOidcAccount function.

However, the deleteOidcAccount function is designed to sever the connection between an account and its issuer entirely. Utilizing it to discontinue an ongoing recovery process consequently eliminates the possibility of future recoveries unless the addOidcAccount function is called again to reestablish the linkage. This binary choice compels users to decide between retaining a potentially vulnerable recovery option or completely losing their account's recovery functionality, a decision that is less than ideal for user security and convenience.

To address this limitation, consider introducing a feature that permits users to selectively revoke or invalidate an ongoing recovery process without the need to eliminate the entire account and issuer connection. This enhancement would significantly improve the flexibility and security of the recovery process, offering users better control over their recovery options.

Update: Resolved in pull request #441 at commit d52ddf9. The Matter Labs team stated:

We added a new method cancelRecovery that allows a user to cancel a recovery for their own account. This can be used for the case where an OIDC based recovery was started but not finished, and then the user was able to recover their account in a different way (for example, using guardians).

We also added a time validation as part of L-06, which also works a way to cancel an ongoing recovery.

Delayed Recovery Validation May Compromise Account Security

In the OidcRecoveryValidator contract, the account recovery process flow is divided into two separate function calls, with the startRecovery and validateTransaction functions, due to the high gas consumption associated with verifying zero-knowledge proofs. This design choice introduces a potential security vulnerability related to the timing of these operations.

The initial startRecovery function includes a check to ensure that the recovery process is not initiated after the expiration of the proof. However, this verification does not account for the time elapsed between the execution of the startRecovery call and the validateTransaction call. Consequently, there exists a significant time window in which the recovery process can be completed long after the execution of the startRecovery contract. This delayed validation poses a risk, especially if account control is regained through alternative means, but the recovery process remains pending execution via validateTransaction through the validation on the OidcRecoveryValidator contract.

To mitigate this risk and enhance the security of the recovery process, consider implementing an additional check within the validateTransaction function to verify the timeliness of the recovery action, successfully validating the transaction if it is still within the time window. This check should ensure that the recovery is still valid at the time of the validateTransaction function execution, thereby eliminating the possibility of having an undesired time window in the recovery flow. Moreover, it is worth noting that a user might initiate recovery of the account through a different mechanism (such as with a Guardian) even if they are still in the time window for the OIDC recovery process.

Update: Resolved in pull request #423 at commit 21d16e5. The Matter Labs team stated:

We have implemented a time validation window of 10 minutes between the initiation and execution of the recovery process. This duration is considered sufficient since the most time-intensive operation - proof generation - occurs before the startRecovery function is called. By enforcing this time constraint, we ensure that delayed recoveries cannot be executed beyond the intended window.

Old digestIndex Not Released on OIDC Account Update

The OidcRecoveryValidator contract uses a digestIndex to ensure the uniqueness of OIDC (OpenID Connect) digests associated with user accounts. Each time a user adds or updates their OIDC account via the addOidcAccount function, a digest representing the OIDC data is stored to prevent reuse. This mechanism ensures that no two accounts can register the same OIDC identity, maintaining integrity across the recovery process.

However, when a user updates their OIDC account, the previously assigned digestIndex value is not cleared. As a result, the old OIDC digest remains reserved under the user's account even though it is no longer in use. If the user later attempts to restore a previously valid OIDC digest, for example, to revert to an earlier identity, they will be blocked by the uniqueness check, which still considers the old digest as taken, despite it originally belonging to the same user.

Consider explicitly clearing the user's old digestIndex value before assigning the new one during an account update. This would allow legitimate reuse of previously held digests and ensure consistency in digest management.

Update: Resolved in pull request #424 at commit d7a71ca.

The addOidcAccount Function Always returns false

In the addOidcAccount function, the returned boolean indicates whether an account has been newly added or merely updated. The function checks the current state by inspecting the length of the oidcDigest field within the stored account data, which is then emitted via the OidcAccountUpdated event and returned by the function. According to the documentation, the function should return true if a new key is added and false if an existing key is updated.

However, the logic currently implemented misunderstands how length is calculated for the bytes32 type. The length of a bytes32 value is always 32 bytes, regardless of the content, meaning the accountData[msg.sender].oidcDigest.length == 0 condition will consistently evaluate to false. Consequently, the returned value and the emitted event will incorrectly indicate that an account is always updated rather than newly created, potentially causing confusion or misinterpretation of account status. It is worth mentioning that the correct check is being performed in the oidcDataForAddress function.

In favor of being able to detect when a new key is added and improving code consistency, consider whether comparing the value of oidcDigest to zero (e.g., oidcDigest == bytes32(0)) might more accurately reflect whether the account is new, rather than relying on its length.

Update: Resolved in pull request #424 at commit d7a71ca. The Matter Labs team stated:

This issue has no PR for itself because it was resolved as part of L-07.

Inconsistencies in Account Recovery Data During Recovery Process

When an OpenID Connect (OIDC) identifier is linked to an account, the addOidcAccount function only updates three specific parameters in the accountData mapping and one in the digestIndex mapping for the concerned account. This selective update process neglects the readyToRecover, pendingPasskeyHash, and recoverNonce fields, leaving them at their default values or preserving their previous states. Such behavior creates a risk of inconsistencies, as outdated data could potentially lead to misleading behaviors or the misuse of obsolete recovery states.

The core issue arises when the recovery process is initiated and a specific set of parameters is verified. However, before the validateTransaction call occurs, these parameters might get overwritten by a subsequent invocation of the addOidcAccount function by the account. This action updates the aforementioned parameters without fully resetting the OidcData variables, inadvertently leaving the readyToRecover flag active and the pendingPasskeyHash variable set from a prior session. This scenario becomes problematic when an account initiates a recovery using a specific publicKey and, after gaining access through another recovery method, the user decides to update the accountData by calling the addOidcAccount function, not realizing that the original publicKey can still be set when using the OidcRecoveryValidator contract as the validator.

Realizing such a scenario requires the functions to be executed from the account itself, indicating that access was previously granted through another method and that the user needs to proactively participate in order for this to happen. Nonetheless, to minimize the risk associated with unexpected recovery flows and to enhance the integrity of the recovery process, particularly not mixing data from 2 different recovery processes, consider implementing mechanisms that reset or suitably update all pertinent fields within the accountData mapping whenever an OIDC identifier is added or the recovery process is initiated.

Update: Resolved in pull request #425 at commit aa78035.

Insufficient Validation in Recovery Process May Lead to Wasted Recovery Attempts

The OidcRecoveryValidator contract introduces a mechanism for account recovery, culminating in the addition of a new publicKey to the WebAuthValidator contract. Initially, an account is linked to an oidcDigest, and upon the need for recovery, a submission process is initiated that verifies proofs against the oidcDigest. If verification is successful, any participant can execute a transaction to incorporate the new publicKey into the WebAuthValidator.

However, a problem arises when the credentialId and originDomain parameters do not meet some of the requirements in the WebAuthValidator contract, in which the call will not revert but will instead return a false output. In such a scenario, the call will be considered successful without reversion, regardless of the non-inclusion of the publicKey. As a result, the recovery attempt will be consumed and a new recovery will be needed.

This flaw not only risks the unnecessary consumption of recovery attempts due to parameter mismatches but also exposes the problem of users who, knowledgeable of the user's publicKey, could deliberately consume the recovery attempt by submitting transactions with arbitrary, incorrect parameters alongside the publicKey, that will not pass the WebAuthValidator contract requirements.

To mitigate the risk of wasted recovery attempts and protect against potential misuse, consider binding the credentialId and originDomain arguments within the zkProof and storing this binding in the contract for subsequent validation in the validateTransaction function. This change ensures that these parameters remain immutable during the validateTransaction call, preventing unauthorized or unintended consumption of recovery attempts.

Update: Resolved in pull request #426 at commit 35ff79c.

Insufficient Validation of iss Argument Length in addOidcAccount

The addOidcAccount function includes a check to ensure that the iss (issuer) argument is not empty. This value is later used within a zero-knowledge circuit, where its length is expected to be less than 32 characters, based on the template argument definition and the constant value provided. While the lower bound is enforced by the existing validation, the upper bound (critical for zero-knowledge proof verification) is not.

As a result, values longer than the expected maximum can be stored, even though they would not be valid for proof generation or verification in the zero-knowledge circuit. This creates a discrepancy between what can be stored on-chain and what can be successfully proven off-chain.

Consider enforcing an upper bound on the iss argument length to ensure that it is strictly less than 32 characters, in addition to the existing non-zero length check.

Update: Resolved in pull request #427 at commit 22d3b4f.

OidcRecoveryValidator Does Not Follow the ERC-1271 Flow

The validateSignature function of the OidcRecoveryValidator contract reverts when being called. This happens because the validator is only supposed to provide a recovery mechanism to the account instead of validating any other kind of transaction. However, its current implementation deviates from the expected ERC-1271 flow by reverting on calls instead of returning a boolean value. This behavior contrasts with the intended flow, where the validateSignature function should signal a negative outcome by returning false to let the ERC1271Handler contract not return the magic value.

Given that analogous validator contracts within the system return false under similar conditions, aligning the OidcRecoveryValidator contract's behavior with this pattern would enhance consistency and predictability in the signature validation process. Therefore, consider modifying the validateSignature function to return false instead of reverting, ensuring that it adheres to the established ERC-1271 flow.

Update: Resolved in pull request #428 at commit 5141a07.

Notes & Additional Information

Using uint8 for Constants Is More Expensive Than uint256

In both OidcKeyRegistry and OidcRecoveryValidator, uint8 is being used for constants (e.g., for the MAX_KEYS constant). However, this introduces unnecessary overhead during deployment, as additional range checks are required, making the deployment more expensive. For instance, using uint8 in the OidcKeyRegistry costs around 28,267,888 gas, whereas uint256 costs about 28,175,109, saving approximately 92,779 gas.

Since these values are constants and manually verified to be within the expected range, there is no added risk in switching to uint256. Consider updating uintN constants to uint256 to optimize deployment costs.

Update: Resolved in pull request #399 at commit d0c788e.

Inconsistent License in OidcKeyRegistry.sol

The OidcKeyRegistry.sol file uses the UNLICENSED license, while the rest of the codebase is either licensed under GPL-3.0 or MIT. This inconsistency may create uncertainty around usage and distribution.

Consider updating the license in OidcKeyRegistry.sol to align with the rest of the codebase, such as GPL-3.0 or MIT.

Update: Resolved in pull request #400 at commit 894321c.

Redundant Local Variable in _validateModulus

In the _validateModulus function, the uint256 limit variable is calculated as (1 << 121) - 1. Currently, the limit is computed within the function on every invocation and, as this value remains unchanged throughout the function execution, recalculating it repeatedly consumes unnecessary gas.

Consider defining limit as a constant outside the _validateModulus function to avoid unnecessary recalculations and reduce gas usage.

Update: Resolved in pull request #401 at commit 7e36375.

Use of uint Instead of uint256

While uint is an alias for uint256, for clarity and consistency, it is recommended to use uint256 explicitly.

Within OidcRecoveryValidator.sol, multiple instances of uint being used instead of uint256 were found:

  • The uint in line 86
  • The uint in line 87
  • The uint in line 88
  • The uint in line 217
  • The uint in line 221
  • The uint in line 226
  • The uint in line 334

In favor of explicitness, consider replacing all instances of uint with uint256.

Update: Resolved in pull request #402 at commit 13312f8.

Magic Numbers in the Code

Throughout the codebase, multiple instances of literal values with unexplained meanings were identified.

  • The 8s literal in OidcRecoveryValidator.sol
  • The 248s literal in OidcRecoveryValidator.sol
  • The 32 literal in OidcRecoveryValidator.sol
  • The 32 literal in OidcRecoveryValidator.sol
  • The 8 literal in OidcRecoveryValidator.sol
  • The 8 literal in OidcRecoveryValidator.sol

Consider defining and using constant variables instead of using literals or properly documenting the literals values to improve the readability and maintainability of the codebase.

Update: Resolved in pull request #403 at commit acd7822.

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.

In OidcRecoveryValidator.sol, multiple instances of require messages were identified:

For conciseness, gas savings, and consistency with the rest of the codebase that makes use of custom errors, consider replacing require and revert messages with custom errors.

Update: Resolved in pull request #404 at commit 2e27794.

Missing Event Emission in startRecovery

When the startRecovery function of the OidcRecoveryValidator contract is invoked, it does not trigger any event.

Consider emitting an event to reflect that a recovery has started, which would improve the clarity of the codebase and ease off-chain monitoring.

Update: Resolved in pull request #405 at commit a375b7a.

State Variable Visibility Not Explicitly Declared

Within OidcRecoveryValidator.sol, multiple instances of state variables lacking an explicitly declared visibility were identified:

For improved code clarity, consider always explicitly declaring the visibility of state variables, even when the default visibility matches the intended visibility.

Update: Resolved in pull request #434 at commit 41e77cb.

Circular Buffer Initialization Causes Counterintuitive Key Indexing

In the implementation of the OidcKeyRegistry contract, keys are added to a circular buffer with an initial offset. This design choice results in non-intuitive behavior during the buffer's initial population cycle. Specifically, when fully initializing the ring with a series of keys, the last key provided in the sequence is assigned to the zero index of the buffer. This occurs due to the offset applied during the key addition process, which effectively shifts the position of the initially added keys.

For example, when populating a ring buffer intended to hold eight keys with five specific keys, one might expect the keys to sequentially occupy indices from zero to four. However, the actual placement after the first addition cycle ranges from indices one to five (| 0 | k1 | k2 | k3 | k4 | k5 | 0 | 0 |). Similarly, when adding a full cycle of eight keys, the last key (k8) occupies the zero index (| k8 | k1 | k2 | k3 | k4 | k5 | k6 | k7 |). This disrupts the expected sequential ordering established during the initialization.

Consider adjusting the key addition logic to ensure that the first key added occupies the zero index of the buffer, maintaining consistent and intuitive indexing throughout the buffer's lifecycle.

Update: Resolved in pull request #435 at commit 8071a0e.

Missing Interface for OidcRecoveryValidator Contract

The OidcRecoveryValidator contract lacks an interface that corresponds to its available implementation functions, such as addOidcAccount and deleteOidcAccount. In addition, definitions such as events, errors, and structs can be included within this interface to improve developer experience and protocol interoperability. This change would streamline the contract's structure and enhance usability across different development contexts. Moving these definitions into the interface would also simplify interactions, event decoding, error handling, and passing structs as input or output parameters in other contracts.

Consider creating an interface that directly includes the above-mentioned definitions. This approach aligns with best practices in smart contract design and enhances developer experience by providing a clearer, more intuitive way to interact with the contract’s functionalities.

Update: Resolved in pull request #408 at commit 02e0df0.

Refactor and Improvement Opportunities

Throughout the codebase, multiple opportunities for code improvement were identified:

  • In the OidcRecoveryValidator contract, two contract addresses (keyRegistry and verifier) are stored as raw address types and then cast to their respective contract types only when accessed. These casting operations are performed only once throughout the contract. Instead of casting the address to a contract at the point of use, consider updating the constructor to include the casting operation and adjust the type of the storage variable accordingly. This approach would not only streamline the contract code by eliminating the need for casting at the point of use but also potentially reduce gas costs associated with the casting operation.
  • The OidcDigestAlreadyRegisteredInAnotherAccount custom error reverts with the oidcDigest argument when the digest has already been registered. However, the oidcDigest is provided by the caller and is therefore already known, making its inclusion potentially redundant. It would be more useful if the error included the address of the account that has registered the digest. Consider replacing such a parameter.
  • The for loop in the startRecovery function iterates key.n.length times and the auxiliary variable index is incremented on each iteration. However, after the loop completes, index will be equal to the number of elements in key.n. Consider not incrementing the index variable inside the loop. Instead, assign the length of key.n directly to index.
  • The casting of each Key.n in the for loop is unnecessary, as each n element in the array is already a uint256 variable. Consider removing such casting.
  • The current implementation of the startRecovery function splits senderHash into two values using multiple bitwise shift operations. This can be optimized by applying a single bitwise mask after calling the _reverse function, setting the most significant byte (in little-endian format) to zero. The last element of the publicInputs can be simplified by truncating senderHash to uint8 and then casting it back to uint256. This approach reduces stack operations, decreases the number of constants pushed onto the stack, and achieves better gas efficiency compared to multiple shifts.
  • The current implementation of the _reverse function reverses bytes by using additional intermediate variables and multiple arithmetic operations per iteration, consuming approximately 29k gas per call. This can be optimized by replacing it with a bitwise accumulation approach, where the result is built by repeatedly shifting the output left by one byte and combining it with the least significant byte of the input. The optimized version reduces gas usage to approximately 3.9k per call.

To improve the readability and gas efficiency of the codebase, consider applying the aforementioned recommendations.

Update: Resolved in pull request #410 at commit 26529e1.

Unnecessary override Keyword in onInstall and onUninstall

In the OidcRecoveryValidator contract, the onInstall and onUninstall functions are marked with the override keyword. This usage suggests that these functions are intended to override implementations from a parent contract. However, these functions implement abstract function declarations from an interface instead of overriding concrete implementations from an ancestor contract. In Solidity, the override keyword is used to indicate that a function is overriding a function from a base contract. When a function is merely implementing an interface's method, the use of override is not necessary. In this case, the use of the override keyword could potentially lead to confusion regarding the inheritance structure of these functions, as it points to an inheritance relationship that does not exist.

To enhance code clarity and accurately reflect the inheritance and implementation structure, consider removing the override keyword from the onInstall and onUninstall function declarations in all places where it is not needed.

Update: Resolved in pull request #411 at commit 5b48556.

Redundant External Call to hashIssuer in OidcRecoveryValidator

The hashIssuer function of the OidcKeyRegistry contract is invoked in the startRecovery function of the OidcRecoveryValidator contract. This function performs a simple hashing operation on a single input argument. Since the logic is minimal and does not rely on any internal state of the OidcKeyRegistry contract, calling it externally introduces unnecessary complexity and potential gas overhead.

Consider moving the hashIssuer function directly into the OidcRecoveryValidator contract or duplicating its logic within the contract to avoid the external call.

Update: Acknowledged, not resolved. The Matter Labs team stated:

The method hashIssuer inside OidcKeyRegistry ensures consistent hash generation for issuers. While duplicating this logic would yield minimal gas savings, it would create redundant code that might need synchronization if changes occur in the future.

Considering the low gas costs in the elastic chain ecosystem, we consider that maintaining the external method call is preferable to duplicating the logic.

Endianness Mismatch in senderHash Representation Between Contract and Circuit

The startRecovery function splits the 32‑byte senderHash into two 31‑byte fields to fit the circuit’s input size. The first part is produced by right‑shifting the hash, which moves the zero inserted by the shift to the least‑significant end, and then discarding that zero, yielding a little‑endian sequence B1 … B31 (with B0 already extracted). The second part is obtained by isolating the least‑significant byte (B0) without reversal, leaving it unchanged in the least‑significant byte of the word. As a result, the same logical value is encoded partly in little‑endian and partly in its original order before being supplied to the circuit.

Because the circuit appears to expect a uniform byte order for the entire 32‑byte value, this mixed representation can cause the circuit to reconstruct an incorrect senderHash, leading to failed proofs or other unexpected verification results.

Consider encoding both parts in the single endianness expected by the circuit—for example, reverse the full 32‑byte hash before splitting or omit reversal for both parts—so that the contract and circuit consume an identical byte order and the hash can be reconstructed unambiguously.

Update: Resolved in pull request #436 at commit d0cf420. The Matter Labs team stated:

We changed the the serialization of the txHash to simply take the first 31 bytes in the first element (without any kind of reverse) and the last byte in a second element (also without any revert).

We also made the changes in the circuit input generation code to be coherent with this. Those changes were also considered in the documentation of the circuit.

This also allowed us to remove the _reverse internal function that was only being used here.

Misleading Comment Regarding OIDC Provider Public Key Format

In the OidcRecoveryValidator contract, the startRecovery function includes a comment intended to describe the structure of the publicInputs array. Specifically, the comment currently states: “First CIRCOM_BIGINT_CHUNKS elements are the OIDC provider public key.” This can be misleading to readers who assume the RSA public key includes both the modulus and the exponent, as per standard cryptographic definitions.

The implementation, however, only includes the modulus in these elements. The exponent is not part of this structure, contrary to what might be inferred from the current comment. This inconsistency can cause confusion for developers or auditors reviewing the code, particularly those relying on the comment to understand the data layout without inspecting the logic in detail.

Consider updating the comment to accurately reflect the content of the input, such as: “First CIRCOM_BIGINT_CHUNKS elements are the OIDC provider modulus.” This will improve code readability and help ensure correct assumptions about the key structure.

Update: Resolved in pull request #442 at commit 5d8d557.

Redundant Inheritance from VerifierCaller in OidcRecoveryValidator

The OidcRecoveryValidator contract inherits from the VerifierCaller contract. However, in the current implementation of the OidcRecoveryValidator contract, none of the functions or features provided by the VerifierCaller contract are utilized.

This results in unnecessary inheritance, which can increase the contract size and complicate the codebase without providing any functional benefit.

Consider removing the inheritance of the VerifierCaller contract in the OidcRecoveryValidator contract to simplify the contract and reduce potential maintenance overhead.

Update: Resolved in pull request #408 at commit 02e0df0f. The Matter Labs team stated:

This issue was solved as part of N-10 fix.

Inconsistent Handling of addOidcAccount Return Value in OidcRecoveryValidator

The current onInstall implementation ignores the return value of the addOidcAccount function. This might be confusing for readers, as it’s not immediately clear whether the return value is intentionally disregarded or if it’s an oversight.

Consider documenting the reasoning behind ignoring the return value of the addOidcAccount function.

Update: Resolved in pull request #440 at commit 0e6415e. The Matter Labs team stated:

The return value was removed because it was not being utilized anywhere in the codebase.

 
 

Conclusion

The reviewed code introduces an OIDC-based account recovery mechanism to enhance the existing SsoAccount setup, as well as updates to the ERC1271Handler to streamline the protocol. This integration leverages zero-knowledge proofs for secure and flexible recovery procedures.

Overall, the addition of OpenID Connect recovery presents a clear path for users to restore access through familiar external identity providers. The creation of OidcKeyRegistry for managing OP keys and the OidcRecoveryValidator for facilitating zero-knowledge-based proof-of-control effectively extends the account’s resilience. Integrating these components with the existing WebAuthValidator ensures a consistent process for validation and eventual recovery.

The Matter Labs team is greatly appreciated for being responsive and cooperative throughout the audit process.

 

Related Posts