Oxorio SAMM 模块审计

该文档是对 Oxorio 团队开发的 Safe Anonymous Mail Module (SAMM) 的代码审计报告,SAMM 旨在为 Safe 多重签名钱包提供匿名交易发起和批准功能。审计发现了多个需要改进的地方,包括安全性和匿名性问题,例如缺少 DKIM 公钥与参与者域名的匹配检查、部分去匿名化、以及潜在的DoS攻击。

目录

概要

TypeZKTimelineFrom 2024-12-09To 2025-01-03LanguagesSolidity, NoirTotal Issues22 (0 resolved)Critical Severity Issues0 (0 resolved)High Severity Issues3 (0 resolved)Medium Severity Issues1 (0 resolved)Low Severity Issues11 (0 resolved)Notes & Additional Information7 (0 resolved)

范围

我们审计了oxor-io/samm-circuits存储库,提交哈希为240b94a,以及oxor-io/samm-contracts存储库,提交哈希为c2d255d

以下文件在审计范围内:

 samm-circuits
└── lib
    ├── src
    │   ├── lib.nr
    │   ├── merkle_tree.nr
    │   └── utils.nr
    └──  builds
        ├── samm_1024
        │   └── src
        │       └── main.nr
        └── samm_2048
            └── src
                └── main.nr

以及

 samm-contracts
└── src
    ├── SAMM.sol
    ├── ModuleGuard.sol
    ├──  libraries
    │   ├── Enum.sol
    │   └── PubSignalsConstructor.sol
    └── utils
        └── DKIMRegistry.sol

系统概述

Oxorio的安全匿名邮件模块 (SAMM) 是 Safe 多签钱包的一个隐私技术模块。它可以通过电子邮件实现匿名交易的发起和批准,确保参与者的身份在整个过程中对外部各方保持机密。它利用零知识 (ZK) 证明,采用自定义Noir电路和从Barretenberg证明库自动生成的验证器。

使用工作流程

SAMM 与 Safe 多签钱包集成,允许用户通过电子邮件发起和批准交易。 工作流程如下:

  1. 交易发起:SAMM 成员向中继器的地址发送一封电子邮件,其中包含交易详细信息,例如接收者 (to)、数据、价值和截止日期。
  2. 批准:其他成员通过发送以 msgHash 作为主题的电子邮件来批准交易,每封电子邮件的标头中都包含 DKIM 签名。
  3. 中继器处理:中继器存储批准数据,并在达到投票阈值后生成 ZK 证明。
  4. 链上执行:中继器将交易数据和证明提交给 SAMM 智能合约,该合约验证证明并在满足阈值时执行交易。 此过程确保所有参与者都可以管理交易,而无需向中继器基础设施之外的实体透露其身份。

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

在围绕 SAMM 模块的整个系统中,我们确定了以下角色和必要的信任假设:

  • EOA 所有者:对 Safe 多签钱包拥有完全控制权的 EOA, 包括管理 SAMM 权限。
  • SAMM 模块:持有受限权限,仅限于特定的交易允许列表。 允许列表中的交易还受到津贴值的限制。 SAMM 无法执行管理操作(例如,它无法更改 Safe 多签钱包的所有者)。
  • 中继器:负责处理批准电子邮件、生成 ZK 证明以及将交易提交给 SAMM 合约。 SAMM 成员的交易信息及其完整的电子邮件地址将透露给中继器。
  • DKIM 注册表合约:假定 DKIM 注册表受到安全管理,并具有关于有效和已撤销的公钥哈希的准确信息。 我们假设 DKIM 注册表管理员不会滥用撤销权(例如,审查在内存池中观察到的批准交易)。
  • 发送邮件服务器:SAMM 成员的邮件服务器负责 DKIM 签名。 我们假设它从不欺骗电子邮件,而是在发送带有 DKIM 签名的电子邮件之前安全地验证每个发件人。 此外,假定邮件服务器配置有宽松的标头规范化算法,因为 Noir 电路代码依赖于小写的电子邮件字段(例如,from:to:subject:,以及已删除的空格)。

附加信息

有关更详细的技术要求和规范,请参阅 SAMM 技术要求

高危

DKIM签名中无约束的域

该电路确保标头的签名与指定的公钥匹配,而SAMM合约验证相应的公钥哈希是否包含在DKIM注册表中。 但是,电路和合约均不能确保特定的公钥与参与者的电子邮件域相对应。

这意味着恶意的DKIM签名者可以伪造来自使用不同电子邮件域的参与者的消息。 为了完成伪造的证明,他们将需要参与者的secret,这可能需要与中继器勾结。 尽管如此,这可能会大大降低多签阈值。

考虑验证 DKIM 签名是否与电子邮件发件人的域匹配。

更新: Oxorio 团队在 PR #3 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

有效的邮件可能无法证明

check_from_field 函数[查找 < 字符](https://github.com/oxor-io/samm-circuits/blob/240b94a36effc0eadc8620766ab056d05e128483/lib/src/utils。 nr#L27) 并根据是否找到它以不同的方式解析电子邮件地址。 check_to_field函数遵循相同的模式。 但是,搜索函数在字段的开头引入了一个偏移量,然后在搜索指针中再次使用相同的偏移量。 偏移量旨在跳过字段的名称和随后的 : 字符,但由于它被重复计算,因此该函数实际上跳过了字段内容的开头。 如果这导致搜索跳过 < 字符,则根据调用方式,这要么会因为地址与缓冲区中电子邮件地址的位置不匹配而失败,要么是因为该字段与预期的电子邮件地址不匹配。 无论哪种情况,这都意味着某些有效的电子邮件无法被证实。

考虑更正偏移量,以避免重复计算字段名称跳过。 在 ZKEmail.nr 存储库 修复此问题之前,请考虑在 constrain_header_field_detect_last_angle_bracket 函数的本地副本中应用此补丁。

更新: 已知悉,未解决。 Oxorio 团队表示:

我们不想 Fork 并修改该库; 我们将等待官方修复并相应地更新版本。

抢跑可能导致多签钱包DoS

SAMM 合约的 executeTransactionexecuteTransactionReturnData 函数当前未验证调用者。 因此,任何人都可以在具有所需数量的正确证明的情况下调用它们。 这允许外部方观察到中继器在内存池中的交易,并以不同的 Gas 量重新提交它。 可以选择此数量,以便在执行所需操作时,转发的 Gas 不足以完成操作,但是 EIP-150 保留金额 足以完成整个交易,这将增加SAMM的 nonce。 这将使中继器的交易无效,从而有效地丢弃了生成证明所需的链下协调和计算。

考虑实施以下一些对策的组合,以对抗上面概述的方案:

  • 将中继器的地址集成作为公共信号,并在链上进行验证。
  • 显式检查调用的成功条件。
  • 将所需的 Gas 量集成作为公共信号,并在链上进行验证

更新: Oxorio 团队在 PR #3 中实施了修复。 中继器的地址已作为电路中的公共输入添加。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

中危

参与者被部分去匿名化

对于每个有效的 ZK 证明,调用者必须指定域和公钥哈希。 这会部分地取消每个参与者的匿名性,因为他们的电子邮件域会被泄露。 这意味着相关的匿名集是共享相同电子邮件域的其他参与者。

为了实现通用匿名性,请考虑保持域的私有性。 这可能涉及将公钥哈希电路输入替换为所有有效的(和未撤销的)公钥的承诺或 Merkle 根。

更新: 已知悉,未解决。 Oxorio 团队表示:

由于此问题仅影响匿名性,并且需要对代码库进行重大更改,因此我们决定在此开发迭代中不修复它。

低危

不充分的设置验证可能导致SAMM无法使用

SAMM 合约是使用代理工厂部署的,其 setup 函数 负责使用特定的输入数据初始化实例。 这些输入之一,relayer 参数,代表中继器的电子邮件地址。 合约假定此电子邮件地址不会超过 124 个字符,如 setRelayer 函数 中所强制执行的那样。 但是,此假设在 setup 函数中未明确验证,从而为意外行为留下了空间。 在 setup 过程中,仅检查relayer 参数以确保它不为空,并且不存在限制其最大长度的验证。 这在getPubSignals 函数中变得有问题,该函数构造了一个公共信号的固定长度数组,大小为 172。 电子邮件地址从索引 1 开始写入,但如果其长度超过数组的容量,则超出范围的写入尝试会触发合约 panic 错误(代码为 0x32)。

另一个输入是 membersRoot 参数,当前可以将其设置为大于零的任何值,而无需针对 BN254 素数字段顺序进行验证。 如果此值超过字段顺序,则会阻止通过 SAMM 合约执行交易。 虽然如果相关的多签更新变量 [12],可以解决这两种状态,但缺乏输入验证会引入初始的 DoS 风险。

考虑在 setup 函数中实施检查,以确保 relayermembersRoot 参数被约束为有效的输入。

更新: Oxorio 团队在 PR #7 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

不完整的交易验证

ModuleGuarddata 缓冲区的前 4 个字节 解释为函数签名,以确定是否允许交易SAMM 合约遵循相同的模式。 在这两种情况下,如果 data 缓冲区少于四个字节,则最终的函数调用将与验证不匹配。

实际上,这意味着 SAMM 合约(以及受 ModuleGuard 保护的任何其他模块)将能够在任何有效的目标合约上执行 fallbackreceive 函数。 还有一种更模糊的可能性:该模块可以发送有效选择器的前缀(例如,前两个字节),如果手动构造而不使用标准的 Solidity 或 Vyper 编译器,这可能会对目标合约产生不同的影响。

考虑检查 data 缓冲区的长度,以便验证与最终的函数调用完全匹配。

更新: 已知悉,未解决。 Oxorio 团队表示:

我们决定在当前的开发迭代中不添加此检查。

不一致的字段大小

lib.nr 中,定义了 MAX_EMAIL_HEADER_LENGTHMAX_EMAIL_FIELD_LENGTH 常量,分别指定了最大电子邮件标头大小和最大标头字段大小。 但是,字段大小被定义为大于总标头大小,这是不一致的。

考虑减小 MAX_EMAIL_FIELD_LENGTH 的大小。

更新: Oxorio 团队在 PR #3 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

Noir代码库中的浮动编译器版本

在整个 Noir 代码库中指定的 >=0.35.0 编译器版本 允许使用各种编译器版本编译协议。 这种可变性可能导致测试期间行为不一致以及验证器合约的生成,因为不同的编译器版本可能会引入影响协议的更改或错误。

考虑在代码库中显式指定固定的编译器版本,以确保跨环境的一致编译和可预测的行为。

更新: Oxorio 团队在 PR #3 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

津贴系统忽略操作类型

_checkTxAllowance 函数 旨在验证 SAMM 合约中调用的执行。 但是,它不区分操作类型,例如 delegatecall 或常规 call。 这种缺乏特异性意味着验证过程可能与预期的限制不一致,因为不同类型的调用对合约安全性和行为有不同的影响。

考虑修改 _checkTxAllowance 函数,以允许用户为每个 txId 指定允许的操作类型(例如,calldelegatecall)。

更新: Oxorio 团队在 PR #7 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

更新后的DKIM注册表可能使用已撤销的公钥哈希

SAMM 合约中,setDKIMRegistry 函数 允许通过 Safe 多签钱包更新 DKIM 注册表。 此函数更改用于 DKIM 相关操作的注册表。 但是,如果更新了 DKIM 注册表,则存在先前 DKIM 注册表中已撤销的公钥哈希仍可能被利用的风险。

考虑记录此方案,以确保用户了解验证新 DKIM 注册表合约的重要性,特别是关于域的任何已取消的公钥哈希。

更新: 已知悉,未解决。

使用未经审计的Base64库

该代码库使用了一个Base64,该库似乎未经审计,因此可能会引入潜在的风险。

考虑使用经过审计的解决方案,例如 OpenZeppelin 的 Base64

更新: 已知悉,未解决。 Oxorio 团队表示:

我们决定在当前的开发迭代中不修改该库。

在设置期间添加的允许交易未发出事件

SAMM 合约的 setup 函数 中,具有可以列入白名单的合约地址和函数签名的功能,SAMM 模块可以代表 Safe 多签钱包与之交互。 与 setTxAllowed 函数 不同,setup 函数不为添加的 allowedTxs 发出事件,后者会正确记录对允许交易的更新。 这可能会导致链上状态和链下组件之间存在差异。

考虑将 setTxAllowed 函数逻辑移动到发出 TxAllowanceChanged 事件的 internal 函数中,该函数可以在 setup 函数中使用。

更新: Oxorio 团队在 PR #7 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

重复的交易ID可能在设置期间相互覆盖

SAMM 合约的 setup 函数 中,txAllowances 数组不对其元素强制执行唯一性。 因此,重复的 txId 条目可能导致津贴被覆盖,从而可能导致意外的协议状态。

考虑检查 s_allowedTxs.add() 的布尔返回值以确保唯一值。 此外,考虑为设置事务津贴逻辑实施一个 internal 函数,该函数在设置和 setTxAllowed 函数 中重复使用。

更新: Oxorio 团队在 PR #7 中实施了修复。 但是,此修复尚未经过 OpenZeppelin 团队的评估。

具有误导性和不完整的文档

在整个 SAMM-contracts 代码库中,发现了以下具有误导性和不完整注释的实例:

  • SAMM.sol 中:

    • txAllowances 的文档仅涉及 address/to 字段和 selector 字段。 但是,相应的代码 使用了额外的 amount 字段。
    • executeTransactionexecuteTransactionReturnData 函数除了证明阈值之外,还涉及未定义的 hash approval amount 概念。 注释的这一部分似乎与代码不对应。
    • 此外,在 executeTransaction 函数中,截止日期 [12] 已被描述为互斥的界限(在截止日期之前执行它),而它是包含的界限(可以在截止日期执行)。
  • DKIMRegistry 合约注释 描述了两种生成公钥哈希的机制。 它们都不完全匹配 Noir 代码:

    • 1024 位密钥被拆分为9 个 31 字节的块 或 248 位,而不是 242 位。
    • 2048 位密钥被拆分为 16 个块 而不是 17 个块。
    • 此外,引用的EmailVerifier.sol 文件不存在。
  • ModuleGuard 指出 isTxAllowed 映射用于 SAMM 模块。 但是,与单个 SAMM 模块一起使用与包含 module 术语相矛盾,因为只有一个模块。

此外,在 SAMM 电路代码库中,samm_2048samm_1024 电路的主入口点包含多个公共和私有输入信号以及公共输出信号。 对于每个信号,- 消息哈希被分割成 2 个 22 字节的块

考虑彻底记录决定块数的理由。

类似地,SAMM-contracts 代码库包含以下 magic number:

在上述所有使用 magic number 的实例中,考虑使用常量并充分记录它们。

更新: 已确认,未解决。Oxorio 团队表示:

我们将添加到 relayer 运营商的文档中(超出本次审计范围)。

注意事项 & 补充信息

代码简化

在整个代码库中,发现了以下代码简化的机会:

考虑应用上述建议以提高代码库的清晰度。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

代码效率低下

在整个代码库中,发现了多个代码效率低下的实例:

  • setDKIMPublicKeyHashes 函数在一个循环中调用 setDKIMPublicKeyHash。这意味着 onlyOwner modifier会在函数开始时调用,然后每个哈希调用一次。考虑将逻辑移动到一个 internal 函数中,以便可以在不调用 modifier 的情况下调用它。
  • _checkNProofs 中,一个循环检查每个 commitment 的唯一性。考虑通过要求 proofs 按照 commitment 排序提交来简化这一点。然后,只需要检查每个 commitment 是否严格大于前一个。
  • _stringEq效率低下,因为它通过每个参数 ababi.encodePacked 进行了冗余的内存扩展。相反,考虑实现 OpenZeppelin 的 String 库的 equal 函数。此外,考虑是否可以完全删除该函数,因为它似乎没有被使用。
  • SAMM 合约的存储布局效率低下:
    • s_threshold 的类型为 uint64。然而,它与 20 字节的 s_safe 打包在一起,留下了 32 位未使用。将其大小增加到 uint96 以利用所有可用空间可能会有益。
    • nonce 变量的大小为 uint256。但是,如果将其大小减少到 uint96,则可以将其与 s_dkimRegistry 地址打包在一起,这仍然允许大约 7.9⋅10287. 9\cdot 10^{28}7.9⋅1028 笔交易。假设增量为 1 并且 proof 生成需要 1 分钟,则需要足够长的时间才能达到最大值。
  • 目前,链下 relayer 的设计是接收成员的电子邮件并为每封电子邮件生成一个 proof。一旦电子邮件的阈值得到证明,就可以执行交易。因此,SAMM 合约检查是否满足阈值单独检查每个 proof。假设链上计算成本高于链下计算成本,则此设计似乎效率低下。考虑让 relayer 等待直到收到阈值数量的电子邮件并共同证明交易。这将产生一个 proof,然后可以在链上验证。
  • 当前协议设计使用 s_relayer 存储变量作为电路的公共信号。在 SAMM 合约中,此值被分割成 bytes32 数组的 1 字节元素的块,并存储在 getPubSignals 函数中的内存中。然后,此处理后的数据将传递给验证器合约。因此,这种方法会产生与解析电子邮件地址相关的成本,该电子邮件地址的长度可能达到 124 字节,内存扩展以及验证器合约中的额外计算。为了优化,请考虑使用 s_relayer 值的哈希来将内存消耗减少到单个 32 字节字,而不是最多 124 字节。然后,可以将 s_relayer 值的 preimage 作为私有信号提供给电路,以确保 proof 的正确性和一致性。
  • checkNProofs 函数中,s_dkimRegistry 存储变量在循环中使用,考虑缓存该值以节省一些读取 storage 的 gas。
  • checkModuleTransaction 函数使用 keccak256 哈希函数计算并返回 moduleTxHash,可能具有很大的参数大小。但是,此值未在后续逻辑中使用。考虑返回一个常量值来降低执行成本。
  • 通过使用调用者指定的序列对象来简化 fromto 字段以及两个电子邮件地址(在这些字段中)的标识,这些序列对象精确地指出了准确的位置。但是,subject 字段的标识使用header 搜索来查找相关字符串。考虑通过迁移到序列对象来优化约束的数量。此修订在保持技术严谨性的同时,提高了可读性和流程。

考虑实施上述建议以提高代码库的效率。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

代码冗余

在整个代码库中,发现了多个代码冗余的实例:

  • SAMM 合约中的几个函数可能只能由 Safe 账户调用。这种访问控制模式多个函数中重复出现。考虑使用 modifier 来实现此逻辑。
  • 在整个 SAMM 合约中,txId 被计算为打包编码的地址和函数选择器。考虑到编码不匹配是问题的常见来源,请考虑将此逻辑移动到专用的 private 函数中。

考虑应用上述建议以提高代码库的清晰度和可维护性。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

未使用的合约

ModuleGuard 合约似乎未使用。相反,它的功能已经在 SAMM 合约中实现。

考虑记录拥有 ModuleGuard 合约的原因,如果它是冗余的,则将其删除。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

DKIM 注册表的事件参数未编入索引

SAMM 合约的 setup 函数中,dkimRegistry 参数是 Setup 事件的一部分。但是,它未编入索引。这使得在查询区块链日志时难以有效地搜索使用特定 dkimRegistrySAMM 合约。

考虑在 Setup 事件中索引 dkimRegistry 参数。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

拼写错误

在整个代码库中,发现了多个拼写错误的实例:

  • SAMM.sol 的第 158184 行中,“one of the proof”应为“one of the proofs”。
  • SAMM.sol 的第 472 行中,“uniq”应为“unique”。

考虑更正任何拼写错误以提高代码库的可读性和清晰度。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

命名建议

在整个代码库中,发现了多个改进命名的机会:

  • DKIMRegistry 的 owner 账户被称为 _signer,但不进行任何签名。考虑将该变量命名为 _owner
  • TxAllowanceChanged 事件应命名为 TxAllowedChanged,以避免与 AllowanceChanged 事件混淆。这也适用于 IModuleGuardEvents 接口。
  • last_angle_bracket 变量名 [ 1, 2] 令人困惑。这是因为它不是最后一个尖括号,即电子邮件地址之后的右括号,而是最后一个_左_尖括号。
  • SAMM 合约中的 allowance 命名给出了关于已实现机制的错误想法。定义的 allowance 永远不会像 ERC-20 标准中那样被消耗。相反,allowance 定义了每个交易 ID 可以转移的最大值。因此,考虑将该变量命名为 maximumTxValue 或类似的名称。

考虑应用上述建议以提高代码库的清晰度和可维护性。

更新: 已确认,未解决。Oxorio 团队表示:

我们决定不在本次开发迭代中修复代码风格问题。

结论

审查的代码引入了 Oxorio 的 Safe Anonymous Mail Module (SAMM),该模块使用 Solidity 合约和 Noir 电路实现。在审查期间,我们确定了几个需要进一步开发的领域,以达到生产部署所需的成熟度。值得注意的是,该实现将受益于增强安全性和匿名性。关键问题包括缺少对应于参与者域名的 DKIM 公钥的检查、通过域名暴露进行的部分去匿名化、无法证明的电子邮件以及对交易进行 DoS 攻击的可能性。此外,还强调了与代码质量和可读性相关的问题。

我们建议采纳本报告中概述的改进措施,然后进行另一轮内部审查和外部审计。这些步骤将有助于确保系统在部署之前是安全、可靠且完全匿名的。我们非常感谢在此过程中与 Oxorio 团队的协作互动。他们在技术讨论中的响应性和以解决方案为导向的方法值得称赞。此外,随附的技术文档对于理解 SAMM 的高级架构非常宝贵。

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

0 条评论

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