ZeppelinOS Smart Contracts Audit

Helping build the future of smart contract infrastructure.

Few teams know the importance of having a fresh set of eyes on new smart contract code better than Zeppelin. With the launch date for their flagship offering approaching, Nomic Labs was asked to conduct a source code audit of the ZeppelinOS smart contracts.

Summary

We found the code to be of very high quality, well documented and backed by a very thorough suite of automated testing.

The code is structured around two main components: zOS-lib and the Kernel.

The zOS-lib lies at the core of the platform. It’s responsible for providing developers the upgradeability support and version management functionality.

On the other hand, the ZeppelinOS kernel is responsible for registering different versions of the system’s standard library, handling the governance mechanism around these versions, and rewarding contributors for their work.

Across the two components we found:

• No critical issues.
• 1 high severity issues
• 1 medium severity issues
• 4 low severity issues

It’s worth highlighting that the one high severity issue we found is present in the state-of-the-art upgradeability approach being used across the industry, and most projects implementing upgradeability were also affected in some way. Read our educational post explaining this finding.

The fixes made by the Zeppelin team based on our findings can be found in this pull request on Github.

Full report

zOS-lib audit

The audit was conducted on version 0.1.5, which can be found on GitHub.

Critical issues

No critical issues were found.

High severity issues

UpgradeabilityProxy is vulnerable to function selector clashing
zos-lib implements upgradable contracts using the Unstructured Storage Proxy Pattern. Like every other Proxy Pattern, it’s based on forwarding messages to an implementation contract using DELEGATECALL.

This forwarding logic is implemented in the Proxy’s fallback function, which is only called if the current message’s data doesn’t start with one of the Proxy’s functions’ selectors. This means that if the selector of one of the implementation’s functions clashes with one of the Proxy’s, the latter will take precedence and the message will not be forwarded.

This behavior can possibly break contracts, but it can also be abused by an user of zos-lib to conceal malicious code. In the case of ZeppelinOS, this can be used for proposing backdoored library upgrades.

You can read our in-depth explanation of this vulnerability in this post.

Update: The Zeppelin team has already implemented a fix, which restricts the Proxy’s functions only to its owner, forwarding every message that comes from other users to the implementation contract.

Medium severity issues

No medium severity issues were found.

Low severity issues

Initializable contracts do not work with multiple inheritance

If a contract has more than one Initializable direct parent, it will not be able to call both initializers due to the handling of the initialized variable.

Update: The Zeppelin team was already aware of this issue, and they’re planning to implement a new initialization mechanism soon. In the meantime, documentation about this has been added.

ContractDirectory#setImplementation doesn’t check if the implementation is a contract

This function can be called with any valid address, including the null address and externally owned accounts, which can lead to a corrupted ContractDirectory.

Update: Fixed in pull request #102.

Migrations may be applied in arbitrary order

Migratable is used to implement upgradable contracts’ state updates in-between versions. It lets developers implement migrations that can require for the execution of another specific migration to have happened first.

If two migrations depend on the same one they can be applied in any order. This can corrupt the contract’s state, as the first migration may have altered it in ways unpredicted by the second migration’s author.

Update: The Zeppelin team is planning to implement a new initialization mechanism soon. In the meantime, documentation about this has been added.

BaseAppManager#upgradeTo and BaseAppManager#upgradeToAndCall can be called with an incorrect contract name

These methods are used to upgrade proxy contracts and take as inputs the name of the implementation these should be upgraded to. This name is never checked to be correct, and if it isn’t, the contract will almost certainly be corrupted.

Update: This functions are intended to be called using zOS’s CLI, which validates this. Future on-chain validation is being evaluated.

Comments and recommendations

• Proxy’s fallback function should have extern visibility. Note that this will be mandatory in the next major release of Solidity. This function can also be simplified, as there’s no need to manage the free memory pointer inside of its inline assembly block; and optimized, as it always has a call to keccak, which comes from solc inlining its implementationSlot constant field.
• Address members won’t be included in contracts in future versions of Solidity, and explicit casting to address will be needed. There are instances of these being used in OwnedUpgradeabilityProxy and UpgradeabilityProxyFactory.
• OwnedUpgradeabilityProxy#setUpgradeabilityOwner’s name is inconsistent with the rest of the contract’s functions. It should be setProxyOwner.
• Package#getVersion reverts if the requested version is not available. This is the only getter in zos-lib with this behavior, which can be confusing. The other getters return address(0) to signal “not available”.
• Consider using pragma experimental “v0.5.0”, as it’s been recommended by Solidity since version 0.4.21. This pragma opt-ins to upcoming breaking changes. In particular, it makes use of STATICCALL for calling externalpure and view functions, which makes a whole class of attacks impossible.

Kernel audit

The audit was conducted on version 0.1.3, which can be found on GitHub.

Critical issues

No critical issues were found.

High severity issues

No high severity issues were found.

Medium severity issues

Kernel#register accepts any contract, not just Release
When developers want to propose a new version of the standard library, they must register a new release, which is represented by a contract.

While the Kernel contract declares the release to be of type Release, this information is only available at compile time, and used to type check the contract, not to validate the received addresses in runtime. This gives attackers the ability to craft backdoored releases, which could potentially compromise the system.

One possible solution to this issue would be to create the Release contracts inside the Kernel, and validate that only those can be registered.

Update: The Zeppelin team informed us that they are aware of this behavior, which is intended to give developers more flexibility when creating new releases. To remedy the possibility of it being used to exploit the zOS users, token holders will be instructed to review the release’s contract, and not just the proposed changes to the standard library.

Low severity issues

No medium severity issues were found.

Comments and recommendations

• The Vouching contract has fields whose names start with an underscore, which is normally reserved for parameters, and can be confusing.
• Kernel#developerFraction is hard to use and may not be granular enough for its needs. Consider expressing this proportion in parts-per-million (PPM) or a similar notation.
• Kernel#_payoutAndVouch forces the payout to the developer to be at least 1 fraction of ZEP (ie: 1E-18 ZEP). We believe this is not necessary, as the gas cost for transferring such a small amount of ZEP will be prohibitive. Moreover, this function is implemented in such a way that no fraction of ZEP will be lost if this requirement is removed.
• The examples’ implementation of ERC721Token is outdated and should use Migratable.
• Also consider using pragma experimental “v0.5.0 in this project.

Conclusion

One high severity issue found. Some changes were proposed to reduce potential attack surface, and the Zeppelin team has applied the fixes described above.