Skip to content

Instantly share code, notes, and snippets.

@pipermerriam

pipermerriam/final-draft.md Secret

Created March 31, 2017 17:37
Star
Embed
What would you like to do?
Learn more about clone URLs
Download ZIP
Raw
final-draft.md

Section 1 - Table of Contents

Section 2 - Introduction

From March 23rd through March March 31st of 2017, Piper Merriam conducted an audit of the smart contracts that make up the ENS service. The findings of the audit are presented in this document.

This audit was performed under a contracted hourly rate with no other compensation.

Disclaimer that Piper Merriam is also one of the keyholders for the ENS multisignature contract.

2.1 Authenticity

This document should have an attached cryptographic signature to ensure it has not been tampered with. The signature can be verified using the public key from Piper Merriam's the keybase.io.

Piper Merriam on Keybase.io

2.2 Audit Goals and Focus

2.2.1 Sound Architecture

This audit includes both objective findings from the contract code as well as subjective assessments of the overall architecture and design choices. Given the subjective nature of certain findings it will be up to the ENS development team to determine the appropriate response to each issue.

2.2.2 Smart Contract Best Practices

This audit will evaluate whether the codebase follows the current established best practices for smart contract development.

2.2.3 Code Correctness

This audit will evaluate whether the code does what it is intended to do.

2.2.4 Code Quality

This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.

2.2.5 Security

This audit will look for any exploitable security vulnerabilities, or other potential threats to the integrity of the ENS system as well as user funds held by ENS contracts.

2.3 About ENS

The primary ENS website found at ens.domains says the following about ENS.

ENS offers a secure and decentralised way to address resources both on and off the blockchain using simple, human-readable names.

More information about ENS can be found at the following resources.

2.4 Terminology

This audit uses the following terminology.

2.4.1 Coverage

Measurement of the testing code coverage. This measurement was done via inspection of the code.

2.4.1.1 untested

No tests.

2.4.1.2 low

The tests do not cover some set of non-trivial functionality.

2.4.1.3 good

The tests cover all major functionality.

2.4.1.4 excellent

The tests cover all code paths.

2.4.2 Severity

Measurement of magnitude of an issue.

2.4.2.1 minor

Minor issues are generally subjective in nature, or potentially deal with topics like "best practices" or "readability". Minor issues in general will not indicate an actual problem or bug in code.

The maintainers should use their own judgement as to whether addressing these issues improves the codebase.

2.4.2.2 medium

Medium issues are generally objective in nature. Most medium level issues will not represent an actively exploitable bugs or security problem, but rather an issue that is likely to lead to a future error or security issue.

In most cases a medium issue should be addressed unless there is a clear reason not to.

2.4.2.3 major

Major issues will be things like bugs or security vulnerabilities. These issues may not be directly exploitable such as requiring a specific condition to arise in order to be exploited.

Left unaddressed these issues are highly likely to cause problems with the operation of the contract or lead to a situation which allows the system to be exploited in some way.

2.4.2.4 critical

Critical issues are directly exploitable bugs or security vulnerabilities.

Left unaddressed these issues are highly likely or guaranteed to cause major problems or potentially a full failure in the operations of the contract.

Section 3 - Overview

3.1 Source Code

The ENS source code is publicly available in the ethereum/ens github repository.

https://github.com/ethereum/ens

The code being evaluated can be found under the commit hash b8dfe46a9ffd141b9c606cd534fbc4f42417fb6e

This audit covers the following Solidity source files

  • ./ENS.lll and ./ENS.sol (see below)
  • ./HashRegistrarSimplified.sol
  • ./ReverseRegistrar.sol

The sha256 of that contract source is as follows.

$ shasum -a 256 ./ENS.lll ./ENS.sol ./HashRegistrarSimplified.sol ./ReverseRegistrar.sol
80d739646939eefb00f5d0d98b85734a070cc365ff5866806fc863efc610009e  ./ENS.lll
71e7486785fcc68d8ba314ff4927b401eeaf4dfc7d9b04086b08024cadf0351f  ./ENS.sol
12dcbcc546593e5a9815e1b59f4c16b4d860225cce5d6b076ddc01165c6d8b5b  ./HashRegistrarSimplified.sol
f39e270662d3ee81881847883b20b40d1098f21c5e2de6c1385034b05a70c002  ./ReverseRegistrar.sol

3.2 Contracts

The contracts included in this audit are as follows.

  • ./ENS.lll:ENS
  • ./HashRegistrarSimplified.sol:Deed
  • ./HashRegistrarSimplified.sol:Registrar
  • ./ReverseRegistrar.sol:ReverseRegistrar

The ENS contract is written in LLL. There is a Solidity implementation that can be found in ENS.sol which is intended to be equivalent in functionality.

Section 4 - General Findings

This section contains higher level issues and analysis.

4.1 General Thoughts

The contracts that make up the ENS system appear to be generally well written. Most functions contain both inlined comments as well as higher level comments for each function explaining the intended functionality.

4.2 Minor Issues

4.3 Medium Issues

4.3.1 Deed Factory

Currently, the Registrar and Deed contracts are tightly coupled. There is no mechanism that would allow replacing one without also replacing the other. If a bug were to be found in the Deed contract it would be unfortunate to have to redeploy the entire ENS system.

The common "Factory" pattern could assist in decoupling these two contracts. The Registrar contract would likely need to implement a mechanism to update the address of the DeedFactory as well as some mechanism for allowing users to upgrade their deeds.

One argument that may be made against this idea is that the Deed contract is quite simple and that it is unlikely to contain such a bug. I would point to the recent Solidity Security Issue as a type of bug that will not surface through security audits but could still result in an exploitable bug.

4.3.2 Implement Registrar.trySetSubnodeOwner function.

The following code appears in three separate places.

  • Line 460
  • Line 406
  • Line 441
if(ens.owner(rootNode) == address(this))
    ens.setSubnodeOwner(rootNode, hash, 0);

Recommend moving this logic into an internal function to remove this code duplication.

4.4 Major Issues

None

4.5 Critical Issues

None

4.6 Test Coverage Analysis

4.6.1 ReverseRegistrar

  • Coverage: Excellent

The test coverage for this contract can be found in ./tests/reverseregistrar_test.js

This contract appears to have coverage for all contract functionality.

4.6.2 HashRegistrarSimplified aka Registrar

  • Coverage: Good

The test coverage for this contract can be found in ./tests/simplehashregistrar_test.js

This contract appears to have adequate coverage for all happy paths as well as ensuring various rules are enforced.

The Deed contract functionality is tested as part of the Registrar tests.

4.6.3 ENS

  • Coverage: Excellent

The test coverage for this contract can be found in ./tests/ens_test.js

This contract appears to have coverage for all contract functionality.

Section 5 - Detailed Findings

The following is an exhaustive list of all issues found during in this audit.

5.1 Minor Issues

5.1.1 Registrar.returnDeed function will always throw.

The Registrar.returnDeed function is intended to facilitate the quick return of funds from any deeds opened during the initial ENS launch.

Given the small number of these deeds, it is recommended that this function be removed. Conversations with the ENS team suggest there are viable alternate routes to ensure that no user funds remain locked in these deeds.

5.2 Medium Issues

5.2.1 Registrar contract uses the entry.highestBid and entry.deed variables to derive secondary information about the state of the entry.

The Registrar contract implements a modifier named state which can be used to restrict execution of a function unless the record it operates on is in the specified state.

This modifier contains the following conditionals.

if(entry.highestBid == 0) {
    return Mode.Open;
} else if(entry.deed == Deed(0)) {
    return Mode.Forbidden;
}
...

Determining whether a record is in either the Open or Forbidden state hinges on the value of the highestBid and the deed properties of the record.

The result of this is that the information stored in these fields is used to convey two separate pieces of information.

Consider the situation where a modification to the logic of the contract allows for the highestBid for an owned record to be set to 0. This would result in the state of that record evaluating to Open when it should in fact be Owned.

Recommend implementing additional fields on the record struct to represent these two pieces of information rather than overloading the existing variables.

5.2.2 Deed.destroyDeed contains a multi-line if statement without braces.

The Deed.destroyDeed function contains the following code.

function destroyDeed() {
    if (active) throw;
    if(owner.send(this.balance))
        selfdestruct(burn);
}

The second if conditional in this function is split across multiple lines but does not make use of braces to logically contain the logic within the if statement. This style of code can result in a higher probability of bugs being introduced at a future date.

Consider the following refactor which adds one additional line of logic to be run prior to destruction of a deed..

function destroyDeed() {
    if (active) throw;
    if(owner.send(this.balance))
        runPreDestroy();
        selfdestruct(burn);
}

Since the runPreDestroy and selfdestruct instructions are both at the same indentation level under the if statement, a cursory inspection of this code may miss the fact that the selfdestruct instruction is in fact not contained within the conditional. This function will now always execute the selfdestruct call regardless of whether the second conditional returns true. This rould result in burning the full balance of the deed.

Recommend either adding braces or to put both instructions on the same line.

5.2.3 Registrar.finalizeAuction uses multi-line if statement without braces.

The Registrar.finalizeAuction contains the following code.

if(ens.owner(rootNode) == address(this))
    ens.setSubnodeOwner(rootNode, _hash, h.deed.owner());

See previous issue regarding Deed.destroyDeed.

5.2.4 Registrar.transfer naively calls ens.setSubnodeOwner

The Registrar.transfer function calls the ens.setSubnodeOwner function without first checking whether the registrar is the current registrar. This has the result of disallowing transfers once the registrar has been deactivated or replaced.

If this functionality is intentional: Recommend making this throw explicit by adding a conditional to check whether we are in fact the current registrar and throwing when this is not the case.

If this is not the intended functionality: Recommend wrapping this call in a conditional. This will allow deed owners to continue transferring old deeds around that have not yet been upgraded.

5.3 Major Issues

None

5.4 Critical Issues

None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment