Skip to content

Instantly share code, notes, and snippets.

@pipermerriam

pipermerriam/draft.md Secret

Created March 29, 2017 18:31
Show Gist options
  • Save pipermerriam/dfa9c541aef80690c29d353bc9301291 to your computer and use it in GitHub Desktop.
Save pipermerriam/dfa9c541aef80690c29d353bc9301291 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
draft.md

Section 1 - Table of Contents

Section 2 - Introduction

From December 2nd through December 9th of 2016, 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 TODO

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
TODO

3.2 Contracts

TODO

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.4 Major Issues

None

4.5 Critical Issues

None

4.6 Test Coverage Analysis

TODO

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 contains the following conditional.

if(deed.registrar() != address(this) || deed.creationDate() > registryStarted)
    throw;

The second statement in this conditional checks whether the deed was created after this registrar began. For the initial registrar this statement will always evaluate to true resulting in this function always throwing an exceptions.

Suggest removing this function.

5.1.2 Deed contract uses non-standard burn address 0xdead

The Deed contract uses the address 0xdead as the recipient of burned funds.

Recommend changing this to use 0x0 to conform with the more commonly used burn address.

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.3 Major Issues

5.4 Critical Issues

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