InstaDApp审计

关于InstaDApp

InstaDApp是一个独立的银行业务的门户网站，它运行在基于区块链的新兴金融协议(如MakerDAO、Uniswap或Compound)之上。它的使命是简化银行业务的日常需求，比如贷款、放贷、交换代币和杠杆持仓。

InstaDApp在其他DeFi协议之上提供了一个接口。它的目标用户群是缺乏高端技术或财务经验的用户。它帮助用户管理CDP，包括提供清除或向第三方CDP添加抵押品的能力，通过电子邮件或电报跟踪CDP活动，以及通过捆绑的交易节省gas燃料。例如，使用CDP的用户可以通过一个交易来提取Dai、购买ETH和锁定ETH。

对于每个用户，InstaDapp创建一个唯一的智能合约——称为“UserWallet”。这个钱包合约允许用户从逻辑合约执行外部代码(使用delegatecall)，这些逻辑合约已经被InstaDapp管理员设入白名单。这样，即使在创建了钱包之后，也可以添加新功能。

为了保存现有钱包和白名单逻辑合约的记录，InstaDapp使用InstaRegistry合约。此合约的管理员具有“启用”或“禁用”逻辑合约的能力(即从白名单中添加或删除逻辑合约的能力)。逻辑合约可以通过以下两种方式之一进行白名单:它们可以被归类为标准逻辑合约，或者它们可以被归类为“静态”逻辑合约。

如果一个逻辑合约被归类为“静态”逻辑合约，那么它就永远不能从白名单中删除。这具有重大的安全影响。特别是，用户从InstaDapp中提取资产所需的任何功能都应该被归类为“静态的”，因为这确保了即使在InstaRegistry管理员变成恶意的或妥协时用户可以始终退出InstaDapp。

InstaRegistry合约还允许任何人创建新的钱包(通过它两个build函数中的一个)。它为用户钱包的所有者提供了将所有权转移到另一个帐户的功能。它还赋予InstaRegistry合约的管理员更改InstaRegistry管理地址和注册其他特权角色的能力。

审计的背景和范围

InstaDapp是以太坊主网上的一个在线项目。我们审计的代码是地址位于0x498b3bfabe9f73db90d252bcd4fa9548cd0fd981，已通过Etherscan验证。这包括两个顶级合约:UserWallet和InstaRegistry。我们审计了这两个合约和他们继承的合约。

InstaRegistry合约用于部署新的UserWallet实例，跟踪它所部署的UserWallet实例的所有权，以及批准/禁用其部署的能被UserWallet实例使用的逻辑代理合约。它无意图持有任何有价值的资产。

UserWallet合约是唯一一个意图持有任何有价值资产的合约。UserWallet合约是使用代理模式构建的，该模式允许使用InstaRegistry合约批准的代理逻辑合约去扩展它们的功能。

我们没有将审计任何逻辑代理合约作为本次审计的一部分。由于代理逻辑合约可以在UserWallet的所有者调用时操作UserWallet合约的状态(包括从钱包中转移有价值的资产)，因此我们建议在用户调用任何代理逻辑合约之前，也要对它们进行审计。

Etherscan验证的代码与4863c0c4156af7ded9cdb38b66e5f5e527c4a6d0提交中的UserWallet.sol和InstaRegistry.sol的代码相同。

范围

范围中的合约有：

RegistryInterface (interface)AddressRecordUserAuthUserNoteUserWalletAddressRegistryLogicRegistryWalletRegistryInstaRegistry

超出范围

下面的合约明确不在范围内：

任何的逻辑代理合约——静态或其他。这包括目前在以太坊主网上使用的任何逻辑合约，以及计划将来使用的任何逻辑合约。在InstaDApp管理员启用或由InstaDApp用户使用任何逻辑合约之前，它们将与InstaDApp一起都应该经过单独的审计。除了我们在上面明确列出的那些可在InstaDApp Github仓库中找到的任何合约。Uniswap, Compound, MakerDAO,或其他 DeFi项目代码。任何旧版本的InstaDApp项目。

发现

我们在这里分享我们的发现和建议。

快速的澄清说明。WalletRegistry合约有两个共享相同名称的单独函数:build()函数和build(address _owner)函数。这两个函数没有相同的参数类型，因此不存在函数签名冲突的风险。但是，如果我们含糊地提到“build函数”，这会使本报告的读者感到困惑。为了避免这种混淆，我们将在引用这些函数时始终包含输入参数，从而消除歧义。也就是说，当引用前一个build函数时，我们将使用build();当引用后一个build函数时，我们将使用build(address _owner)。

严重风险

无

高风险

无

中风险

钱包所有权转移被拒绝服务——方式一

一个活跃的攻击者可以阻止钱包的所有者转移钱包的所有权。攻击过程如下:

攻击者可以使用调用build(X)的交易抢先运行所有调用setOwner(X)的交易。

这使得在处理setOwner(X)交易时prixies[X]不等于UserWallet(0)，从而导致第139行上的require语句回滚。结果是攻击者阻止了钱包所有权的转移。

可以考虑限制对build(address _owner)函数的访问，以便只能从InstaRegistry地址和(可选的)一组白名单内的第三方地址调用该函数。

钱包所有权转移被拒绝服务——方式二

即使对build(address _owner)的调用是受限制的(正如在"钱包所有权转移被拒绝服务——方式一"中提到的)，攻击者也可能阻止钱包的所有者转移所有权。第二种防止所有权转移的方法如下。

攻击者可以使用一个交易抢先运行setOwner(X)组合的每个交易，该交易原子性地执行以下两个函数:

1.调用build()来创建攻击者控制的新UserWallet实例。

2.通过在攻击者的新UserWallet实例上调用setOwner(X)，将新UserWallet实例的所有权转移到地址X。

(通过创建一个在单个函数调用中执行两个函数的利用合约，可以在单个交易中原子地执行这两个操作。)

和前面一样，这导致在处理setOwner(X)交易时proxies[X]不等于UserWallet(0)—导致InstaRegistry的第139行发生回滚，从而阻止诚实的用户转移所有权。

可以考虑实施“主动接受所有权”计划，要求新提出的所有者主动接受所有权。例如，所有权转让将分两步进行，其中:

1.当前所有者设置一个“待定所有者”。

2.“待定所有者”调用一个函数(即acceptOwnershipTransfer())来索取所有权。

有关此模式的审计范例，请参见Micah Zoltu的可恢复钱包合约。

这个主动接受所有权方案将阻止这种拒绝所有权转移的方式，以及防止没有经验的用户将他们的钱包所有权转移到一个不活跃的地址(或一个不具备与Userwallet交互功能的合约)而导致意外烧毁Userwallet。

管理员/所有者角色可能被误销毁

如果InstaRegistry合约的管理员或所有者在没有显式输入_userAddress参数(与合约交互时常见的用户错误)的情况下错误地调用setAddress函数，一些前端可能会解析丢失的address参数，以零地址作为_userAddress参数进行传递。这将导致管理员或所有者错误地销毁他们的管理员/所有者权限。

可以考虑在setAddress函数中添加一条require语句，如果_userAddress参数是零地址，该语句将回滚。这样可以防止意外销毁管理员/所有者权限。(仍然有可能故意烧毁管理员/所有者权限)

无文档的汇编块

UserWallet合约包括两个无文档的汇编块。第一个在note修饰符中，第二个在excecute函数中。

虽然这本身不会造成安全风险，但它们是系统的关键部分，应该更好地记录下来。对于汇编块，我们强烈建议使用代码注释解释每一行汇编命令。这有助于提高这些代码关键部分的可读性和可理解性。

还请注意，使用汇编丢弃了几个重要的安全特性，这可能使代码更不安全，更容易出错。

可以考虑实现全面的测试来覆盖这些函数的所有潜在用例，以确保它们的行为符合预期。

日志记录一个内存指针而不是实际的数据

UserNote合约实现了note修饰符，它在调用execute函数时记录信息。在修饰符内部，有一个assembly块，它将来自交易的信息存储在本地变量foo和bar中，然后在LogNote事件中发出这些信息。

bar变量是通过bar:= calldataload(36)设置的，它包含数据在内存中的位置，而不是实际数据本身。如果目的是记录实际数据，而不仅仅是记录数据的位置，那么可以考虑更改这段代码并编写测试，以确保它的行为符合预期。

各种NatSpec问题

在代码中有有很多与NatSpec文档相关的问题。我们在此将其中几个问题作为一个问题列出。我们认为NatSpec文档是合约公共API的一部分，因此与NatSpec相关的问题比其他代码注释相关的问题具有更高的严重性。

AddressRegistry合约上的@notice和@dev标记为空。LogicRegistry合约上的@notice标记为空。@dev标记在LogicRegistry合约上有重复@title标记。logic函数的@dev标记为空/没有解释logic函数的作用。logicStatic函数的@dev标记为空/没有解释logicStatic函数的作用。用户钱包constructor函数的@dev标记是不正确的/有误导性的。record函数的@dev标记包含一个拼写错误。isAuth修饰符的NatSpec应该有一个@return标记。execute函数的NatSpec应该有一个@return标记。

可以考虑删除空标记，在需要的地方添加@return标记，使用@dev标记来描述合/函数的预期用途，否则标明上面列出的问题。有关更多信息，请参见Solidity文档。

低风险

钱包创建交易可能失败-方式一

攻击者可能导致所有诚实用户的钱包创建交易失败。诚实用户最终仍然会拥有一个他们可以控制的钱包，但是他们的钱包软件(例如MetaMask)将显示一个错误，说明他们的钱包创建事务不成功的(回滚的)。攻击过程如下:

攻击者可以提前运行所有对于build()或build(address _owner)的调用，并自己调用build(address _owner)，将_owner参数设置为诚实用户的地址。这将导致诚实用户的交易在InstaRegistry.sol的第127行被回滚。

诚实用户最终仍将拥有一个他们控制的功能钱包，因为攻击者的交易将为他们创建钱包。然而，失败的交易消息可能会让不熟悉交易的用户感到困惑，也可能会增加前端开发人员处理这种情况的复杂性。

可以考虑限制对build(address _owner)函数的访问，以便只能从InstaRegistry地址和(可选的)一组白名单中第三方地址调用该函数。

钱包创建交易可能失败-方式二

即使调用build(address _owner)是受限制的(如“钱包所有权转移被拒绝服务——方式一”和“钱包创建交易可能失败-方式一”中所述)，攻击者也可能导致所有诚实用户的钱包创建交易失败。和之前一样，诚实用户最终还是会得到一个他们可以控制的钱包，但是他们的钱包软件(比如MetaMask)将显示一个错误，说明他们的钱包创建交易是不成功的(回滚的)。第二种导致钱包创建交易失败的方法如下。

攻击者可以使用执行以下两种操作的单个交易抢先运行对build()或build(address _owner)的所有调用:

1.调用build()来创建攻击者控制的新UserWallet实例。

2.通过在攻击者的新UserWallet实例上调用setOwner(address nextOwner)并将诚实用户的地址作为nextOwner参数传递，从而将攻击者的新UserWallet实例的所有权转移到诚实用户的地址。

(通过创建一个在单个函数调用中执行两个函数的利用合约，可以在单个事务中原子地执行这两个操作。)

诚实用户最终仍将拥有一个他们控制的功能钱包，因为攻击者的交易将创建一个钱包并将所有权转移给诚实用户。然而，失败的交易消息可能会让不熟悉交易的用户感到困惑，也可能会增加前端开发人员处理这种情况的复杂性。

为了防止这种情况发生，可以考虑实施“钱包所有权转移被拒绝服务——方式二”问题中建议的“主动接受所有权”方案。

UserWallet所有者可能会误销毁所有权

用户常犯的一个错误是在调用函数时忘记传递函数的一个或多个参数的显式值。当发生这种情况时，丢失的输入可能被解析为其给定类型的默认未初始化值。例如，当通过Remix调用函数时，如果没有提供uint256参数，就会导致uint256参数被解析为0。

如果用户在调用UserWallet的setOwner函数时犯了这种错误，他们可能会通过将所有权分配给零地址而误销毁钱包的所有权。虽然这种情况最多只会发生在一个用户身上，但这仍然是一个问题。

为了防止这种情况，可以考虑在setOwner函数中使用require语句来确保nextOwner参数不是零地址。

或者可以考虑实施“钱包所有权转移被拒绝服务——方式二”问题中建议的“主动接受所有权”方案。

第三种方法不需要重新部署任何在线合约，它将调用build(address _owner)，并将零地址作为_owner参数传递。这将为零地址创建一个新的UserWallet，这将使proxies[0]!= UserWallet(0)，这将防止任何未来的用户错误地将他们钱包的所有权转移到零地址。

特权角色只能有一个成员

AddressRegistry合约有一个名为registry的映射，它将角色(hash)映射到具有该角色的address。重要的是，这意味着每个角色只能有一个具有该角色的地址。

两个重要的角色是admin和owner角色，这是仅有的两个可以使用isAdmin修饰符访问函数的角色。setAddress函数具有isAdmin修饰符，可用于创建新角色。它还可以用来更改现有角色映射到的地址。任何角色的撤销都必须通过setAddress函数来完成——例如，将角色的地址设置为零地址。

这些访问控制按预期工作，但在每个角色只能有一个地址意味着是有限制的。admin和owner角色在现有合约中具有相同的权限。似乎拥有这两个不同的角色似乎是一种克服限制的方法，每个角色类型只能具有一个与之关联的地址。

可以考虑使用更灵活的角色管理模式，允许每种角色类型有多个地址，例如OpenZeppelin的角色库。您可以在MinterRole合约中看到如何使用它的示例。

当您希望具有多个具有相同角色的地址时，此方法将提供更大的灵活性。

proxies映射丢失钱包跟踪

UserWallet可以通过InstaRegistry的build函数创建，但最终不在proxies映射的映像中。

如果UserWallet的所有者使用InstaRegistry地址作为nextOwner参数来调用setOwner，就会发生这种情况。然后，下一次任何人再次调用build()或build(address _owner)函数时，初始的UserWallet将在InstaRegistry的第129行proxies映射中被覆盖，因此不再出现在proxies映射的映像中。

如果这被认为是不可取的行为，可以考虑要求record函数不能让它的_nextOwner参数等于InstaRegistry地址。或者，考虑实施“主动接受所有权”方案，该方案建议在“钱包所有权转移被拒绝服务——方式二”问题中。

硬编码剩余gas燃料

UserWallet合约的execute函数使用一个delegatecall给用户输入的逻辑代理合约。这似乎是MakerDAO的delagate调用模式的克隆。

在这个delegatecall中，gas参数被设置为sub(gas, 5000)。这个值的原因没有文档说明。这大概是为了确保有足够的剩余gas燃料(即至少5000)来完成汇编块的其余部分。这可能是一个有问题的方法，因为操作码的gas燃料成本可能会发生变化(请参见EIP 1884)，这可能会导致未来gas燃料不足的问题。

可以考虑将该值设置为sub(gas, minRemainingGas)，其中minRemainingGas可以是用户输入的参数，也可以是用户可设置的全局变量。

函数/修饰符/变量命名可以优化以提高可读性

一些变量、函数和参数的名称并不清楚地表示它们的用途。在UserNote合约中最为严重。为了提高可读性，可以考虑重新命名它们，以更清楚地反映它们的用途。我们的建议是:

在AddressRecord:

registry 改成 registryAddresslogicAuth 改成 isLogicAuthorized

在UserAuth:

LogSetOwner改成OwnerChangedauth改成onlyAuthorizedsetOwner改成changeOwnerisAuth改成isAuthorized

在UserNote:

foo改成targetAddressbar改成dataInputguy改成callerwad改成valuefax 改成datanote改成receipt

在AddressRegistry:

LogSetAddress改成UpdatedRoleregistry改成rolesRegistrygetAddress()改成getRoleAddress()setAddress()改成setRoleAddress()isAdmin改成hasPermission_name改成_roleName_userAddress改成_actorAddress

在LogicRegistry:

logicProxiesStatic改成staticLogicProxieslogic改成hasLogiclogicStatic改成hasStaticLogic

在WalletRegistry:

Created改成WalletCreatedLogRecord改成TransferedOwnershipproxies改成walletRegistryproxy改成wallet

隐式返回

一些返回值的函数显式地这样做。例如，logic和logicStatic函数都显式地返回它们的返回值。

但是，其他函数不会显式地返回它们的返回值。例如，build(address _owner)和record函数不使用显式的返回语句。

可以考虑对所有带有返回值的函数使用显式返回，而不是隐式返回。这有助于防止在代码随时间变化时可能发生的回退。

函数的可见性需要更多的限制

多个函数在限制为external的情况下是给予public可见性。我们建议尽可能限制函数的可见性，因为这样可以节省gas燃料并简化安全性分析。可以考虑将以下功能的可见性设置为external:

AddressRegistry合约的setAddress函数。UserAuth合约的setOwner函数。LogicRegistry合约中的每个函数。WalletRegistry合约的build函数。WalletRegistry合约的record。

注意

setOwner中的良性重入

UserWallet.sol的setOwner函数违反check -effect -interaction模式，如果registry地址曾经是恶意合约的地址，允许良性的重入。

在这种情况下，是没有问题的。但是，要遵循最佳的实践，可以考虑将对record函数的调用移动到setOwner函数的最底部。

错误的注释

InstaRegistry.sol的124行注释建议build(address _owner)函数在如果msg.sender不是由InstaRegistry合约创建的代理合约时抛错。然而，事实并非如此。这条注释似乎是为record函数准备的。

可以考虑将这个注释直接移到record函数的上面。

隐式变量的可见性

AddressRegistry合约中的registry映射变量是隐式public。可以考虑明确可见性以提高可读性。

不一致的参数指标

在大多数情况下，函数名和修饰符参数名用下划线表示。但在logicAuth, setOwner和isAuth中就不一样了。可以考虑保持一致可读性。

缺乏原生的ETH提现方法

UserWallet合约依赖于逻辑代理合约来执行ETH提取。可以考虑在UserWallet合约中提供一个原生的ETH-withdraw函数。否则，请考虑通过静态逻辑代理添加一个ETH-withdraw函数。

更新：InstaDApp团队已经通知我们，已部署的InstaRegistry合约已经批准了一个ETH-withdraw函数作为静态逻辑代理。我们没有审计这个ETH-withdrawal函数，因为它超出了范围。

贫乏的测试覆盖率

合约的测试覆盖率非常少。可以考虑编写更多的测试以获得更好的覆盖率。

避免使用类型别名

为了提高可读性，可以考虑在任何地方使用uint256而不是它的别名uint。

公共getter的重复

LogicRegistry合约中的logicStatic函数是不必要的，因为它重复了公共logicProxiesStatic的getter。可以考虑删除logicStatic函数并使用公共logicProxiesStatic映射的getter。

在函数定义中声明的返回变量

为了便于显示和可读性，最好在函数体中声明返回变量，而不是在函数定义中这样做。

可以考虑在函数体中声明输出response，并在代码流程完成后显式地返回变量。

Web3版本

web3目前作为一个依赖项包含在package.json文件作为“web3”:“^1.0.0-beta.37”。开发人员应该注意web3 v1.0.0-beta中引入的问题#2266。不接受自定义提供者的地方可能会在项目中引入意想不到的错误。

可以考虑将web3依赖关系固定到一个固定的版本上(例如"web3": "1.0.0-beta.37")或更新到最新版本，这个问题得到解决。

结论

没有发现严重或高风险的问题。一些修改已提议，以遵循最佳实践、提高可扩展性和减少潜在的攻击面。

总结

如果您对本次审计的非技术概述感兴趣，我们将在总结文章中提供与审计相关的系统摘要以及一些有趣的发现。

原文链接：https://blog.openzeppelin.com/instadapp-audit

最后提醒一下，市场有风险，本文只是个研究，不作为投资建议，请合理控制风险。

点赞就是对传教士最大的鼓励，谢谢支持。

—-

编译者/作者：DeFi传教士

玩币族申明：玩币族作为开放的资讯翻译/分享平台，所提供的所有资讯仅代表作者个人观点，与玩币族平台立场无关，且不构成任何投资理财建议。文章版权归原作者所有。