2021 年 12 月 16 日
介绍
1inch 团队要求我们审查和审计他们的 限价单协议 v2 智能合约。 我们查看了代码,现在发布我们的结果。
范围
我们审核了提交 4d94eea25e4dac6271bfd703096a5c4a4d899b4a
的 1inch/limit-order-protocol
存储库。 范围包括以下合同:
– OrderMixin.sol
– OrderRFQMixin.sol
– PredicateHelper.sol
– RevertReasonParser.sol
– Permitable.sol
– ChainlinkCalculator.sol
– ArgumentsDecoder.sol
– AmountCalculator.sol
– NonceManager.sol
– LimitOrderProtocol.sol
– ImmutableOwner.sol
– InteractiveNotificationReceiver.sol
– AggregatorInterface.sol
– IDaiLikePermit.sol
所有其他项目文件和目录(包括测试),以及外部依赖项和项目、博弈论和激励设计,也被排除在本次审计的范围之外。 假设外部代码和合约依赖项按照记录的方式工作,并且假设 1inch 提供的后端服务符合协议的最佳利益。
总体健康
总的来说,我们发现该项目的代码库可读且组织良好,尽管它可以受益于更广泛的文档,特别是围绕汇编代码块、协议的边缘情况、将使用的资产/谓词/外部资源,所提供的后端服务的职责/限制以及参与者之间的交互。 该项目不遗余力地提高操作的 Gas 效率,有时甚至冒着使代码更难以推理的风险; 我们在下文提出与此相关的问题。 在整个审核过程中,1inch 团队的可用性很高、响应迅速且非常易于合作。
系统总览
限价订单协议支持订单 makers
签署链下代币交换订单。 然后,该协议有助于按订单填写先前签署的订单 takers
。 订单具有高度可扩展性,并且可以在整个订单填写过程中的多个点静态调用外部合约。 这种可扩展性使协议具有实用性,但它增加了订单本身的复杂性和更大的攻击面。
值得注意的是,没有订单详细信息的链上存储。 订单的填写状态或取消状态只能通过订单哈希来跟踪。 这需要点对点或通过中心化方共享订单。 在这种情况下,1inch 团队打算充当中心化一方,聚合已签署的订单并将这些订单用作其他协议的流动性来源。 订单将通过他们自己的 API 发布,以便用户能够与其交互。
这种集中化使 1inch 团队能够对订单的发布和最终执行进行完全控制。 这也使他们能够审查订单,这在恶意或欺骗性订单的情况下可能很有用,但也可能被滥用,并允许他们在有利订单的情况下通过不通过 API 显示来抢先于任何其他用户。
特权角色
尽管该角色所使用的合同超出了范围,但仍确定了一个特权角色。 一个 immutableOwner
在构建时设置为代理合约的创建者,用于限制对代理合约的访问 external
功能。
外部依赖和信任假设
该协议的设计需要链下和链上组件,并且这种混合模型可用于减轻我们在报告中确定的一些攻击向量,但这种能力的成本是增加对 1inch 团队和基础设施的依赖。
此外,限价订单协议提供了从 Chainlink 预言机检索价格的功能。 我们假设这些预言机是诚实的、可访问的且功能正常的。
此外,由于订单的灵活性,有多个与外部合同的联系点未经验证。 这意味着恶意用户可能会滥用此类调用,并使用恶意合约冒充谓词、资产或预言机,以便在订单填写期间执行操作。 尽管该项目在某些区域受到保护以防止重入,但此类向量可能会导致拒绝服务攻击或未检测到的垃圾邮件订单。 1inch 团队意识到在协议中使用不熟悉的合约时可能会出现某些问题,并表示他们的意图是只有主要的“蓝筹”资产才会得到该项目的完全支持。 然而,应该指出的是,即使是最受欢迎的资产,每种资产也存在一些固有行为,这些行为可能会导致协议出现问题,而这些问题无法正确解决,例如在使用 USDT 转账时产生费用,或者返回错误代码而不是错误代码。 cTokens 的成功布尔值。
发现
在这里,我们展示我们的发现。
严重程度
没有。
高严重性
[H01] 传入的数据不一致 _makeCall
在 OrderMixin
合同 _makeCall
功能用于转移资产 从接受者到制造者 然后 从制造者到接受者。 在后者的转移中, _makeCall
函数错误地传递了订单的 makerAsset
作为最后一个参数,它应该是订单的 makerAssetData
.
因此,任何依赖于 makerAssetData
争论将会破裂。
为了与之前的调用保持一致 _makeCall
并要完全支持代理功能,请考虑更新 order.makerAsset
参数 order.makerAssetData
.
更新: 固定在 拉取请求#57.
[H02]部分成交的私人订单任何人都可以成交
该协议允许创建私人和公共秩序。 对于私人订单,仅 allowedSender
由制造商在订单创建期间指定的地址能够填写订单。
但是,在 OrderMixin
合同, 验证 allowedSender
地址 范围不正确,这意味着它仅在处理订单首次执行的逻辑内进行评估。 如果私人订单已部分履行,则检查 allowedSender
地址不再可达,订单可由任何人填写。
要澄清关于任何用户是否应该能够填写部分填写的私人订单的意图,请考虑记录当前行为的原因或验证 allowedSender
地址超出第一次填充的范围,以确保每次尝试填充时都会对其进行验证。
更新: 固定在 拉取请求#58.
[H03]恶意制造商可能利用部分填充来窃取taker的资产
订单来自 OrderMixin
合同有能力部分履行。 为了支持部分填充,协议需要一种计算交换双方的方法。 两个都 getMakerAmount
和 getTakerAmount
字段由订单制造商为此目的定义。
填写订单时,接受者必须提供 makingAmount
或者 takingAmount
值以及 thresholdAmount
价值。 根据是否可以采用两种不同的代码路径 makingAmount
或者 takingAmount
被提供。
第一个是当 makingAmount
参数已定义。 它可以 截短 此 makingAmount
价值也 计算 takingAmount
它的价值。 在这种情况下, thresholdAmount
确保 takingAmount
取值为 并不出乎意料地大.
第二个是当 takingAmount
参数已定义。 在这种情况下,它会 计算 makingAmount
值,有可能 截断它 和 重新计算 takingAmount
如果发生这种情况的话。 在这种情况下, thresholdAmount
值确保 makingAmount
返回值是 并不出乎意料地小.
存在两种利用方法,每种方法对于前面提到的代码路径之一都是唯一的。 这些利用方法需要恶意 getMakerAmount
和 getTakerAmount
功能。 这些函数的简单实现将具有相同的行为 AmountCalculator
的 getMakerAmount
和 getTakerAmount
功能,但具有硬编码开关,将迫使它们在需要时返回攻击者控制的值。
第一个不太严重的漏洞利用模式涉及第一个代码路径,其中 makingAmount
值已指定 在填充订单中。 恶意制造商会等待指定的填写订单 makingAmount
出现在内存池中以抢占先机。 他们会从制造商那里耗尽除 1 之外的所有价值,然后强制 _callGetTakerAmount
返回用户指定的金额 thresholdAmount
价值(或他们的津贴,如果较少的话)。 当用户的交易最终完成时,他们将交换其全部 thresholdAmount
价值 takerAsset
对于单个单位 makerAsset
。 此漏洞利用的数量受到限制 thresholdAmount
的价值或金额 takerAsset
用户允许的 LimitOrderProtocol
合同。
第二种更严重的利用模式涉及第二个代码路径,其中 takingAmount
值已指定。 恶意制造商同样会等待指定的成交订单 takingAmount
显示在内存池中的值。 他们会抢先交易并迫使 makingAmount
返回值 _callGetMakerAmount
函数要高于两者 remainingMakerAmount
和 thresholdAmount
。 他们还将设置 takingAmount
返回值由 _callGetTakerAmount
为 takerAsset
允许的资产 LimitOrderProtocol
由接受者。 当taker的交易完成后, 截断 makingAmount
值,然后 重新计算 takingAmount
价值。 然而,这种重新计算并不能保证会更低,在这种情况下,将耗尽接受者的所有资金 takerAsset
他们在合同上允许的。 在此代码路径中, thresholdAmount
价值是 确保 makingAmount
也不算太低,所以取所有接受者的 takerAsset
资产未经检查。 损失的资金以金额为限 takerAsset
用户允许的资产 LimitOrderProtocol
合同。
如果没有部分订单,更具体地说,如果没有恶意的部分订单,这些漏洞是不可能实现的。 getMakerAmount
和 getTakerAmount
实现。
该问题的主要问题是 thresholdAmount
值检查是它只覆盖交换的一侧,但另一侧可以通过抢先交易来操纵。 无法保证接受者最初提出的价值保持不变。 考虑删除 makingAmount
如果订单无法支持与请求一样大的填充,则从两个代码路径截断并恢复。 通过这样做, thresholdAmount
可用于充分限制交换的另一方并避免意外行为,即使是恶意订单。
更新: 固定在 拉取请求#83.
中等严重程度
[M01] 在动态参数之后传递静态参数
在 OrderMixin
合同 getTakerAmount
和 getMakerAmount
bytes 字段用作参数 _callGetTakerAmount
和 _callGetMakerAmount
功能。 这些调用提供了一种根据另一侧计算掉期的一侧的方法,并且允许用户部分填写订单。
getTakerAmount
/getMakerAmount
字段是动态变量并且被打包在前面 takerAmount
和 makerAmount
中的值 _callGetTakerAmount
和 _callGetMakerAmount
功能。 恶意制造者有可能提供比预期更多的数据 getTakerAmount
和getMakerAmount
字段来推动 takerAmount
和 makerAmount
在下一个函数中解码时超出其假定位置的字节。 这允许制造商将传入的接受者或制造商数量向右移动一个完整字节,如果提供额外的 32 字节数据,甚至可以完全替换它们。
用户已经必须手动查看 getTakerAmount
和 getMakerAmount
顺序中的字段,但这种技术很难被发现。 另外值得注意的是,这种攻击甚至适用于内部信任的 getMakerAmount
和 getTakerAmount
功能。 对于大多数攻击,提供合理的阈值金额将防止资金损失。
为了防止这种情况,请考虑在动态参数之前对静态参数进行编码,以避免为动态参数提供控制静态参数的方法。
更新: 不固定。 1inch团队表示:
我们会特别注意吸气剂验证。 我们将尝试在 sdk 中实现 getter 的健全性验证,这将有助于过滤潜在的恶意订单。
[M02]可操纵ERC721订单
不仅可以通过 ERC20 进行交换 OrderMixin
通过部署与 IERC20 共享相同功能选择器的合约 transferFrom
,并提供该合同作为 makerAsset
或者 takerAsset
按顺序。
超出范围的代理,即 ERC721Proxy
, ERC721ProxySafe
及 ERC1155Proxy
合同遵循这种模式来提供支持 ERC721
和 ERC1155
代币。 由于必须使用与 IERC20 相同的模式来调用代理 transferFrom
调用时,签名必须以 address from
, address to
和 uint256 amount
。 代理需要的任何其他内容都可以在之后传入,并按顺序定义为 makerAssetData
和 takerAssetData
.
ERC1155 自然可以一次传输多个相同 id 的代币,这意味着 ERC1155Proxy
合同利用了 amount
场地。 另一方面, ERC721
s 没有明显的用途 amount
场地。 由于它们代表不可替代的代币,因此特定的 tokenId 将仅存在一个,从而呈现 amount
场无用。 因此,两者的实施 ERC721Proxy
和 ERC721ProxySafe
合同使用所需的 amount
场作为 tokenId
代替。
这种超载的 amount
参数创造了部分填充的可能性 ERC721
订单以便以折扣价购买单独列出的代币。 例如,可能存在单个用户拥有多个 ERC721
允许转让的同一合同的 ERC721Proxy
合同并在单独的限价订单中列出它们。
如果限价单还提供 getMakerAmount
和 getTakerAmount
字段,可以部分填充这些字段 ERC721
命令。 自从订单的 amount
字段实际上对应于 tokenId
,恶意用户可以部分填充 ERC721
具有较高的 tokenId,导致 makingAmount
/takingAmount
一个 ERC721
这可能对应于较低的 tokenId
。 结果是 ERC721
与较低 tokenId
将以以下价格转让 (higher tokenId price) * (lower tokenId's id) / (higher tokenId's id)
.
此漏洞利用有一些要求:
- 多
ERC721
来自同一合同的 s 允许在任一ERC721
由单一所有者代理。 - 为其中之一开放订单
ERC721
这不是最低的tokenId
允许的。 - 订单上允许部分填充。
彻底消除部分的可能性 ERC721
填充,考虑分开 amount
和 tokenId
论据。 无论参数是否分开,请考虑记录下来以提醒用户这种行为并避免将来出现这种模式。
更新: 固定在 拉取请求#59.
[M03]未记录的小数假设
LimitOrderProtocol
合约继承了 ChainlinkCalculator
合同通过 OrderMixin
合同。 该合约公开了两个函数,以便在 谓词检查 和查找 厂商数量/接受者金额.
然而,合约对 Chainlink 预言机应报告的小数位数以及函数参数应包含的小数位数做出了未记录的假设。 在某些情况下,这可能会导致意外行为,包括资产错误定价和无意的资金损失。
更具体地说,整个合约中隐含的假设是 Chainlink 预言机将以 18 位小数的精度进行报告。 然而,不 所有 Chainlink 预言机 报告的小数位数。 事实上,如果预言机报告以货币(例如美元)表示的代币对,它的精度只有 8 位小数。 由于没有任何限制 这 可以使用预言机,但不应对其报告的小数位数做出隐含的假设。
相关地,有一个隐含的假设: amount
的参数 ChainlinkCalculator
函数将使用 18 位小数,以及误导性的显式声明: singlePrice
功能 Calculates price of token relative to ETH scaled by 1e18
。 事实上,即使有一个神谕 不 报告具有18位小数,返回值 singlePrice
函数将按小数位数缩放 amount
参数,不一定是18位小数。
同样的, doublePrice
函数假设两个 Chainlink 预言机将报告相同的小数位数,导致函数结果偏离预期。
考虑明确记录有关参数和返回值应采用的小数位数的假设。 此外,请考虑限制依赖于打破这些假设的预言机的计算,或者让相关计算考虑实际的小数位数。
更新: 固定在 拉取请求#75.
严重程度低
[L01]未显式声明的常量
代码库中出现了一些具有无法解释含义的文字值。 例如:
- 在
OrderMixin
合同_remaining
映射在语义上超载(如问题中所述 映射的语义重载) 跟踪部分成交订单的剩余资产金额 以及 如果订单已完全填写。 具体来说,0
表示尚未执行与订单相关的任何订单,1
意味着订单无法再被执行,并且任何大于1
表示订单相关的剩余金额有可能被执行。 - 在
ChainlinkCalculator
合同,字面价值1e18
用于singlePrice
功能。
为了提高代码的可读性并促进重构,请考虑为每个幻数定义一个常量,并为其指定一个清晰且不言自明的名称。 对于复杂值,请考虑添加内联注释,解释它们的计算方式或选择它们的原因。
[L02] 恶意方可能阻止执行允许的命令
OrderMixin
合约允许创客用户提交 允许的命令 因此这些可以在一笔交易中执行,而不必进行单独的交易进行批准。 此外,接单者还可以 提交自己的许可证 在出于同一目的填写订单期间。
然而,由于制造商的许可证包含在 秩序,当订单执行交易位于内存池中时,制造商和接受者的许可证都可以访问。 这将使任何恶意用户都可以获取这些许可并在相应的资产合约上执行它们,同时抢先执行填充交易。 因为这些许可证有一个 nonce
为了防止双重支出攻击,订单的执行交易将因尝试使用刚刚在抢先交易期间使用的相同许可证而失败。
尽管不存在安全风险,并且制造商可以创建新订单并预先批准交易,但这种攻击肯定会影响允许订单的可用性。 事实上,有动机的攻击者可以阻止 所有 允许进行此攻击的命令。 考虑在订单填写期间验证许可证是否已提交,或者津贴是否足够。 还应考虑在订单组成期间让用户了解这种可能的攻击。
更新: 不固定。 1inch 团队表示:
我们之前进行过审批检查,但决定简化许可流程,仅在审批不成功时恢复。 我们将考虑如何通知制造商有关该问题的信息。
[L03]重复代码
代码库中存在重复代码的实例。 重复代码可能会导致开发生命周期后期出现问题,并使项目更容易出现错误。 当功能更改未在本应相同的所有代码实例中复制时,可能会无意中引入此类错误。 重复代码的示例包括:
与其重复代码,不如考虑只使用一个包含重复代码的合约或库,并在需要重复功能时使用它。
更新: 部分固定在 拉取请求#60.
[L04] 错误或误导性的测试套件
测试套件中存在一些测试偏离预期行为的实例。 例如:
-
ChainlinkCalculator
合同由继承人继承OrderMixin
合同。 然而,在测试过程中AmountCalculator.arbitraryStaticCall
函数用于调用ChainlinkCalculator
合同作为外部的、独立的合同。 即使结果是预期的,测试也应该通过调用来反映系统当前设计和预期用例的行为ChainlinkCalculator
直接运行函数而不使用任意静态调用。 - 尽管代理合约超出了范围,但我们注意到,在使用 ERC721 资产测试协议时,
ERC721Proxy
合约不用于交换其内的资产 测试套件.
由于测试套件本身不在本次审核的范围内,因此请考虑彻底审查测试套件,以确保所有测试根据协议规范成功运行。
更新: 固定在 拉取请求#57, 拉取请求#59及 拉取请求#61.
[L05]事件中的错误和遗漏
在整个代码库中,当对合约进行敏感更改时,通常会发出事件。 然而,许多事件缺乏索引参数和/或缺少重要参数。 例如:
还有一些缺少事件的敏感操作,例如:
考虑对现有事件进行更完整的索引,并在缺少的地方添加新参数。 此外,请考虑以完整的方式发出所有事件,以便它们可用于通过链外服务重建合约的状态。
更新: 不固定。 然而,1inch团队确实添加了一个 orderRemaining
的参数 OrderCanceled
事件 拉取请求#62.
1inch 团队表示:
我们发现只需要有限的数据子集即可满足前端需求。 在进行广泛分析的情况下,所有建议的字段都可以通过跟踪获得。 为了
OrderRFQMixin
我们期望做市商建立自己的复杂方法来跟踪哪些订单已被取消。
[L06]事件发射期间存储变化
在 NonceManager
合同,当 NonceIncreased
事件被发出, 消息发送者的随机数也增加.
同时执行多个操作可能会使代码库更难推理,更容易出错,并可能导致操作被忽视或误解。
为了提高代码的整体意图、可读性和清晰度,请考虑在发出事件之前增加随机数值。
更新: 固定在 拉取请求#63.
[L07]不一致的解码方法可能导致结果差异
为了支持其所有可扩展性和灵活性,限价订单协议通常必须处理动态字节数据和来自外部合约的任意返回值。 因此,该协议包括 ArgumentsDecoder
库可以更有效地将动态字节值转换为基本数据类型。 然而,这个库并不是专门使用的,在某些情况下 abi.decode
被用来代替。 此外,一些合同正在使用 abi coder v1
当其他人正在使用时 abi coder v2
。 前者的表现与后者更相似 ArgumentsDecoder
库,而后者在解码时执行额外的检查。
这些不同解码方法的不一致使用可能会导致代码库的意图和实际行为之间存在细微的差异。
例如,该 simulateCalls
函数仅使用 ArgumentsDecoder.decodeBool
功能。 如果 simulateCalls
函数用于检查将在订单的谓词部分中进行的调用,那么其结果可能会偏离评估谓词条件时实际发生的结果,因为采用了不同的解码方法。
因此,举例来说,如果一个谓词创建了一个外部 staticcall
某个返回 a 的函数 uint256
值大于 XNUMX 而不是预期值 bool
,那么该调用将恢复,因为返回值是 解码为 abi coder v2
的 abi.decode
它不会接受这样的值 bool
。 但是,如果使用完全相同的调用 simulateCalls
,然后 将简单地标记为 true
因为 decodeBool
将任何大于零的值视为 true
.
制作 simulateCalls
函数完全镜像实际谓词调用的行为,考虑修改它以使用 abi.decode
.
更新: 固定在 拉取请求#82.
[L08]缺乏输入验证
fillOrderToWithPermit
和 fillOrderTo
的功能 OrderMixin
合同,以及 fillOrderRFQToWithPermit
和 fillOrderRFQTo
的功能 OrderRFQMixin
合同,不验证 target
地址参数。
这使得用户有可能无意中传递零地址,从而锁定他们在填写订单后应收到的资产。
为了确保用户不会意外锁定其资金,请考虑验证 target
地址不等于引用函数中的零地址。
更新: 固定在 拉取请求#78.
[L09]单元测试覆盖率低
整个项目的单元测试覆盖率在75%左右,部分合约的覆盖率特别低。
考虑到单元测试在重构和开发新功能时验证代码和防止回归的重要性,我们鼓励将单元测试覆盖率大幅提高到至少 95%,并包括覆盖甚至不太可能发生的情况的边缘情况。
更新: 不固定。
[L10] 误导性或不完整的内联文档
在整个代码库中,发现了一些误导性和/或不完整的内联文档实例,应予以修复。
以下是误导性内联文档的实例:
- 在
ChainlinkCalculator
合同singlePrice
功能 国家规范@notice
行李牌 说它Calculates price of token relative to ETH scaled by 1e18
,但事实上,它的结果是 折扣值 ofamount
令牌按比例缩放1e18
,其中预言机可能不会以 ETH 进行报告(例如,对于不包括 ETH 的货币对)。 - 在
OrderRFQMixin
合同invalidatorForOrderRFQ
功能 国家规范@return
行李牌 具有误导性,因为可能尚未填写报价以设置相应的无效器位。 订单也可能已被取消。 - 上线 147, 165及 188 of
OrderMixin.sol
, 国家规范@param
标签不符合语法。 - 在线 20 of
ERC1155Proxy.sol
是,@notice
标签指出计算出的哈希值是对func_733NCGU
函数,它应该是func_301JL5R
函数代替。
以下是不完整的内联文档的实例:
- 中的功能
AmountCalculator
合同没有描述任何参数。 - 在
ChainlinkCalculator
合同singlePrice
和doublePrice
函数没有描述所有参数。 - 在
ImmutableOwner
合约中,公共变量和修饰符没有 NatSpec。 - 在
InteractiveNotificationReceiver
合同notifyFillOrder
函数不描述任何参数。 - 在
LimitOrderProtocol
合同DOMAIN_SEPARATOR
函数没有 NatSpec。 - 中的事件和映射
NonceManager
没有国家规范。 - 在
OrderRFQMixin
合同,cancelOrderRFQ*
函数不描述返回值。 - 在
OrderMixin
合同中,一些功能缺乏完整的 NatSpec。 - 在线 168 of
OrderMixin.sol
并在线 71 ofOrderRFQMixin.sol
,它缺少@dev
标签。 - 中的功能
PredicateHelper
合同没有描述所有参数。
清晰的内联文档对于概述代码的意图至关重要。 内联文档和实现之间的不匹配可能会导致对系统预期行为的严重误解。 考虑修复这些错误,以避免开发人员、用户和审计人员感到困惑。
更新: 部分固定。 中解决的误导性文档 拉取请求#75 和 拉取请求#77.
1inch 团队表示:
我们修复了误导性文档。 文档的完成将在稍后完成。
[L11] 使用钩子时可能出现的 DoS 命令
OrderMixin
合约实现了填写通用链下互换订单的功能,这可能有其成功的条件。 在订单填写期间,订单可以 检查预定义的“谓词”条件 在继续执行之前。
然而,由于这些谓词条件可能针对任意合约的逻辑,因此恶意制造者可能会欺骗接受者,让他们相信订单行为正确并且在链下检查时有效,但在尝试填写同一订单时会失败链上。 谓词行为的这种改变可以通过前置运行谓词所依赖的某些变量状态、通过检查发送的气体甚至调用中涉及哪些地址、或者通过一些其他逻辑来实现。
此外,如果制造商定义了 交换期间的互动是, interactionTarget
合约可以自行恢复或撤销津贴以阻止订单成功填写,本质上会导致与恶意谓词相同的结果。
尽管资产不会面临风险,但找到有利订单的用户或机器人将承担更大的负担,试图识别这些表面上看似合法的垃圾邮件订单。 如果他们无法识别此类订单,他们将产生浪费的天然气成本。 要减少垃圾邮件订单的数量,请考虑限制这些挂钩的可用目标。 还可以考虑在用户尝试填写订单之前警告用户这种可能性。
更新: 不固定。 1inch 团队表示:
我们在后端处理这个问题,我们会考虑如何通知可能的接受者有关该问题的信息。
[L12] 舍入可能不利于 taker
在 OrderMixin
和 OrderRFQMixin
合约,当订单正在被填写并且接受者仅提供 makingAmount
or takingAmount
amount,协议尝试计算交换的对应金额。
这些计算存在两个问题,第一个是没有文档或逻辑限制金额参数应使用的小数位数,我们在 未记录的十进制假设 问题。
第二个问题是,在这些计算过程中,协议对制造商有利。 当隐含的小数假设被打破时,四舍五入问题可能会大大加剧,但即使一切都符合预期,四舍五入也会发生小而奇数的情况。
考虑允许接受者指定最低金额 makerAsset
他们愿意接受的资产以及最大金额 takerAsset
他们愿意交换的资产,以便更明确地接受任何舍入。
更新: 不固定。 1inch 团队表示:
阈值金额应足以保护接受者。
[L13]缺少参数时的矛盾订单处理
在 OrderMixin
合同 fillOrderTo
函数进行内部调用 _callGetMakerAmount
和 _callGetTakerAmount
每当尝试填充并且任一 makingAmount
或者 takingAmount
参数分别为零,或者如果 makingAmount
值大于 remainingMakerAmount
计算值。
_callGetMakerAmount
和 _callGetTakerAmount
如果订单不是使用创建的,调用将导致撤销 getMakerAmount
or getTakerAmount
分别参数,并且正在执行部分填充。
An 旁边的内嵌注释 _callGetMakerAmount
和 旁边的内嵌注释 _callGetTakerAmount
如果订单不是用以下命令创建的,则声称“只允许全部成交” getMakerAmount
or getTakerAmount
参数。
但是,有些代码路径不适用,因为这些路径不检查 length
两者的 getMakerAmount
和 getTakerAmount
参数。
具体而言, 当一个 taker
指定一个 takerAmount
仅具有一个订单的价值 getMakerAmount
,除非调用 getMakerAmount
返回的金额大于 remainingMakerAmount
,可以执行与内嵌文档相矛盾的部分填充。
这使得这些代码路径的意图不清楚。 如果这是预期的行为,请考虑修改内联文档以使其更加明确。 如果这是无意的行为,请考虑始终检查两个的长度 getMakerAmount
和 getTakerAmount
同时参数,以便实现强化内联文档描述的行为。
更新: 固定在 拉取请求#79.
[L14] 使用已弃用的 Chainlink 调用
ChainlinkCalculator
合约旨在用于查询 Chainlink 预言机。 它通过打电话给他们来做到这一点 latestTimestamp
和 latestAnswer
方法, 两者均已被弃用。 事实上,Chainlink 聚合器的 API 中不再存在这些方法 从第三版开始.
为了避免未来与 Chainlink 预言机潜在的不兼容,请考虑使用 latestRoundData
方法代替。
更新: 固定在 拉取请求#67.
注释和其他信息
[N01]不导入接口
AggregatorInterface
接口似乎是从以下位置复制的代码的子集 ChainLink
的公共代码存储库。 完整的接口包含在 ChainLink
的合约 npm 包.
如果可能,为了减少接口不匹配和由此产生的问题的可能性,而不是重新定义和/或重写另一个项目的接口,请考虑使用通过其官方 npm 包安装的接口。
更新: 固定在 拉取请求#66.
[N02] 已弃用的项目依赖项
在安装过程中 项目的依赖关系,NPM 警告已安装其中一个软件包, Highlight
,“将来将不再受支持或接收安全更新”。
尽管此包不太可能导致安全风险,但请考虑将使用此包的依赖项升级到维护版本。
更新: 固定在 拉取请求#64.
[N03]对视图方法的外部调用不是静态调用
在大部分代码库中,协议通过 OpenZeppelin 显式进行外部调用 functionStaticCall
当状态改变不是预期的或不需要的时,限制状态改变的可能性的方法。 然而,在 ChainlinkCalculator
合同,尽管意图仅向外部调用 view
Chainlink 预言机上的方法,外部调用 singlePrice
和 doublePrice
函数不是通过显式创建的 staticcall
s.
虽然我们没有发现由此产生的任何直接安全问题,但为了减少攻击面、提高一致性并澄清意图,请考虑使用显式 staticcall
s,对于所有外部调用 view
中的功能 ChainlinkCalculator
合同。
更新: 不固定。 1inch 团队表示:
我们认为语法复杂化会抵消一致性的改进。
[N04] 不会因无效订单而提前失败
在 OrderMixin
合同 fillOrderTo
函数处理先前未提交订单时的特殊情况(remainingMakerAmount == 0
),但是当订单不再有效时,它不会显式处理条件(remainingMakerAmount == 1
).
在后一种情况下,该功能最终会恢复,但只有在燃烧大量气体后才会恢复。 为了阐明意图、提高可读性并减少 Gas 使用量,请考虑在函数开始时显式处理无效订单场景。
更新: 固定在 拉取请求#68.
[N05] 辅助合约未标记为摘要
在 Solidity 中,关键字 abstract
用于本身不是功能性合同或不打算用作功能性合同的合同。 反而, abstract
合约被系统中的其他合约继承,创建独立的功能合约。
在整个代码库中,有一些辅助合约的示例没有标记为抽象,尽管它们并不意味着要单独部署。 例如, AmountCalculator
, ChainlinkCalculator
, ImmutableOwner
, NonceManager
及 PredicateHelper
合约都由一组基本函数组成,这些函数旨在由继承合约使用。
考虑将辅助合约标记为 abstract
明确表示它们的设计仅仅是为了向继承它们的合约添加功能。
更新: 不固定。 1inch 团队表示:
这些助手可以单独部署。 它们只是为了节省燃气而被继承。
[N06]函数顺序不一致
在整个代码库中,函数顺序通常遵循 Solidity 风格指南中的推荐顺序,这是: constructor
, fallback
, external
, public
, internal
, private
.
然而,在 OrderMixin
合同 public
checkPredicate
函数偏离了风格指南,将 external
功能。
为了提高项目的整体易读性,请考虑按照 Solidity 风格指南的建议,在整个代码库中标准化函数排序。
更新: 固定在 拉取请求#69.
[N07]订单填写流程不一致
OrderMixin
和 RFQOrderMixin
合约都处理已签署订单的填写,但是两个合约之间的总体订单流不一致。
OrderMixin
的 fillOrderTo
函数遵循以下一般流程(伪代码):
if ((takingAmount == 0) == (makingAmount == 0))
else if (takingAmount == 0)
else (handle makingAmount == 0) THEN swapTokens
而 RFQOrderMixin
类似的 fillOrderRFQTo
函数遵循以下流程(伪代码):
if (takingAmount == 0 && makingAmount == 0)
else if (takingAmount == 0)
else if (makingAmount == 0)
else revert THEN swapTokens
文档中没有解释为什么这两个函数中的第一个条件不同,或者为什么 takingAmount
和 makingAmount
在后一个函数中不能同时为零。 另外,如果两者都是 makingAmount
的网络 takingAmount
提供的内容更容易推理 fillOrderRFQTo
函数,因为它在最终的处理中被明确处理 else
块。
为了阐明意图并提高代码的整体可读性,请考虑标准化这两个合约的一般订单流,或明确记录存在差异的原因。
更新: 不固定。 1inch 团队表示:
这是由于限价订单中的自定义定价功能造成的。 自从
getMakerAmount
可能与getTakerAmount
,我们认为最好不要为接受者设置默认选项,因为在这些获取者不同的情况下,这可能会让他们感到困惑。
[N08] 错误消息格式不一致或具有误导性
在整个代码库中, require
和 revert
错误消息旨在通知用户导致事务失败的特定情况,但被发现格式不一致。
例如,行中的每一条错误消息 85的 OrderMixin.sol
, 16的 ERC721ProxySafe.sol
及 26的 Permitable.sol
采用不同的风格。
此外,一些错误消息具有误导性:
错误消息旨在通知用户有关失败情况的信息,因此它们应该提供足够的信息,以便可以进行适当的更正以与系统交互。 缺乏信息的错误消息会极大地损害整体用户体验,从而降低系统的质量。 此外,格式不一致的错误消息可能会带来不必要的混乱。 因此,请考虑检查整个代码库以确保每个 require
和 revert
语句具有格式一致、准确、信息丰富且用户友好的错误消息。
更新: 部分固定在 拉取请求#81.
[N09]命名返回变量的使用不一致
命名返回变量的使用不一致 OrderMixin
合同。 部分功能 返回命名变量, 其他 返回显式值, 和别的 声明一个命名的返回变量但覆盖它 带有明确的 return 语句。
考虑采用一致的方法在整个代码库中返回值,方法是删除所有命名的返回变量,将它们显式声明为局部变量,并在适当的情况下添加必要的返回语句。 这将提高代码的明确性和可读性,并且还可能有助于减少未来代码重构期间的回归。
更新: 固定在 拉取请求#73.
[N10]订单哈希计算未开放API
external
功能 remaining
, remainingRaw
和 remainingsRaw
所有人都期望成功操作的订单哈希值。
然而,辅助函数 _hash
,它返回订单的哈希值,有 private
能见度。 这意味着用户必须手动打包部分订单和域字符串才能获得订单的哈希值。
为了避免计算订单哈希值时出现错误并为用户提供生成订单各自哈希值的方法,请考虑扩展订单哈希值的可见性 _hash
发挥作用 public
并将名称重构为 hash
与代码的其余部分保持一致。
更新: 固定在 拉取请求#74.
[N11]映射的语义重载
_remaining
映射在 OrderMixin
合约在语义上被重载,以跟踪订单的状态以及这些订单可用的剩余资产数量。
它可以采取的三种状态是:
0
:尚未看到订单哈希。1
:订单已被取消或完全成交。- 不限
uint
比大1
: 剩余的makerAmount
可在订单上填写加上1
.
这种语义重载需要在期间包装和解开该值 lookup
, cancellation
, initialization
及 storage
.
语义重载和实现它所需的逻辑很容易出错,并且会使代码库更难理解和推理,它也可能为未来代码更新中的回归打开大门。
为了提高代码的可读性,请考虑在单独的映射中跟踪订单的完成状态。
更新: 不固定。 1inch 团队表示,修复该问题会增加 Gas 成本 fillOrder
功能。
[N12] 获得许可的订单允许调用任意合约
OrderMixin
合约继承了 Permitable
合约允许用接受此类的资产填写单笔交易订单 permit
呼吁修改津贴。
然而,本 调用 Permitable
合同 不要验证目标是否是允许的资产,甚至是否是资产,这可能允许恶意用户传递任意合约的地址,该合约可以在订单填写完成之前执行另一个调用。
虽然合同是 防止重入,始终建议减少攻击面并防止在执行过程中调用外部合约。 考虑将许可证中涉及的资产限制为订单中涉及的资产或协议的资产白名单。
更新: 不固定。 1inch 团队表示:
OrderMixin
实际上没有关于实际令牌的信息makerAsset
和takerAsset
有时是代理或其他中间合约,有关实际代币的信息存储在一些任意字节中。 所以没有可行的方法来限制哪些资产permit
被召唤。
[N13] solhint
永远不会重新启用
在整个代码库中,有几个 solhint-disable
声明,特别是网上的声明 23 并在线 41 of RevertReasonParser.sol
,没有终止于 solhint-enable
.
赞成明确性并在禁用时尽可能限制 solhint
,请考虑使用 solhint-disable-line
or solhint-disable-next-line
相反,类似于线 16 同一文件的。
更新: 固定在 拉取请求#72.
[N14]错别字
代码库包含以下拼写错误:
另外该项目的 README
(超出本次审核的范围)包含以下拼写错误:
考虑更正这些拼写错误以提高代码的可读性。
[N15] 使用 uint
而不是 uint256
为了明确起见,所有实例 uint
应声明为 uint256
。 特别是,那些在 for
在线循环 98 和 119 of OrderMixin.sol
和线条 16 和 30 of PredicateHelper.sol
.
更新: 固定在 拉取请求#70.
结论
发现 3 个高严重性问题。 提出了一些更改,以遵循最佳实践并减少潜在的攻击面。
- &
- 7
- 关于
- ACCESS
- 根据
- 账号管理
- 横过
- 法案
- 行动
- 额外
- 地址
- 优点
- 所有类型
- 允许
- 已经
- 尽管
- 量
- 分析
- API
- 的途径
- 参数
- 围绕
- 财富
- 办公室文员:
- 审计
- 后端
- 开始
- 作为
- 最佳
- 最佳实践
- 位
- 机器人
- 建立
- 呼叫
- 关心
- 例
- 原因
- 链环
- 更改
- 检查
- 支票
- 码
- 复杂
- 流程条件
- 混乱
- 施工
- 包含
- 合同
- 合同的
- 矫正
- 成本
- 可以
- 情侣
- 创造者
- 货币
- 电流
- data
- 处理
- 拒绝服务
- 部署
- 设计
- 开发
- 研发支持
- DID
- 不同
- 不同
- 域
- 翻番
- 动态
- 早
- 边缘
- 鼓励
- 特别
- ETH
- 活动
- 事件
- 一切
- 例子
- 交换
- 预期
- 体验
- 利用
- 特征
- 字段
- 终于
- 姓氏:
- 固定
- 高度灵活
- 流
- 遵循
- 发现
- ,
- 功能
- 资金
- 未来
- 游戏
- 天然气
- 其他咨询
- 给予
- 大
- 指南
- 处理
- 哈希
- 散列
- 有
- 帮助
- 高
- 高度
- 创新中心
- HTTPS
- 杂交种
- 鉴定
- 影响力故事
- 实施
- 重要
- 输入
- 包括
- 包含
- 增加
- 增加
- info
- 信息
- 基础设施
- 可行的洞见
- 意图
- 兴趣
- 接口
- 参与
- 问题
- IT
- 大
- 大
- 铅
- 领导
- 自学资料库
- 有限
- Line
- 流动性
- 已发布
- 书单
- 本地
- 看着
- 查找
- 主要
- 制造者
- 制作
- 市场
- 内存池
- 镜面
- 模型
- 最先进的
- 最受欢迎的产品
- 亦即
- 新功能
- 不可替代
- 不可替代的代币
- 官方
- 打开
- 运营
- 附加选项
- 神谕
- 秩序
- 订单
- 其他名称
- 业主
- 模式
- 热门
- 当下
- 预防
- 车资
- 价格
- 私立
- 过程
- 项目
- 项目
- 保护
- 协议
- 提供
- 提供
- 代理
- 国家
- 发布
- 采购
- 质量
- 提高
- 现实
- 减少
- 信赖
- 报告
- 业务报告
- 知识库
- 岗位要求
- REST的
- 成果
- 回报
- 检讨
- 风险
- 轮
- 运行
- SDK
- 保安
- 特色服务
- 集
- 共用的,
- 分享
- 转移
- 类似
- 简易
- 小
- 智能
- 智能合同
- So
- 坚固
- 垃圾邮件
- 特别是
- 花费
- Spot
- 开始
- 州/领地
- 个人陈述
- 州
- Status
- 存储
- 样式
- 提交
- 成功
- 成功
- 顺利
- SUPPORT
- 支持
- 磁化面
- Switch 开关
- 系统
- 目标
- test
- 测试
- 测试
- 通过
- 始终
- 次
- 一起
- 象征
- 令牌
- 跟踪时
- 跟踪
- 交易
- 信任
- 独特
- 最新动态
- us
- 可用性
- USD
- USDT
- 用户
- 效用
- 折扣值
- 查看
- 能见度
- 等待
- 什么是
- 白名单
- 中
- 也完全不需要
- 工作
- 价值
- 零