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

Audit Results

[zOS4-R01] ProxyFactory#deploy is not trustless with respect to the sender

As discussed with members of the zOS and LevelK teams, the ProxyFactory contract 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 msg.sender .

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.

Other comments

Update: all of the following comments have been addressed by the Zeppelin team.

[zOS4-O01] 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