Fireblocks 可升级代币审计 - ERC1155F

目录

• 目录
• 摘要
• 范围
• 系统概述
  • 特权角色
  • 信任假设
• 低严重性
  • 检索整个访问列表可能会回溯
• 备注与附加信息
  • 缺乏安全联系人
  • 映射中缺少命名参数
  • 不必要的变量转换
  • 未使用的导入
  • 缺乏 __gap 变量
  • AccessListUpgradeable 中错误的存储间隔大小
  • 使用了混淆的错误
  • 缺少输入验证
  • 拼写错误和错误的注释
  • gas 优化
  • 缺失 ERC-721 和 ERC-1155 代币救援功能
• 结论

摘要

TypeDeFiTimelineFrom 2023-11-13To 2023-11-17LanguagesSolidity总问题12 (8 已解决, 1 部分解决)临界严重性问题0 (0 已解决)高严重性问题0 (0 已解决)中等严重性问题0 (0 已解决)低严重性问题1 (1 部分解决)备注与附加信息11 (8 已解决)

范围

我们审核了 Fireblocks 仓库在提交 e7003dfb 处的代码。

在审计结束后，Fireblocks 已在提交 6d8327b 的 拉取请求 #1 中将许可证更新为 AGPL-3.0-or-later。此更新对源代码逻辑的安全性没有影响。

此次审计的合同包括：

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

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

系统概述

Fireblocks 可升级代币项目实现了三个可升级的代币合约，即 ERC1155F。该合约符合 ERC-1155 代币标准，并集成了管理访问控制的附加功能。此代币支持配置访问列表以进行代币转账，提供灵活性以实现允许列表，只有在该列表上的地址才能使用代币，或者拒绝列表，这限制了在该列表上的地址与合约的交互。

特权角色

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

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 角色的持有者预计为非恶意，并会为项目的最佳利益行事。

低严重性

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

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?) 的形式。这种更新的语法提供了映射目的的更透明表示。

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

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

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

不必要的变量转换

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

为了提高整个代码库的清晰度、意图和可读性，考虑删除任何不必要的转换。

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

未使用的导入

在 代码库 中，有多次未使用的导入，可以删除：

• import {LibErrors} from "../Errors/LibErrors.sol"; 导入在 ContractUriUpgradeable.sol 中未使用的别名 LibErrors
• import {LibErrors} from "../Errors/LibErrors.sol"; 导入在 MarketplaceOperatorUpgradeable.sol 中未使用的别名 LibErrors
• import {LibErrors} from "../Errors/LibErrors.sol"; 导入在 PauseUpgradeable.sol 中未使用的别名 LibErrors
• import {LibErrors} from "../Errors/LibErrors.sol"; 导入在 TokenUriUpgradeable.sol 中未使用的别名 LibErrors

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

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

缺乏 __gap 变量

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

• AllowList 合约
• DenyList 合约
• ERC1155F 合约

请注意，当前实现并不需要实现 __gap 存储变量，因为所列合约不再被继承。然而，仍建议添加它，因为它暗示合同实现可能会升级，并且未来可能会引入额外的存储变量。

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

更新： 认可，未解决。Fireblocks 团队表示：

_缺乏 __gap 变量的合约对于我们的使用案例不是必需的，因为升级旨在通过合同继承进行。在这个过程中，初始实现 V1 被 V2 继承，其中新的逻辑存在。这使得可以添加存储槽，而不会与现有槽发生冲突。_

AccessListUpgradeable 中错误的存储间隔大小

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

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

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

使用了混淆的错误

ERC1155F 合约具有一个 recoverTokens 函数，旨在从缺乏访问权限的账户中恢复代币。当账户已经获得代币访问权限时，就会出现问题，导致交易回溯并返回潜在混淆的错误，例如 ERC155InvalidReceiver。在这种情况下，由于该账户不是接收者，错误不应暗示与接收者相关的问题。

考虑添加一个新错误，表明该账户已经获得代币的访问权限。

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

缺少输入验证

ERC1155F 合约的 mintBatch 函数缺少数组大小验证。考虑验证 id、amount 和 URI 数组是否具有相同的大小。

考虑实现建议的验证，以防止可能导致对协议的潜在攻击的意外行为。

更新： 认可，已在提交 6953e70 中解决。

拼写错误和错误的注释

考虑解决以下拼写错误和错误的注释：

• AccessRegistrySubscriptionUpgradeable 合约实现的 __AccessRegsitrySubscription_init 函数名称中存在拼写错误。应为 __AccessRegistrySubscription_init。
• 描述 AccessListUpgradeable、AllowList 和 DenyList 合约的 NatSpec 注释使用参数名称 claim，而不是 data。

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

gas 优化

在代码库中，有多个潜在的 gas 优化实例：

• ERC1155F 合约中的 _requireHasAccess 函数重复检查 isSender 值两次。通过先通过 accessRegistry 合约验证账户是否有访问权限会更加高效。如果没有访问权限，则根据 isSender 值要么因无效发送者返回，要么因无效接收者返回错误。
• 代码库遵循在更新状态变量后检查访问的常见模式。例如，在 RoleAccessUpgradeable 合约中，revokeRole 函数在更新角色后调用_authorizeRoleAccess 函数。为了优先确保早期故障并节省 gas，考虑在更新状态变量之前执行访问检查。

为了提高 gas 消耗、可读性和代码质量，考虑尽可能进行重构。

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

缺失 ERC-721 和 ERC-1155 代币救援功能

SalvageUpgradeable 抽象合约，由 ERC1155F 合约继承，实施了恢复 ETH 逻辑 和 ERC-20 代币逻辑。当代币意外发送到代币合约本身而不是有效接收者时，这很有用。然而，救援逻辑仅适用于恢复 ETH 和 ERC-20 代币，而不适用于 ERC-721 或 ERC-1155 代币。因此，虽然可以恢复偶然发送到任意三个代币合约的 ERC-20 代币，但无法恢复 ERC-721 或 ERC-1155 代币。

考虑添加恢复意外发送到 ERC1155F 合约的 ERC-721 和 ERC-1155 代币的功能。

更新： 认可，未解决。Fireblocks 团队表示：

目前，没有观察到对救援 ERC-721 或 ERC-1155 代币的客户需求。尽管该功能可能会通过升级在未来进行集成，但我们谨慎对待不希望用户因当前功能未得到利用而承担额外的字节码和 gas 费用。

结论

Fireblocks 可升级代币项目引入了符合 ERC-1155 标准的可升级代币合约，并增加了访问控制的功能。这些代币允许为转移创建访问列表，实现允许列表，限制使用于指定地址，或拒绝列表，防止与列表上的地址交互。

确定了一个低严重性的问题。此外，还提出了多项建议，以提高代码库的质量和文档。Fireblocks 团队响应及时，并解答了我们关于代码库的任何问题。

• 原文链接： blog.openzeppelin.com/fi...
• 登链社区 AI 助手，为大家转译优秀英文文章，如有翻译不通的地方，还请包涵～