ZeppelinOS Smart Contracts Audit IV
Our fourth audit report on ZeppelinOS
Changes to the code since our previous audit
- Some OpenZeppelin contracts have been imported into the project and renamed
- The Initializable contract has been optimized
- Migrated to Solidity 0.5
- The ProxyFactory contract was added
- The InitializableAdminUpgradeabilityProxy contract added
ProxyFactory#deploy is not trustless with respect to the sender
As discussed with members of the zOS and LevelK teams, the
ProxyFactorycontract requires users to trust the caller of
ProxyFactory#deploy.Precomputing an address doesn’t prevent the sender to deploy a contract with any implementation and admin into it.
Note that the contract can’t be deployed by anyone, as the deployer is defined when computing the address. This makes the contract secure in a scenario where the deployer is the same user that computed its address.
If the intention of the contract is for proxies to be deployed by someone else (e.g. in a meta-transactions scenario), the current implementation would be insecure. In this case, using the implementation and its initialization parameters to compute the address is required. In that case, there’s no need to define a specific sender, as the contract would be fully defined as long as the initialization code doesn’t use
One possible implementation is using
AdminUpgradeabilityProxy instead of
InitializableAdminUpgradeabilityProxy, by appending the constructor parameters to the
creationCode before computing the address and calling
create2. If this is implemented, consider removing
InitializableAdminUpgradeabilityProxy to simplify the system.
Update: the function
deploySigned was added to deal with this.
Update: all of the following comments have been addressed by the Zeppelin team.
assertRevert never fails
This helper function is supposed to fail if a transaction doesn’t revert. Its implementation is incorrect and never fails.
If the transaction doesn’t revert an exception containing the “revert” string is thrown, which could easily be confused with an actual transaction revert.
[zOS4-O02] Incorrect test for ProxyFactory
This test is incorrect, it should use
salt1 in both calls. This error was unnoticed because of [zOS4-O01].
[zOS4-O03] Failing test in Transactions.test.js
This test fails once [zOS4-O01] is corrected.
[zOS4-O04] InitializableAdminUpgradeabilityProxy is not tested
There are no tests for this contract. In particular, its initialization is untested.
[zOS4-O05] Outdated comment in ImplementationProvider
This comment is outdated. Now
ImplementationProvider is an abstract contract, not an interface.
Get a high-quality smart contract audit from Nomic Labs.