signed-ens.md.asc

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

# Section 1 - Table of Contents<a id="heading-0"/>

* 1 - [Table of Contents](#heading-0)
* 2 - [Introduction](#heading-2)
    * 2.1 - [Authenticity](#heading-2.1)
    * 2.2 - [Audit Goals and Focus](#heading-2.2)
        * 2.2.1 - [Sound Architecture](#heading-2.2.1)
        * 2.2.2 - [Smart Contract Best Practices](#heading-2.2.2)
        * 2.2.3 - [Code Correctness](#heading-2.2.3)
        * 2.2.4 - [Code Quality](#heading-2.2.4)
        * 2.2.5 - [Security](#heading-2.2.5)
    * 2.3 - [About ENS](#heading-2.3)
    * 2.4 - [Terminology](#heading-2.4)
        * 2.4.1 - [Coverage](#heading-2.4.1)
            * 2.4.1.1 - [**untested**](#heading-2.4.1.1)
            * 2.4.1.2 - [**low**](#heading-2.4.1.2)
            * 2.4.1.3 - [**good**](#heading-2.4.1.3)
            * 2.4.1.4 - [**excellent**](#heading-2.4.1.4)
        * 2.4.2 - [Severity](#heading-2.4.2)
            * 2.4.2.1 - [**minor**](#heading-2.4.2.1)
            * 2.4.2.2 - [**medium**](#heading-2.4.2.2)
            * 2.4.2.3 - [**major**](#heading-2.4.2.3)
            * 2.4.2.4 - [**critical**](#heading-2.4.2.4)
* 3 - [Overview](#heading-3)
    * 3.1 - [Source Code](#heading-3.1)
    * 3.2 - [Contracts](#heading-3.2)
* 4 - [General Findings](#heading-4)
    * 4.1 - [General Thoughts](#heading-4.1)
    * 4.2 - [Minor Issues](#heading-4.2)
    * 4.3 - [Medium Issues](#heading-4.3)
        * 4.3.1 - [Deed Factory](#heading-4.3.1)
        * 4.3.2 - [Implement `Registrar.trySetSubnodeOwner` function.](#heading-4.3.2)
    * 4.4 - [Major Issues](#heading-4.4)
    * 4.5 - [Critical Issues](#heading-4.5)
    * 4.6 - [Test Coverage Analysis](#heading-4.6)
        * 4.6.1 - [`ReverseRegistrar`](#heading-4.6.1)
        * 4.6.2 - [`HashRegistrarSimplified` aka `Registrar`](#heading-4.6.2)
        * 4.6.3 - [`ENS`](#heading-4.6.3)
* 5 - [Detailed Findings](#heading-5)
    * 5.1 - [Minor Issues](#heading-5.1)
        * 5.1.1 - [`Registrar.returnDeed` function will always throw.](#heading-5.1.1)
    * 5.2 - [Medium Issues](#heading-5.2)
        * 5.2.1 - [`Registrar` contract uses the `entry.highestBid` and `entry.deed` variables to derive secondary information about the state of the entry.](#heading-5.2.1)
        * 5.2.2 - [`Deed.destroyDeed` contains a multi-line `if` statement without braces.](#heading-5.2.2)
        * 5.2.3 - [`Registrar.finalizeAuction` uses multi-line `if` statement without braces.](#heading-5.2.3)
        * 5.2.4 - [`Registrar.transfer` naively calls `ens.setSubnodeOwner`](#heading-5.2.4)
    * 5.3 - [Major Issues](#heading-5.3)
    * 5.4 - [Critical Issues](#heading-5.4)


# <a id="heading-2"/> 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.


## <a id="heading-2.1"/> 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](https://keybase.io/pipermerriam)


## <a id="heading-2.2"/> 2.2 Audit Goals and Focus

### <a id="heading-2.2.1"/> 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.


### <a id="heading-2.2.2"/> 2.2.2 Smart Contract Best Practices

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


### <a id="heading-2.2.3"/> 2.2.3 Code Correctness

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


### <a id="heading-2.2.4"/> 2.2.4 Code Quality

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


### <a id="heading-2.2.5"/> 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.


## <a id="heading-2.3"/> 2.3 About ENS

The primary ENS website found at [ens.domains](https://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.

- - Project Documentation: http://ens.readthedocs.org/
- - EIP137: https://github.com/ethereum/EIPs/issues/137
- - EIP162: https://github.com/ethereum/EIPs/issues/162
- - EIP181: https://github.com/ethereum/EIPs/issues/181


## <a id="heading-2.4"/> 2.4 Terminology

This audit uses the following terminology.


### <a id="heading-2.4.1"/> 2.4.1 Coverage

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


#### <a id="heading-2.4.1.1"/> 2.4.1.1 **untested**

No tests.


#### <a id="heading-2.4.1.2"/> 2.4.1.2 **low**

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


#### <a id="heading-2.4.1.3"/> 2.4.1.3 **good**

The tests cover all major functionality.


#### <a id="heading-2.4.1.4"/> 2.4.1.4 **excellent**

The tests cover all code paths.


### <a id="heading-2.4.2"/> 2.4.2 Severity

Measurement of magnitude of an issue.


#### <a id="heading-2.4.2.1"/> 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.


#### <a id="heading-2.4.2.2"/> 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.


#### <a id="heading-2.4.2.3"/> 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.


#### <a id="heading-2.4.2.4"/> 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.


# <a id="heading-3"/> Section 3 - Overview

## <a id="heading-3.1"/> 3.1 Source Code

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

[https://github.com/ethereum/ens](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.

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


## <a id="heading-3.2"/> 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.


# <a id="heading-4"/> Section 4 - General Findings

This section contains higher level issues and analysis.


## <a id="heading-4.1"/> 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.


## <a id="heading-4.2"/> 4.2 Minor Issues


## <a id="heading-4.3"/> 4.3 Medium Issues

### <a id="heading-4.3.1"/> 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](https://blog.ethereum.org/2016/11/01/security-alert-solidity-variables-can-overwritten-storage/) 
as a type of bug that will not surface through security audits but could still
result in an exploitable bug.


* [https://blog.ethereum.org/2016/11/01/security-alert-solidity-variables-can-overwritten-storage/](https://blog.ethereum.org/2016/11/01/security-alert-solidity-variables-can-overwritten-storage/)


### <a id="heading-4.3.2"/> 4.3.2 Implement `Registrar.trySetSubnodeOwner` function.

The following code appears in three separate places.

* Line 460
* Line 406
* Line 441

```javascript
if(ens.owner(rootNode) == address(this))
    ens.setSubnodeOwner(rootNode, hash, 0);
```

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


## <a id="heading-4.4"/> 4.4 Major Issues

None

## <a id="heading-4.5"/> 4.5 Critical Issues

None


## <a id="heading-4.6"/> 4.6 Test Coverage Analysis

### <a id="heading-4.6.1"/> 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.


### <a id="heading-4.6.2"/> 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.


### <a id="heading-4.6.3"/> 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.



# <a id="heading-5"/> Section 5 - Detailed Findings

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


## <a id="heading-5.1"/> 5.1 Minor Issues

### <a id="heading-5.1.1"/> 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.


## <a id="heading-5.2"/> 5.2 Medium Issues

### <a id="heading-5.2.1"/> 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.

```javascript
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.


### <a id="heading-5.2.2"/> 5.2.2 `Deed.destroyDeed` contains a multi-line `if` statement without braces.

The `Deed.destroyDeed` function contains the following code.

```javascript
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..


```javascript
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.


### <a id="heading-5.2.3"/> 5.2.3 `Registrar.finalizeAuction` uses multi-line `if` statement without braces.

The `Registrar.finalizeAuction` contains the following code.

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

See previous issue regarding `Deed.destroyDeed`.


### <a id="heading-5.2.4"/> 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.


## <a id="heading-5.3"/> 5.3 Major Issues

None


## <a id="heading-5.4"/> 5.4 Critical Issues

None
-----BEGIN PGP SIGNATURE-----
Version: Keybase OpenPGP v2.0.68
Comment: https://keybase.io/crypto

wsFcBAABCgAGBQJY4tlFAAoJEI0iZdXOvoIv6vgP/0VMOb9x5aNXL5bKkFMPYxqX
rfr1jvZAYa13g+krL4ibXoxUkPcJTXfNlDimn8brSqRY7LnlVs8G/UaScmZg/FJd
zUL0QiwFsx57AZTar5VCI2PCwrjzOkg/5+xm9wFEBUFakLCLy6A7euOc+3qczP5T
yXv9fdv1SNROHO1sPwAkon2oDRgDb2Mh8xVCfsTaTOuWN4366otWStIYAXfqtkfY
Ih6uYKH7HuIVL0Dveg5iqAff9NPaM0MZFUBo+rAwACFb5O7Z6Rio0a5MssEtyyal
+CHk3V+u5OaV3ssKnk6g7ub/5qznwDtHFR+T0kBuzWm6Wyu8g7B1OZ1J2mvHn4Ro
og4YCRzUWRIgXK6wy8xHtTf9JkC4XmdRTh7fl4VIUNB0jDrvJBsVe5h10dn20Pye
rjPZIPDgz4dfMH2kEaNYxdu+QrJTnQLPg9J953SV1+Vl/wZ5f3+lGJK2Ljk89oUm
IWzvcUPPNQ+HFxospWblrlvf6UVfiqDxumTWgfE/m0f9GASMm5PdVs+um2FUEtzf
vXI/buW7loAFdpsOVk8lPvF5tE9bRg74i2Tmv1vZs5g+9lPeyKRMV6jPnAiZdEE8
GGjF1nMerfZ1NKYnkZY0+n95wAjuFP6BQF0LoANuVjyftgHhfURfNYOIGEtixksN
NoJFBfsjxCv+focxFE7Y
=UuSR
-----END PGP SIGNATURE-----