Nomic Labs
May 28 · 2 min read

Summary

We conducted a fourth security audit on the ZeppelinOS smart contracts. The audited version is commit e962de452a74c9385181b64a24f1936de8d41432 and we found no security issues. The fixes made by the Zeppelin team based on our findings can be found in this commit on Github.


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 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 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

This comment is outdated. Now ImplementationProvider is an abstract contract, not an interface.


Get a high-quality smart contract audit from Nomic Labs.

Nomic Labs

We design, build and audit decentralized systems.

Nomic Labs

Written by

We design, build and audit decentralized systems.

Nomic Labs

We design, build and audit decentralized systems.

Welcome to a place where words matter. On Medium, smart voices and original ideas take center stage - with no ads in sight. Watch
Follow all the topics you care about, and we’ll deliver the best stories for you to your homepage and inbox. Explore
Get unlimited access to the best stories on Medium — and support writers while you’re at it. Just $5/month. Upgrade