- Section 1 - Overview
- Section 2 - General discussion
- Section 3 - Solidity Contracts Analysis
- 3.1 Unanticipated behavior for unrevealed bids on unowned auctions [medium]
- 3.2 Outdated or missing comments [minor]
- 3.3 Several functions have no natspec comments [minor]
- 3.4 the
destroyDeedfunction should beprivate[minor] - 3.5 The
max,minandstrLenfunctions should beprivaterather thaninternal[minor]
- Section 4 - Test Coverage Analysis
My primary objective with this review was to verify the correctness of the code. Observations are also made regarding code quality, test coverage, and best practices, though this was not the primary focus of my review.
Though there may be opportunities for further improvements through major refactorings, and architectural changes, I considered such discussion beyond the scope of this review.
The primary focus of the review was on the HashRegistrarSimplified.sol file, in the master branch of the ethereum/ens repository. The state of the source code at the time of the audit can be found under the commit hash f98280e9355a5fb074b89844137daffb8fa381d2.
This file contains two contracts, Deed and Registrar. The only external calls from these contracts are to the ENS Registry. I did not review the ENS code, and assumed it to be correct.
This review was conducted independently, and voluntarily, outside of my employment.
None of the issues discovered in my review (covered in Section 3) would be considered a "show stopper", and would be unlikely to cause significant issues during operation. Their presence does however make me feel generally concerned about the pace of launch.
The ENS code is included in the Ethereum Foundations bounty program, but not activity has been added aside from the two issues discovered on the Pi day launch. The tremendous value of community reviewers not already familiar with the code base was made clear during the initial launch, and my concern is that the ENS bounty has been under promoted.
Recommendation: Following the model of WeiFund's bug bounty, launch a well promoted campaign to encourage and incentivize community reviewers, allowing at least two weeks for the bounty.
Although outside the scope of my review, I must call attention to the already deployed ENS Registrar, which is written in Ethereum's LLL (low-level language).
LLL is a language with a very small community of practitioners, and I'm skeptical that it has been properly reviewed by anyone other than its author.
The Registry is the cornerstone of the ENS system, it's non-upgradeable, and completely mission critical.
Edit: after discussing with Nick, I understand that himself, the author (Daniel Ellison), and Martin Swende have reviewed the assembly code generated from the LLL implementation. I would still prefer to see more eyeballs on it.
The test suite outputs only the following for ENS.lll:
ENS.lll
✓ transfers ownership (48ms)
✓ prohibits transfers by non-owners
✓ sets resolvers (41ms)
✓ prohibits setting resolver by non-owners
✓ permits setting TTL (44ms)
✓ prohibits setting TTL by non-owners
✓ creates subnodes (50ms)
✓ prohibits subnode creation by non-owners
Gas report for ENS.lll: 79022
By comparison, for ENS.sol the gas report value 80034. I do not see how the small gas savings are worth any additional risk.
This is a rather minimal test suite. Although it tests each of the state changing functions twice for both valid and prohibited outcomes, it might be help to have some "maliciously oriented" tests written, even if they were redundant.
Edit: Nick has published the ENS Registry's disassembly code, with some annotations for public review.
Recommendation: I am not sufficiently familiar reviewing the disassembly code to assess it's quality or security. I urge the team to:
- carefully consider the pros and cons of a Solidity vs. LLL implementation,
- explain the rationale publicly,
- incentivize as much review of the ENS Registry as possible.
The following are findings from manual inspection of the code.
When the unsealBid() function is called, the registrar burns 99.5% of the value if a bid is revealed for an Owned hash. This is to prevent an extortion attack on the current highest bidder, by threatening to reveal a new second highest bid. The burn mechanism force bids to be revealed, or lose all funds.
An edge case exists: if no other bids have been revealed, once the reveal period passes, the extortionist can open the auction again, and then buy the name, and eventually get their ETH back by releasing it.
Recommendation: A possible fix would be to modify the condition on line 377 from
if(auctionState == Mode.Owned) {
to
if(auctionState == Mode.Owned || now > h.registrationDate) {
- On line 298 this comment is no longer accurate, and could cause confusion.
// for the first month of the registry, make longer auctions
- On lines 316-318,
@param owneris missing from the natspec comments. - On lines 178-180,
@param _startdateis mising. - On lines 415-416,
@param bidderis missing
Recommendation: Keep comments current.
In both the Deed and Registrar contracts, several functions have no natspec comments, nor any comments at all.
Recommendation: Add natspec comments on all functions for consistency, and to clarify design decisions.
My understanding is that the destroyDeed() function should only be called by the Deed contract's own closeDeed() function. Although calls to this function are also protected by the active boolean value, there is no clear benefit to making it publicly callable.
Recommendation: Add a private visibility specifier.
Although a minor difference, I cannot see a need for derived contracts to call on these functions.
Recommendation: At an earlier stage of the process, I would recommend changing to private. At this stage, I leave it to the designers to weigh the benefits of this very minor change. Proper labelling, with the lowest possible permission level helps to clarify the design for reviewers and contributors.
Since the ENS repository has been reconfigured to run with Truffle I assumed that tests should be run with truffle test --network test, but this command resulted in an error:
Error: Cannot find module 'eth-ens-namehash'
After seeking clarification, I ran the tests with npm test. I did not manually identify any test cases which were not covered.
The following tests were run on the Registrar:
SimpleHashRegistrar
✓ starts auctions (180ms)
✓ launch starts slow with scheduled availability (498ms)
✓ records bids (139ms)
Bidder #1 A regular bid. Spent: 0.5%; Expected: 0.5%;
Bidder #2 Winning bid. Spent: 75%; Expected: 75%;
Bidder #4 Losing bid that doesn't affect price. Spent: 0.5%; Expected: 0.5%;
Bidder #5 Bid with deposit less than claimed value. Spent: 0.5%; Expected: 0.5%;
Bidder #3 Losing bid that affects price. Spent: 0.5%; Expected: 0.5%;
Bidder #6 Bid that wasn't revealed in time. Spent: 99.5%; Expected: 99.5%;
✓ concludes auctions (772ms)
Bid #1 not cancelled
Bid #2 not cancelled pre-reveal
Bid #2 revealed
Bid #2 not cancelled post-reveal. Sealedbid removed
Bid #3 not cancelled immediately
Bid #3 could not cancel a second time
✓ cancels bids (645ms)
Name released and 0.9999999999999672 ether returned to deed owner
✓ releases deed after one year (422ms)
✓ allows releasing a deed immediately when no longer the registrar (161ms)
✓ rejects bids less than the minimum (171ms)
✓ doesn't allow finalizing an auction early (299ms)
✓ allows finalizing an auction even when no longer the registrar (131ms)
✓ doesn't allow revealing a bid on a name not up for auction (114ms)
✓ doesn't invalidate long names (148ms)
✓ allows invalidation even when no longer the registrar (182ms)
✓ calling startAuction on a finished auction has no effect (198ms)
✓ takes the max of declared and provided value (244ms)
✓ invalidates short names (108ms)
✓ zeroes ENS records on invalidation (223ms)
✓ supports transferring deeds to another registrar (339ms)
✓ supports transferring domains to another account (281ms)
Bidder #1 underfunded bid spent: 5 finney;
underfunded bid loses
✓ prohibits late funding of bids (323ms)
✓ prohibits bids during the reveal period (141ms)
✓ prohibits starting auctions when it's not the registrar
✓ permits anyone to zero out ENS records not associated with an owned name (199ms)
✓ does not permit owned names to be zeroed (138ms)
✓ does not permit an empty name to be zeroed
✓ does not allow bidders to replay others' bids (80ms)
A coverage report using solcover was generated, with tremendous assistance from contributor cgewecke.
The results of the report can be viewed here: https://cgewecke.github.io/ens-coverage-reports/contracts/HashRegistrarSimplified.sol.html
Caveat: due to limitations in solcover, inline assembly code had to be commented out, so the portion of the contract using this feature in strlen() appears untested. I am not fluent in assembly, but it appears to me that all of this code should run given a long enough name.
The following are observations from the results of the measurement
This is not missions critical, as it is basically a convenience function, but it should be tested for thoroughness.
The reason for this is explained in this pull request: ensdomains/ens#138. I agree with the decision to leave the code as is.
Many of the conditions which prevent state changing code from running incorrectly are never triggered, this is especially true in the Deed contract's functions. It may be because they are redundant, but I have not thoroughly verified this.
An alternative explanation might be that the tests written are focused on verifying the "happy path", rather than testing the possibility of following an "attack path". If any throw conditions can be triggered, I would recommend that tests are written to do so.
The case of providing a future start date, rather than immediately activating the registrar doesn't seem to be tested. If too cumbersome to add a complete set of tests, it should be manually tested by altering the test suite setup, especially prior to use of the feature for a production launch.
I'm very pleased to see this attempt at reading the code and assessing any seen security issues. Thanks maurelian!
I share the concern of maurelian that the entire process of re-releasing the ENS into the wild seems to be a bit rushed. A longer amount of time for reviews, and especially a large, several-week-long, and well-publicized bug bounty would likely make me, and the broader Ethereum community, rest a bit easier.