Fireblocks可升级代币审计 - ERC721F

该文章对Fireblocks的可升级tokens进行了审计，主要围绕ERC721F合同中的访问控制实现及其漏洞进行了讨论。审计发现了一些高、中、低严重性的安全问题，并提供了相应的修复建议和代码改进的方向，强调了合同的可升级性及其实现中的功能特点。

目录

总结

TypeDeFi时间从 2023-11-13 到 2023-11-17
语言：Solidity
总问题：14 (解决10)
关键严重性问题：0 (解决0)
高严重性问题：1 (解决1)
中等严重性问题：1 (解决1)
低严重性问题：2 (解决1，部分解决1)
备注与附加信息：10 (解决7)

范围

我们审计了 Fireblocks 仓库的提交 e7003dfb。

在审计完成后，Fireblocks 在 pull request #1 中将许可证更新为 AGPL-3.0-or-later，提交为 6d8327b。此更新对源代码逻辑的安全性无影响。

以下合约在范围内：

 contracts
├── ERC721F.sol
└── library
    ├── AccessRegistry
    │   ├── AccessListUpgradeable.sol
    │   ├── AccessRegistrySubscriptionUpgradeable.sol
    │   ├── AllowList.sol
    │   ├── DenyList.sol
    │   └── interface
    │       └── IAccessRegistry.sol
    ├── Errors
    │   ├── LibErrors.sol
    │   └── interface
    │       └── IERC721Errors.sol
    └── Utils
        ├── ContractUriUpgradeable.sol
        ├── MarketplaceOperatorUpgradeable.sol
        ├── PauseUpgradeable.sol
        ├── RoleAccessUpgradeable.sol
        ├── SalvageUpgradeable.sol
        └── TokenUriUpgradeable.sol

更新：该代码库已迁移到新仓库 https://github.com/fireblocks/fireblocks-smart-contracts，并在 pull request #3 中审计到提交 a8942e9。该 PR 随后被合并到 main 分支的提交 108a92f。

系统概览

Fireblocks Upgradeable Tokens 项目实现了三个可升级的代币合约，即 ERC721F。该合约遵循 ERC-721 代币标准，并集成了额外的功能以管理访问控制。该代币支持配置访问列表，以便于转移代币，提供了实现一个 allowlist（只有列表上的地址可以使用代币）或 denylist（限制列表上的地址与合约交互）的灵活性。

特权角色

该协议实现了多个与 ERC721F 合约相关的特权角色。

DEFAULT_ADMIN_ROLE 的持有者可以：

  • 授权其他地址获取角色。

UPGRADER_ROLE 的持有者可以：

  • 将实现合约升级为新版本。

PAUSER_ROLE 的持有者可以：

  • 暂停和恢复合约。

CONTRACT_ADMIN_ROLE 的持有者可以：

  • 更改使用的访问列表地址。
  • 更新合约 URI。
  • 更新代币 URI 地址。
  • 更新市场操作员地址。

MINTER_ROLE 的持有者可以：

  • 铸造代币。

BURNER_ROLE 的持有者可以：

  • 销毁代币。

RECOVERY_ROLE 的持有者可以：

  • 从缺少访问的账户中恢复代币。

SALVAGE_ROLE 的持有者可以：

  • 恢复发送到代币合约本身的 ETH 和 ERC-20 代币。

信任假设

  • DEFAULT_ROLE_ADMIN 角色的持有者被期望是非恶意的，并会在项目的最佳利益下行事。

高严重性

ERC721F 转移访问列表可以被绕过

ERC721F 合约实现了逻辑以验证发送者和接收者是否具有访问权限。该功能通过重写 transferFrom 函数并包含必要的访问检查引入。然而，问题出现是因为该合约继承自 ERC721Upgradeable 合约，后者包括两个额外的函数，均命名为 safeTransferFrom，但具有不同的参数。这些函数不内部使用重写的 transferFrom 函数，而是依赖于 _safeTransfer 函数。因此，用户可以在没有适当访问权限的情况下使用 safeTransferFrom 功能绕过访问列表检查并成功转移代币。

考虑引入额外检查，以确保通过 safeTransferFrom 功能正确处理转移。

更新：已确认，在提交 7710940 中解决。

中等严重性

Burner 可以燃烧任何 ERC721F 代币

ERC721F 合约实现了一个仅允许 BURNER_ROLE 地址执行的 burn 函数。该功能旨在仅允许调用者燃烧其拥有的代币，如该函数的 NatSpec 描述 所示。然而，该函数直接调用了 _burn 函数，而没有进行任何基本检查，以验证调用者是否是被燃烧代币的所有者或具有燃烧权。

考虑使用 _isApprovedOrOwner 函数来确保 _msgSender() 是被批准的方或指定代币的所有者。

更新：已确认，在提交 a5e2488 中解决。

低严重性

遵循检查-效果-交互 (CEI) 模式

ERC721F 合约实现了一个 mint 函数，但该函数不遵循 CEI 模式。该函数使用 _safeMint 来铸造代币，并确保接收方是通过 onERC721Received Hook指定的合约。这样接收者可以劫持执行，并通过 _tokenUriUpdate 函数 设置代币 URI。

考虑首先更新代币 URI，然后调用 _safeMint。

更新：已确认，在提交 db999c8 中解决。

检索整个访问列表可能会回退

AccessListUpgradeable 合约包括一个 getAccessList 函数，检索访问列表中的所有值。该函数使用 EnumerableSetUpgradeable.AddressSet 的 values 函数，将整个存储复制到内存中。当访问列表过大时，这可能导致问题，因为函数执行仍可能受到区块Gas限制的影响，从而导致调用失败。

考虑实现一个额外的函数，用于在值集过大时使用。该函数应仅返回全部集合中的一部分或切片值。

更新：已确认，在提交 130dbb5 中部分解决。Fireblocks 团队声明：

为了解决这个问题，我们通过为该函数添加额外文档进行了处理。这是因为 getAccessList() 的目的是作为一个支持函数，旨在为较小项目获取紧凑的访问列表。在拥有更大访问列表的较大项目中，建议设计自定义索引逻辑以高效地从访问列表中检索所有地址。

备注与附加信息

缺乏安全联系人

在智能合约中提供特定的安全联系人（例如，电子邮件地址或ENS名称）可以大大简化个人在发现代码中的漏洞时与开发者的交流过程。这种做法是有益的，因为它允许代码拥有者规定漏洞披露的沟通渠道，从而消除因缺乏如何进行报告而导致的误解或报告失败的风险。此外，如果合约包含第三方库且出现漏洞，那么这些库的维护者也可以更容易地联系到适当的负责人，提供缓解建议。

考虑在合约定义之上添加一个包含安全联系人的 NatSpec 注释。建议使用 @custom:security-contact 约定，因为它已被 OpenZeppelin Wizard 和 ethereum-lists 采用。

更新：已确认，未解决。Fireblocks 团队声明：

由于这些合约被视为其他公司和实体去大规模部署的模板，因此不可能有单一的安全联系人。因此，当公司部署这些合约的其中一个时，他们可以独立地通过合约 URI 提供自己的安全联系人。

映射中的命名参数缺失

自 Solidity 0.8.18 起，开发者可以在映射中使用命名参数。这意味着映射可以的形式为 mapping(KeyType KeyName? => ValueType ValueName?)。这种更新的语法提供了对映射目的的更透明表征。

例如，_tokenUri 映射在 TokenUriUpgradeable 合约中未使用命名参数。

考虑为映射添加命名参数，以提高代码库的可读性和可维护性。

更新：已确认，在提交 967c097 中解决。

不必要的变量类型转换

在 AccessRegistrySubscriptionUpgradeable 合约中，_accessRegistry 变量在 _accessRegistryUpdate 函数中不必要地进行了类型转换。

为了改善代码库的整体清晰性、意图和可读性，考虑删除任何不必要的类型转换。

更新：已确认，在提交 69c5add 中解决。

未使用的导入

在 代码库 中，有多个未使用的导入可以被移除：

考虑删除未使用的导入，以提高代码库的整体清晰性和可阅读性。

更新：已确认，在提交 4243399 中解决。

缺乏 __gap 变量

在整个 代码库 中，有多个可升级合约缺少 __gap 变量。例如：

请注意，目前的实现不需要实施 __gap 存储变量，因为所列合约不被继承。然而，建议仍然添加它，因为它表明合约实现可能会升级，并且未来可能会引入其他存储变量。

考虑添加一个 __gap 存储变量，以避免在可升级合约中将来的存储冲突。

更新：已确认，未解决。Fireblocks 团队声明：

_缺少 __gap 变量的合约在我们的用例中没有必要，因为升级旨在通过合约继承进行。在此过程中，初始实现 V1 会被 V2 继承，后者会包含新的逻辑。这使得可以在不与现有插槽冲突的情况下添加存储插槽。_

AccessListUpgradeable 中存储间隙大小不正确

AccessListUpgradeable 合约使用名为 __gap 的存储变量，大小为 49。它假设 _accessList 存储变量占用一个存储插槽，而实际上它占用两个存储插槽。_accessList 的类型为 EnumerableSetUpgradeable.AddressSet 这是一个使用 两个存储插槽 的结构。

考虑将 AccessListUpgradeable 合约中 __gap 存储变量的大小从 49 更改为 48。

更新：已确认，在提交 519e415 中解决。

使用混淆的错误

ERC721F 合约包含一个 recoverTokens 函数，旨在从缺少访问的账户中恢复代币。当账户已经可以访问代币时，问题出现，导致交易回退并显示诸如 ERC721InvalidReceiver 之类的混淆错误。在这个上下文中，由于账户不是接收者，因此错误不应暗示接收方相关的问题。

考虑添加一个新错误，表明账户可以访问这些代币。

更新：已确认，在提交 6b3f311 中解决。

排版错误和不正确的注释

考虑解决以下排版错误和不正确的注释：

更新：已确认，在提交 7a5e68e 中解决。

Gas优化

在代码库中，有几个潜在的Gas优化实例：

  • ERC721F 合约中的 _requireHasAccess 函数多次检查 isSender 值。初步验证账户是否通过 accessRegistry 合约具有访问权限将更高效。如果该账户没有访问权限，则根据 isSender 值，可选择因无效发送者或无效接收者错误而回退。
  • 代码库中常见的模式是在更新状态变量后检查访问权限。例如，在 RoleAccessUpgradeable 合约中，revokeRole 函数在更新权限后调用了 _authorizeRoleAccess 函数。为了优先确保购入早期并节省Gas，考虑在更新状态变量之前执行访问检查。

为了提高Gas消耗、可读性和代码质量，考虑在可能的地方进行重构。

更新：已确认，在提交 f84408b 中解决。

缺失的恢复 ERC-721 和 ERC-1155 代币的功能

SalvageUpgradeable 抽象合约被 ERC721F 合约继承，实现了 恢复 ETH 和 ERC-20 代币 的逻辑。这在代币意外发送到代币合约自身而不是有效接收者时非常有用。然而，救助逻辑仅适用于恢复 ETH 和 ERC-20 代币，而不适用于 ERC-721 或 ERC-1155 代币。因此，当任何三种代币合约中的 ERC-20 代币意外发送时，可以进行恢复；但 ERC-721 或 ERC-1155 代币则无法恢复。

考虑添加恢复 ERC-721 和 ERC-1155 代币的功能，以应对意外发送到 ERC721F 合约的情况。

更新：已确认，未解决。Fireblocks 团队声明：

目前尚未观察到客户对恢复 ERC-721 或 ERC-1155 代币的需求。虽然可以通过升级引入此功能，但我们仍然对因未使用当前功能而使用户面临额外字节码和Gas费用持谨慎态度。

结论

Fireblocks Upgradeable Tokens 项目引入遵循 ERC-721 标准的可升级代币合约，增加了访问控制的功能。这些代币允许创建访问列表用于转让，可以实现一个 allow list，限制使用于特定地址，或 deny list，防止与列表中地址的交互。

发现了一项高严重性和一项中等严重性漏洞，以及多个低严重性问题。此外，还提出了多项建议，以提高代码库的质量和文档。Fireblocks 团队反应迅速，及时回答了我们关于代码库的任何问题。

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

0 条评论

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

OpenZeppelin

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