The ENS audit results are in!

Nick Johnson
Apr 6, 2017 · 3 min read

As you might be aware, we’ve been working hard on ENS in preparation for a relaunch. One of the things we’ve been doing is getting the code thoroughly audited, as not doing a formal audit was an issue identified by the postmortem of the first launch. Two auditors did the work, Martin Holst Swende and Piper Merriam.

I’m happy to say that they’ve both now finished their audits. Martin’s audit report can be found here, and Piper’s is here (signed version here).

Both auditors identified several minor issues, and no major ones. I’ll go through the findings of each briefly below, and outline how we intend to respond to them.

  • Registrar.transfer naively calls ens.setSubnodeOwner (medium)- Martin and Piper both observed that the transfer operation will not function on a registrar that is no longer active. Martin further observed that a registrar does not immediately deactivate when a replacement registrar is deployed.
  • Missing user protection (low) — There are a couple of situations where users could accidentally cause problems for themselves that are easily checked and avoided, such as transferring ownership to 0.
  • User inactivity causes loss of funds (low) — A user will lose their bid if they fail to reveal during the specified period.
  • Loss of funds due to forgotten salt (low) — This is another consequence of the auction process.
  • Old registry does not become inactive (low) — Martin discusses the process by which upgrades work, which does not immediately disable all functions of the outgoing registrar.
  • Character set ambiguity (low) — Martin discusses character set and permitted character issues, and the fact that the registrar cannot check these directly.
  • Deed.setBalance should not be payable (info)
  • ENS.lll should revert instead of bad jump (info)
  • Deed factory coupling (medium) — There is tight coupling between the Deed and Registrar contracts, and suggests using the factory pattern to avoid this.
  • Implement Registrar.trySetSubnodeOwner function (medium) — Piper suggests a refactoring to remove some redundant code.
  • Registrar.returnDeed function will always throw. (minor) — The returnDeed function is intended to return ether locked up during the original launch of the ENS. Given the small amount of locked up ether, this function should be removed.
  • Registrar contract uses the entry.highestBid and entry.deed variables to derive secondary information about the state of the entry (medium)
  • Multi-line if statements without braces (medium)

Actions

All of the code issues identified by the auditors will be addressed before deploying the updated contract, with the exception of two of Piper’s suggested refactors, which we believe are substantial enough that they would result in a significant rewrite of the contract, introducing additional risk. Instead, we will aim to improve those in a post-deployment refactoring pass.

Several process/communication issues, such as communicating reveal periods, were also highlighted; we will write documentation and UI text to make these constraints clear to users and implementers.

Conclusion

Both auditors identified a couple of situations in which the code could be improved to prevent user error and to make it easier to read and maintain, but neither found significant issues with the contract itself. Given that, we feel comfortable proceeding with a launch in the near future. Stay tuned for further announcements!

The Ethereum Name Service

The official source for news related to the Ethereum Name Service (ENS). Follow this publication for the latest ENS developments.

Thanks to Alex Van de Sande.

Nick Johnson

Written by

The Ethereum Name Service

The official source for news related to the Ethereum Name Service (ENS). Follow this publication for the latest ENS developments.