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.
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.
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.setBalanceshould not be payable (info)
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.
Registrar.trySetSubnodeOwnerfunction (medium) — Piper suggests a refactoring to remove some redundant code.
Registrar.returnDeedfunction 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.
Registrarcontract uses the
entry.deedvariables to derive secondary information about the state of the entry (medium)
ifstatements without braces (medium)
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.
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!