SSO账户OIDC恢复Solidity审计

本次审计主要评估了 Matter Labs 的 zksync-sso-clave-contracts 仓库中关于使用 OpenID Connect (OIDC) 进行账户恢复的合约。

目录

概要

TypeAccount AbstractionTimelineFrom 2025-04-03To 2025-04-11LanguagesSolidityTotal Issues33 (31 resolved)Critical Severity Issues0 (0 resolved)High Severity Issues1 (1 resolved)Medium Severity Issues3 (3 resolved)Low Severity Issues12 (11 resolved)Notes & Additional Information17 (16 resolved)

范围

OpenZeppelin 审计了 matter-labs/zksync-sso-clave-contracts 仓库，提交哈希为 ed21d09。

在审计范围内的文件如下：

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

系统概述

本次审查的代码引入了一种新的账户恢复类型，使用 OpenID Connect (OIDC) 到 SsoAccount。除了现有的恢复验证器之外，还使用 OIDC 和零知识证明来恢复丢失的账户控制权。此外 ERC1271Handler 合约也根据 2c20eb0 提交进行了审核，其中 ERC-712 逻辑被删除以简化协议。有关 SSOAccount 和其他验证器的概述，请参阅之前的审计报告。

新的 OIDC 恢复非常有用，因为它允许用户通过外部 OpenID 提供商 (OP)，比如 Google，来恢复丢失的账户访问权限。为此，引入了两个新的合约：OidcKeyRegistry 和 OidcRecoveryValidator。

OidcKeyRegistry

OidcKeyRegistry 存储 OP 的 OIDC 密钥，这些密钥只能由合约所有者添加和删除。每个发行者标识符（issHash）使用八个密钥的循环缓冲区。当添加第九个密钥时，它会覆盖第一个密钥，保持循环结构。缓冲区中的任何密钥都可以由所有者删除。删除后，剩余的密钥会被压缩以占据连续的存储槽。

OidcRecoveryValidator

OidcRecoveryValidator 是用于此基于 OIDC 的恢复流程的主要合约。要使用此合约恢复访问权限，用户必须事先在其 SSOAccount 上启用它，并确保 WebAuthValidator 也作为验证器安装。当用户仍然可以访问他们的账户时，他们应该调用 addOidcAccount 函数。此函数需要 OIDC 数据的哈希值（通过哈希主题标识符、ID Token 的受众、发行者标识符和用户特定的盐来构造），以及 OIDC 发行者作为第二个参数。来自此调用的数据被存储，并在以后的恢复期间使用。

使用存储在合约中的 OIDC 数据，以及零知识电路所需的其他参数，用户可以生成一个证明来演示对指定账户的控制权。他们可以使用另一个账户调用 startRecovery 函数，提交证明和其他参数，包括要恢复的地址。如果数据有效，则账户设置为“准备恢复”状态，并记录一个待处理的密钥对哈希。请注意，该证明包括一个时间限制，超过该时间限制后，它将不再用于验证。

一旦恢复过程开始，任何人都可以从正在恢复的账户发起调用，触发 validateTransaction 函数。成功执行需要提供密钥对哈希的有效原像。此时唯一允许的合约调用是通过 addValidationKey 函数调用 WebAuthValidator，该函数设置账户的验证密钥并完成恢复。

ERC1271Handler

ERC1271Handler 合约已在提交 2c20eb0 中更新，删除了 ERC-712 逻辑以简化流程。OpenZeppelin 被要求验证此合约的新版本。

安全模型、信任假设和特权角色

在审计期间，对安全模型和特权角色做出了以下信任假设：

  • OidcKeyRegistry 的所有者负责提供有效且最新的 OIDC 密钥。否则，用户将无法恢复对其账户的访问权限。
  • SSOAccount 的所有者不得从其验证器中删除 WebAuthValidator。否则，OidcRecoveryValidator 无法用于恢复账户。

设计选择、局限性和集成问题

在评估期间，讨论了与跨账户和多链重放场景相关的潜在问题。虽然没有发现直接的多链风险，但该项目的未来路线图包括构建一个生态系统，在该生态系统中，可以在当前基础设施之上部署额外的链。

在这种情况下，在 senderHash 函数加入被恢复的钱包地址和唯一的 nonce 可以作为一种预防措施，以避免可能在共享辅助地址的账户之间造成重放攻击。同样值得注意的是，当前的 salt 推导过程可能被认为是薄弱的, 应该相应地加强。最后，代码库可以通过扩展当前的测试套件来覆盖更多的边界情况，包括 zk-proof 验证流程，从而受益。

高危

ERC1271Handler 中潜在的签名重放攻击

isValidSignature 函数通常用于验证不需要外部账户发起调用的场景中的签名。它确保合约允许基于所提供输入的检查来执行某些逻辑。

在 ERC1271Handler 合约的 isValidSignature 函数 中，如果调用 isValidSignature 的合约没有完全验证所提供的数据或哈希，则不充分的检查可能允许几乎任何操作。对于 EOA 签名（65 字节签名），对哈希的构造方式没有任何限制，该函数仅确认签名的有效性及其与 k1owner 的关联。攻击者可以重用历史交易哈希和签名，并代表账户执行操作。此外，EIP712 逻辑已在备选验证方法中删除，允许跨链攻击和在不同账户之间重用签名。以前，_hashTypedDataV4 在哈希中包含了链 ID 和验证器合约，从而防止了这种滥用。

考虑实施 ERC7739 中描述的方法，该方法提出了一种防御性哈希方案，专门用于解决 ERC1271 签名重放漏洞，尤其是在由同一 EOA 管理的多个智能账户之间。此方法采用嵌套的 EIP-712 类型结构来保持可读性，同时有效地防止签名重放。此外，考虑保护备选验证方法（例如使用验证器的方法）免受签名重放攻击。

更新： 已在 pull request #439 和 commit 19747f1c 中解决。该修复利用了来自 Solady 库的 ERC1271.sol 合约。我们的团队没有对该依赖项进行独立的安全审查，而是依赖于 GitHub 仓库中提供的 审计报告。但是，对合约的集成进行了彻底审查。Matter Labs 团队表示：

我们实施了 ERC7739 以防止签名重放攻击，并确保签名内容在签名时对用户完全可见。

中危

_compactKeys 函数中不正确的密钥排序导致潜在的覆盖新密钥

OidcKeyRegistry 合约的 _compactKeys 函数 管理环状结构中的密钥列表，在删除一个元素后重新排列元素以保持紧凑性和连续性。但是，它处理排序过程的方式是从当前索引指针开始，而不是重置为零，这会导致意外的密钥重新排列和潜在的数据完整性问题。

例如，如果密钥数组为 [0, key1, key2, key3, key4, 0, 0, 0]，当前 keyIndexes 指针位于 key4，并且删除了 key3，则该函数从该指针开始重新排序密钥，生成 [key4, key1, key2, 0, 0, 0, 0, 0]。在此操作之后，插入指针错误地移动到索引 2（key2），导致下一个新密钥插入到索引 3。这种错位会首先覆盖 key4 的风险，这与 FIFO 原则相矛盾，该原则规定应首先替换 key1（最旧的密钥）。因此，较新的密钥可能会过早地被覆盖，从而导致潜在的数据丢失和意外的替换。

考虑修改 _compactKeys 函数以在每次压缩后保留原始排序，而与当前插入指针的位置无关，从而确保维护 FIFO 序列并防止覆盖较新的密钥。

更新： 已在 pull request #429 的 commit 1829cf8 和 pull request #435 的 commit 8071a0ec 中解决。

_validateKeyBatch 中 RSA 模数和指数的验证不足

OidcKeyRegistry 合约的 _validateKeyBatch 函数 使用 _hasNonZeroExponent 和 _validateModulus 分别验证指数和模数。_validateModulus 函数负责验证加密操作中的 RSA 模数，确保它们满足安全使用的基本结构要求。

_validateModulus 函数检查每个块的大小和非零值，但不强制执行最小位长度以防止因式分解攻击。它也不能确保模数是奇数，这通常是必需的，因为 RSA 模数是两个奇素数的乘积。此外，仅针对零检查指数，从而留下可能易受已知攻击的较小指数的可能性。

考虑实施一项检查以确保模数满足安全最小位长度（例如，2048 位），验证它是奇数，并强制执行至少 65537 的最小指数阈值。这些措施将有助于加强系统的加密属性，并缓解多个潜在的攻击途径。

更新： 已在 pull request #430 的 commit 1999421 中解决。Matter Labs 团队表示：

关于建议： - 模数始终被解释为 2048 位数字。合约接收一个由 17 个 121 位块组成的数组。所有这些块稍后都将由电路解释为 2048 位模数。 - 验证模数为奇数的建议已得到解决。 我们已经实施了验证，该验证考虑了 Circom 用于大数的特定格式，其中模数被编码为 17 个块，并且最低有效块位于左侧（与字节的小端顺序相同）。我们的实现确保第一个块不是 0 mod 2，从而有效地验证了模数是 RSA 安全所需的奇数。 - 关于指数阈值 (65537)，合约不再处理可变指数 - 该值已硬编码在电路中，无需运行时验证。

通过 startRecovery 流程中被篡改的 pendingPasskeyHash 进行未授权的控制

当 OidcRecoveryValidator 和 WebAuthValidator 合约都对一个账户激活时，并且账户所有者失去访问权限，则可以使用零知识 (ZK) 证明来启动恢复过程。该证明演示了对 OidcRecoveryValidator 合约中注册的关联 OIDC 身份的所有权。一旦证明和关联数据被验证，任何一方都可以调用 startRecovery 函数来开始账户恢复。

尽管 startRecovery 函数 中的大多数参数都由 ZK 电路和合约验证，但是在将 data.pendingPasskeyHash 写入账户存储 之前，没有对其进行验证。这使得启动恢复的一方（可以访问有效 ZK 证明）可以提交恶意的 pendingPasskeyHash（他们拥有相应私钥的哈希）的情况成为可能。因此，他们可以通过 WebAuthValidator 合约完成恢复，从而有效地控制该账户，即使他们不是恢复请求的预期接收者。

考虑向 ZK 证明添加一个公共信号，以将 pendingPasskeyHash 参数绑定到证明本身，并且仅在证明已验证后才存储此哈希。

更新： 已在 pull request #431 的 commit ba4967a 中解决。Matter Labs 团队表示：

为了解决这个问题，我们将 pendingPasskeyHash 包含在 JWT nonce 的内容中。之前，nonce 内容的计算方式为：

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

现在它的计算方式是：

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

这确保了用户实际上想要使用给定的密钥对，并且还使 pendingKeyHash 成为电路正在检查的数据的一部分。

低危

OidcKeyRegistry 中的重复 kid 值允许部分密钥删除和检索

在 OidcKeyRegistry 合约中，addKeys 函数 能够同时添加多个密钥。尽管此函数执行多个验证，但它缺少一项关键检查，以防止插入具有相同 kid 值的多个密钥。这种疏忽可能会导致密钥注册表中的状态不一致，尤其是在使用 deleteKey 函数时。deleteKey 函数旨在根据密钥的 kid 值删除密钥。但是，它仅删除匹配 kid 的第一次出现，而保持任何其他重复项不变。

例如，如果一个实体无意中添加了多个具有相同 kid 的密钥，并且稍后尝试通过其 kid 撤销被泄露的密钥，则只会删除一个实例（第一个匹配项）。剩余的具有相同 kid 的重复密钥将保留在注册表中，从而可能使系统容易受到攻击。此外，此问题会影响 getKey 函数，该函数旨在通过其 kid 检索密钥。与 deleteKey 函数类似，getKey 函数将仅返回与提供的 kid 匹配的第一个密钥，而忽略可能错误添加的具有相同 kid 的任何其他密钥。

为了缓解此问题并保持密钥管理流程的完整性，请考虑在 addKeys 和 addKey 函数中实施验证机制，以强制执行每个 kid 值的唯一性。

更新： 已在 pull request #432 的 commit bc71d5e 中解决。

addOidcAccount 账户注册中的抢跑交易

OidcRecoveryValidator 合约使用 digestIndex 映射将唯一标识符（oidcDigest）与用户账户相关联。此机制确保每个标识符在系统中都是唯一的。但是，当前的实现允许恶意用户执行抢跑攻击。

具体来说，攻击者可以监视内存池中待处理的注册交易。在检测到合法用户尝试注册 oidcDigest 时，攻击者可以使用相同的 oidcDigest 提交自己的交易，从而首先声明它。因此，当合法的交易最终到达 addOidcAccount 函数 时，由于 oidcDigest 已经被注册了，验证步骤会失败，导致交易回滚，并阻止合法的用户将其验证器与他们的账户关联。

请注意，鉴于没有弹性链运营商拥有公共内存池或共享排序器，这可能是一个未来的问题。

考虑实施额外的验证步骤或保护交易提交机制，以将注册请求直接绑定到发起账户。这种方法将有效地防止攻击者抢先声明 oidcDigest 标识符。

更新： 已确认，未解决。Matter Labs 团队表示：

鉴于没有弹性链运营商拥有公共内存池或共享排序器，这最多是一个未来可能出现的问题。设计的基本原理在于避免要求用户在账户恢复期间输入其地址。将来，我们可能会考虑删除 OIDC 与 Google 账户之间的一对一限制，这将大大复杂化 UX，但会防止此类抢跑。

OIDCKeys 的内置 Getter 不返回 n

OIDCKeys 变量 使用 public 可见性修饰符声明，导致 Solidity 编译器自动生成一个 public getter 函数。但是，此 getter 将仅返回结构的简单成员（issHash、kid 和 e），并且将省略复杂的数据结构，比如 n。发生这种情况的原因是，即使在嵌套时，为 public 状态变量自动生成的 getter 不会返回结构中的数组或映射成员。有关更多信息，请参阅 Solidity 文档。

由于合约已经实现了 getKeys getter，请考虑通过降低变量的可见性来删除自动 getter（不检索所有数据）。

更新： 已在 pull request #433 的 commit cd57ff7 中解决。

转移期间潜在的所有权丢失

OidcKeyRegistry 合约 实施了一个单步所有权转移流程，如果新的所有者地址不正确或无法访问，可能会导致意外的所有权丢失。此模式没有提供从这种场景中恢复的保障，并且转移中的任何错误都可能导致永久性地失去管理控制权。

问题在于所有权转移期间缺少确认步骤。在当前的实现中，一旦调用转移函数，所有权会立即被重新分配，而不需要新的所有者接受该角色。这增加了执行期间出现错误的风险，尤其是在指定了不正确或未准备好的地址时。

考虑使用来自 OpenZeppelin 库的 Ownable2StepUpgradeable 合约，该合约引入了两步转移模式。此方法要求新的所有者明确接受所有权，从而降低了意外失去控制的风险并提高了运营安全性。

更新： 已在 pull request #422 的 commit 77036be 中解决。

OidcRecoveryValidator 中不灵活的恢复流程终止

当前实现的 OidcRecoveryValidator 合约缺少一种方法来停止已启动的恢复流程，而无需完全终止账户与其发行者的链接。一旦恢复流程 启动，如果未执行触发 validateTransaction 函数的后续交易，则 中止恢复流程 的唯一方法是调用 deleteOidcAccount 函数。

但是，deleteOidcAccount 函数旨在完全断开账户与其发行者之间的连接。使用它来停止正在进行的恢复流程会因此消除将来恢复的可能性，除非再次调用 addOidcAccount 函数 以重新建立链接。这种二元选择迫使用户在保留潜在的易受攻击的恢复选项或完全失去其账户的恢复功能之间做出决定，这对用户来说安全性和便利性都不理想。

为了解决此限制，请考虑引入一项功能，允许用户有选择地撤销或使正在进行的恢复流程无效，而无需消除整个账户和发行者连接。此增强功能将大大提高恢复流程的灵活性和安全性，为用户提供对其恢复选项的更好控制。

更新： 已在 pull request #441 的 commit [d52ddf9](https://github.com更新： 已在提交 21d16e5 的 pull request #423 中解决。Matter Labs 团队声明：_

我们 实现了恢复过程发起和执行之间 10 分钟的时间验证窗口。这个时长被认为是足够的，因为最耗时的操作——证明生成——发生在调用 startRecovery 函数之前。通过强制执行此时间限制，我们确保延迟恢复无法在预期窗口之外执行。

OIDC 账户更新时未释放旧的 digestIndex

OidcRecoveryValidator 合约 使用 一个 digestIndex 来确保与用户账户关联的 OIDC (OpenID Connect) 摘要的唯一性。每次用户通过 addOidcAccount 函数 添加或更新他们的 OIDC 账户时，都会存储一个代表 OIDC 数据的摘要，以 防止重用。这种机制确保没有两个账户可以注册相同的 OIDC 身份，从而维护整个恢复过程的完整性。

然而，当用户更新他们的 OIDC 账户时，先前分配的 digestIndex 值 不会被清除。因此，即使旧的 OIDC 摘要不再使用，它仍然以用户的账户被保留。如果用户稍后尝试恢复先前有效的 OIDC 摘要，例如，恢复到更早的身份，他们将被 唯一性检查 阻止，该检查仍然将旧摘要视为已被占用，尽管它最初属于同一用户。

考虑在账户更新期间分配新的 digestIndex 值之前，显式清除用户的旧 digestIndex 值。这将允许合法地重用先前持有的摘要，并确保摘要管理的一致性。

更新： 已在提交 d7a71ca 的 pull request #424 中解决。

addOidcAccount 函数始终返回 false

在 addOidcAccount 函数 中，返回的布尔值指示是新添加了账户还是仅仅更新了账户。该函数通过检查存储的账户数据中 oidcDigest 字段的长度来 检查 当前状态，然后通过 OidcAccountUpdated 事件发出并由函数返回。根据 文档，如果添加了新密钥，该函数应返回 true，如果更新了现有密钥，则返回 false。

但是，当前实现的逻辑误解了如何计算 bytes32 类型的长度。bytes32 值的长度始终为 32 字节，无论内容如何，这意味着 accountData[msg.sender].oidcDigest.length == 0 条件将始终评估为 false。因此，返回的值和发出的事件将错误地指示账户始终被更新而不是新创建的，这可能会导致混淆或对账户状态的误解。值得一提的是，正确的检查 正在 oidcDataForAddress 函数中执行。

为了能够检测何时添加了新密钥并提高代码一致性，请考虑将 oidcDigest 的值与零进行比较（例如，oidcDigest == bytes32(0)）是否可以更准确地反映账户是否为新账户，而不是依赖于其长度。

更新： 已在提交 d7a71ca 的 pull request #424 中解决。Matter Labs 团队声明：

此 问题没有自己的 PR，因为它已作为 L-07 的一部分解决。

恢复过程中账户恢复数据的不一致

当 OpenID Connect (OIDC) 标识符链接到账户时，addOidcAccount 函数 仅更新 accountData 映射中的三个特定参数和 digestIndex 映射中相关账户的一个参数。这种选择性更新过程忽略了 readyToRecover、pendingPasskeyHash 和 recoverNonce 字段，使它们保持默认值或保留其先前状态。这种行为会产生不一致的风险，因为过时的数据可能会导致误导性行为或滥用过时的恢复状态。

核心问题出现在 恢复过程启动 时，并验证 一组特定的参数。但是，在发生 validateTransaction 调用之前，这些参数可能会被账户后续调用 addOidcAccount 函数覆盖。此操作更新上述参数，而没有完全重置 OidcData 变量，无意中使 readyToRecover 标志保持活动状态，并且 pendingPasskeyHash 变量设置为先前会话的值。当账户使用特定的 publicKey 启动恢复，并且在通过另一种恢复方法获得访问权限后，用户决定通过调用 addOidcAccount 函数来更新 accountData 时，这种情况会变得有问题，他们没有意识到在使用 OidcRecoveryValidator 合约作为验证器时，仍然可以设置原始 publicKey。

意识到这种情况需要从账户本身执行这些函数，表明以前已通过另一种方法授予了访问权限，并且用户需要主动参与才能使其发生。尽管如此，为了最大限度地降低与意外恢复流程相关的风险，并提高恢复过程的完整性，特别是不要混合来自 2 个不同恢复流程的数据，请考虑实施机制，每当添加 OIDC 标识符或启动恢复过程时，重置或适当更新 accountData 映射中的所有相关字段。

更新： 已在提交 aa78035 的 pull request #425 中解决。

恢复过程中验证不足可能导致浪费恢复尝试

OidcRecoveryValidator 合约引入了一种账户恢复机制，最终将新的 publicKey 添加到 WebAuthValidator 合约中。最初，账户 链接到 oidcDigest，并且在需要恢复时，会启动一个 提交过程，该过程验证针对 oidcDigest 的证明。如果验证成功，任何参与者都可以执行交易以将新的 publicKey 合并到 WebAuthValidator 中。

但是，当 credentialId 和 originDomain 参数不符合 WebAuthValidator 合约中的某些要求时，就会出现问题，在这种情况下，调用 不会恢复，而是会返回 false 输出。在这种情况下，无论 publicKey 是否包含在内，该调用都将被视为成功而不会恢复。因此，恢复尝试将被消耗掉，并且需要新的恢复。

此缺陷不仅会因参数不匹配而导致不必要地消耗恢复尝试的风险，还会暴露以下问题：知道用户 publicKey 的用户可以通过提交带有任意不正确的参数以及 publicKey 的交易来故意消耗恢复尝试，这将无法通过 WebAuthValidator 合约的要求。

为了降低浪费恢复尝试的风险并防止潜在的滥用，请考虑将 credentialId 和 originDomain 参数绑定到 zkProof 中，并将此绑定存储在合约中以供后续在 validateTransaction 函数中进行验证。此更改可确保这些参数在 validateTransaction 调用期间保持不变，从而防止未经授权或意外地消耗恢复尝试。

更新： 已在提交 35ff79c 的 pull request #426 中解决。

addOidcAccount 中 iss 参数长度验证不足

addOidcAccount 函数包含一项检查，以确保 iss（颁发者）参数 不为空。此值稍后将在零知识电路中使用，其中根据 模板参数定义 和提供的 常量值，其长度预计小于 32 个字符。虽然下限通过现有的验证来强制执行，但上限（对于零知识证明验证至关重要）没有强制执行。

因此，可以存储长于预期最大值的值，即使它们对于零知识电路中的证明生成或验证无效。这会在链上可以存储的内容与可以成功进行链下证明的内容之间产生差异。

除了现有的非零长度检查之外，请考虑强制执行 iss 参数长度的上限，以确保它严格小于 32 个字符。

更新： 已在提交 22d3b4f 的 pull request #427 中解决。

OidcRecoveryValidator 不遵循 ERC-1271 流程

OidcRecoveryValidator 合约的 validateSignature 函数 在被调用时会恢复。发生这种情况是因为验证器仅应为账户提供恢复机制，而不是验证任何其他类型的交易。但是，其当前实现通过在调用时恢复而不是返回布尔值来偏离预期的 ERC-1271 流程。此行为与预期流程形成对比，在预期流程中，validateSignature 函数应通过返回 false 来表示负面结果，以使 ERC1271Handler 合约不返回 魔法值。

鉴于系统中的类似验证器合约在类似条件下 返回 false，使 OidcRecoveryValidator 合约的行为与此模式保持一致将增强签名验证过程的一致性和可预测性。因此，请考虑修改 validateSignature 函数以返回 false 而不是恢复，确保其符合已建立的 ERC-1271 流程。

更新： 已在提交 5141a07 的 pull request #428 中解决。

说明和补充信息

使用 uint8 作为常量比 uint256 更昂贵

在 OidcKeyRegistry 和 OidcRecoveryValidator 中，uint8 都被用于常量（例如，用于 MAX_KEYS 常量）。但是，这会在部署期间引入不必要的开销，因为需要额外的范围检查，从而使部署更加昂贵。例如，在 OidcKeyRegistry 中使用 uint8 的成本约为 28,267,888 gas，而 uint256 的成本约为 28,175,109，节省了约 92,779 gas。

由于这些值是常量并且经过手动验证以确保在预期范围内，因此切换到 uint256 没有增加任何风险。请考虑将 uintN 常量更新为 uint256 以优化部署成本。

更新： 已在提交 d0c788e 的 pull request #399 中解决。

OidcKeyRegistry.sol 中的许可不一致

OidcKeyRegistry.sol 文件 使用 UNLICENSED 许可，而代码库的其余部分要么根据 GPL-3.0 许可，要么根据 MIT 许可。这种不一致可能会在使用和分发方面产生不确定性。

请考虑更新 OidcKeyRegistry.sol 中的许可，以与代码库的其余部分（例如 GPL-3.0 或 MIT）保持一致。

更新： 已在提交 894321c 的 pull request #400 中解决。

_validateModulus 中存在冗余局部变量

在 _validateModulus 函数 中，uint256 limit 变量计算为 (1 << 121) - 1。目前，limit 在每次调用该函数时都会在该函数内部计算，并且由于该值在整个函数执行过程中保持不变，因此重复重新计算会消耗不必要的 gas。

请考虑将 limit 定义为 _validateModulus 函数外部的常量，以避免不必要的重新计算并减少 gas 使用量。

更新： 已在提交 7e36375 的 pull request #401 中解决。

使用 uint 代替 uint256

虽然 uint 是 uint256 的别名，但为了清晰起见和一致性，建议显式使用 uint256。

在 OidcRecoveryValidator.sol 中，发现了多个使用 uint 代替 uint256 的实例：

  • 第 86 行中的 uint
  • 第 87 行中的 uint
  • 第 88 行中的 uint
  • 第 217 行中的 uint
  • 第 221 行中的 uint
  • 第 226 行中的 uint
  • 第 334 行中的 uint

为了明确起见，请考虑将所有 uint 实例替换为 uint256。

更新： 已在提交 13312f8 的 pull request #402 中解决。

代码中的 Magic Number

在整个代码库中，发现了多个带有无法解释含义的字面量实例。

  • OidcRecoveryValidator.sol 中的 8 的 字面量
  • OidcRecoveryValidator.sol 中的 248 的 字面量
  • OidcRecoveryValidator.sol 中的 32 字面量
  • OidcRecoveryValidator.sol 中的 32 字面量
  • OidcRecoveryValidator.sol 中的 8 字面量
  • OidcRecoveryValidator.sol 中的 8 字面量

请考虑定义和使用 constant 变量，而不是使用字面量或正确记录字面量值，以提高代码库的可读性和可维护性。

更新： 已在提交 acd7822 的 pull request #403 中解决。

使用自定义错误

自 Solidity 版本 0.8.4 以来，自定义错误提供了一种更简洁且成本效益更高的方式来向用户解释操作失败的原因。

在 OidcRecoveryValidator.sol 中，发现了 require 消息的多个实例：

为了简洁、节省 gas 以及与代码库的其余部分保持一致，这些部分使用了 自定义错误，请考虑将 require 和 revert 消息替换为自定义错误。

更新： 已在提交 2e27794 的 pull request #404 中解决。

startRecovery 中缺少事件发出

调用 OidcRecoveryValidator 合约的 startRecovery 函数 时，它不会触发任何事件。

请考虑发出一个事件来反映恢复已开始，这将提高代码库的清晰度并简化链下监控。

_更新： 已在提交 a375b7a 的 [pull request #405OidcRecoveryValidator 合约 缺少一个与其可用实现函数相对应的接口，例如 addOidcAccount 和 deleteOidcAccount。此外，像事件、错误和结构体这样的定义可以包含在这个接口中，以改善开发者体验和协议互操作性。这项更改将简化合约的结构，并增强在不同开发环境中的可用性。将这些定义移到接口中，还可以简化与其他合约的交互、事件解码、错误处理以及传递结构体作为输入或输出参数。

考虑创建一个直接包含上述定义的接口。这种方法符合智能合约设计的最佳实践，并通过提供一种更清晰、更直观的方式与合约的功能进行交互来增强开发者体验。

加粗更新：已在 pull request #408 中的 commit 02e0df0 中解决。

重构与改进机会

在整个代码库中，发现了多个代码改进的机会：

  • 在 OidcRecoveryValidator 合约 中，两个合约地址（keyRegistry 和 verifier）存储为原始的 address 类型，只有在访问时才转换为各自的合约类型。这些类型转换操作在整个合约中只执行一次。 可以考虑更新构造函数以包含类型转换操作，并相应地调整存储变量的类型，而不是在使用时将 address 转换为合约。 这种方法不仅可以通过消除使用时进行类型转换的需要来简化合约代码，还可以潜在地降低与类型转换操作相关的 gas 成本。
  • 当摘要已被注册时，OidcDigestAlreadyRegisteredInAnotherAccount 自定义错误 会返回 oidcDigest 参数。 然而， oidcDigest 由调用者提供，因此已经是已知的，使其包含变得多余。 如果错误包含已注册摘要的帐户地址，则会更有用。 考虑替换这样的参数。
  • startRecovery 函数中的 for 循环 迭代 key.n.length 次，辅助变量 index 在每次迭代中都会递增。 但是，在循环完成后，index 将等于 key.n 中的元素数。 考虑不要在循环内递增 index 变量。 而是直接将 key.n 的长度分配给 index。
  • for 循环中每个 Key.n 的类型转换是不必要的，因为数组中的每个 n 元素已经是一个 uint256 变量。 考虑删除此类类型转换。
  • startRecovery 函数的当前实现在使用多个按位移位操作将 senderHash拆分为两个值。可以通过在调用 _reverse 函数后应用单个按位掩码，将最高有效字节（采用小端格式）设置为零来优化它。 通过将 senderHash 截断为 uint8 然后将其转换回 uint256，可以简化 publicInputs 的最后一个元素。 与多次移位相比，这种方法减少了堆栈操作，减少了推送到堆栈上的常量数量，并实现了更好的 gas 效率。
  • _reverse 函数的当前实现通过使用额外的中间变量和每次迭代的多个算术运算来反转字节，每次调用消耗大约 29k gas。 这可以通过用按位累积方法替换它来优化，其中通过重复将输出向左移动一个字节并将它与输入的最低有效字节组合来构建结果。 优化后的版本将 gas 使用量减少到每次调用大约 3.9k。

为了提高代码库的可读性和 gas 效率，请考虑应用上述建议。

加粗更新：已在 pull request #410 中的 commit 26529e1 中解决。

onInstall 和 onUninstall 中不必要的 override 关键字

在 OidcRecoveryValidator 合约中，onInstall 和 onUninstall 函数用 override 关键字标记。 这种用法表明这些函数旨在覆盖父合约中的实现。 但是，这些函数实现来自接口的抽象函数声明，而不是覆盖来自祖先合约的具体实现。 在 Solidity 中，override 关键字用于指示函数正在覆盖来自基合约的函数。 当函数只是实现接口的方法时，不需要使用 override。 在这种情况下，override 关键字的使用可能会导致关于这些函数的继承结构的混淆，因为它指向一种不存在的继承关系。

为了增强代码的清晰度并准确地反映继承和实现结构，请考虑从所有不需要它的地方的 onInstall 和 onUninstall 函数声明中删除 override 关键字。

加粗更新：已在 pull request #411 中的 commit 5b48556 中解决。

OidcRecoveryValidator 中对 hashIssuer 的冗余外部调用

OidcKeyRegistry 合约的 hashIssuer 函数 在 OidcRecoveryValidator 合约的 startRecovery 函数中被调用。 此函数对单个输入参数执行简单的哈希运算。 由于逻辑最少且不依赖于 OidcKeyRegistry 合约的任何内部状态，因此外部调用会引入不必要的复杂性和潜在的 gas 开销。

考虑将 hashIssuer 函数直接移动到 OidcRecoveryValidator 合约中，或者在合约中复制它的逻辑以避免外部调用。

加粗更新：已确认，未解决。 Matter Labs 团队声明：

OidcKeyRegistry 中的 hashIssuer 方法确保了发行者的一致哈希生成。 虽然复制此逻辑会产生最小的 gas 节省，但它会创建冗余代码，如果将来发生更改，可能需要同步。

考虑到弹性链生态系统中的低 gas 成本，我们认为维护外部方法调用优于复制逻辑。

合约和电路之间 senderHash 表示中的字节序不匹配

startRecovery 函数将 32 字节的 senderHash 拆分 为两个 31 字节的字段，以适应电路的输入大小。 第一部分是通过右移哈希生成的，这会将移位插入的零移动到最低有效端，然后丢弃该零，从而产生一个 小端 序列 B1 … B31（其中 B0 已经提取）。 第二部分是通过隔离最低有效字节 (B0) 而不反转获得的，使其在字的最低有效字节中保持不变。 因此，相同的逻辑值部分以小端编码，部分以发送给电路之前的原始顺序编码。

由于电路似乎期望整个 32 字节值的统一字节序，因此这种混合表示可能会导致电路重建不正确的 senderHash，从而导致证明失败或其他意外的验证结果。

考虑以电路期望的单个字节序对两部分进行编码——例如，在拆分之前反转整个 32 字节的哈希，或者省略两部分的反转——以便合约和电路使用相同的字节序，并且可以明确地重建哈希。

加粗更新：已在 pull request #436 中的 commit d0cf420 中解决。 Matter Labs 团队声明：

我们更改了 txHash 的序列化，只是简单地将前 31 个字节放在第一个元素中（没有任何类型的反转），并将最后一个字节放在第二个元素中（也没有任何恢复）。

我们还在电路输入生成代码中进行了更改，以与此保持一致。 这些更改也在电路的文档中考虑了。

这也允许我们删除仅在此处使用的 _reverse 内部函数。

关于 OIDC 提供者公钥格式的误导性注释

在 OidcRecoveryValidator 合约中，startRecovery 函数包含一个注释，旨在描述 publicInputs 数组的结构。 具体来说，该注释目前声明：“前 CIRCOM_BIGINT_CHUNKS 元素是 OIDC 提供者公钥。” 这可能会误导那些认为 RSA 公钥包括模数和指数的读者，因为根据标准加密定义，情况就是如此。

然而，该实现仅在这些元素中包括模数。 与当前注释可能推断出的相反，指数不属于此结构。 这种不一致可能会给审查代码的开发人员或审计人员造成混淆，特别是那些在不详细检查逻辑的情况下依靠注释来理解数据布局的人员。

考虑更新注释以准确反映输入的内容，例如：“前 CIRCOM_BIGINT_CHUNKS 元素是 OIDC 提供者模数。” 这将提高代码的可读性，并有助于确保关于密钥结构的正确假设。

加粗更新：已在 pull request #442 中的 commit 5d8d557 中解决。

OidcRecoveryValidator 中来自 VerifierCaller 的冗余继承

OidcRecoveryValidator 合约继承自 VerifierCaller 合约。 然而，在 OidcRecoveryValidator 合约的当前实现中，没有使用 VerifierCaller 合约提供的任何函数或特性。

这导致了不必要的继承，这可能会增加合约大小并使代码库复杂化，而不会提供任何功能上的好处。

考虑在 OidcRecoveryValidator 合约中删除 VerifierCaller 合约的继承，以简化合约并减少潜在的维护开销。

加粗更新：已在 pull request #408 中的 commit 02e0df0f 中解决。 Matter Labs 团队声明：

这个问题已作为 N-10 修复 的一部分解决。

OidcRecoveryValidator 中 addOidcAccount 返回值的不一致处理

当前的 onInstall 实现 忽略了 addOidcAccount 函数的返回值。 这可能会使读者感到困惑，因为无法立即清楚地知道返回值是否被故意忽略，或者是否是一个疏忽。

考虑记录忽略 addOidcAccount 函数返回值的原因。

加粗更新：已在 pull request #440 中的 commit 0e6415e 中解决。 Matter Labs 团队声明：

返回值已被删除，因为它没有在代码库中的任何地方使用。

结论

审查的代码引入了一种基于 OIDC 的帐户恢复机制，以增强现有的 SsoAccount 设置，并更新 ERC1271Handler 以简化协议。 这种集成利用零知识证明来实现安全和灵活的恢复过程。

总的来说，OpenID Connect 恢复的添加为用户提供了一条通过熟悉的外部身份提供者恢复访问权限的清晰途径。 创建用于管理 OP 密钥的 OidcKeyRegistry 和用于促进基于零知识的控制证明的 OidcRecoveryValidator 有效地扩展了帐户的弹性。 将这些组件与现有的 WebAuthValidator 集成可确保验证和最终恢复的一致过程。

非常感谢 Matter Labs 团队在整个审计过程中反应迅速且配合良好。

  • 原文链接： blog.openzeppelin.com/ss...
  • 登链社区 AI 助手，为大家转译优秀英文文章，如有翻译不通的地方，还请包涵～
点赞 0
收藏 0
分享
本文参与登链社区写作激励计划 ，好文好收益，欢迎正在阅读的你也加入。

0 条评论

请先 登录 后评论
OpenZeppelin
OpenZeppelin
江湖只有他的大名，没有他的介绍。

OpenZeppelin

如果觉得我的文章对您有用，请随意打赏。你的支持将鼓励我继续创作！