Skip to content

Instantly share code, notes, and snippets.

@pipermerriam

pipermerriam/digix-token-review-final-2016-04-21.md

Last active April 22, 2016 15:15
Show Gist options
  • Star 1 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save pipermerriam/9e381fd34adb062633a9a024b2353e05 to your computer and use it in GitHub Desktop.
Save pipermerriam/9e381fd34adb062633a9a024b2353e05 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
digix-token-review-final-2016-04-21.md

Section 1 - Table of Contents

Section 2 - Introduction

From April 5th through April 14th of 2016, Piper Merriam via Quick Left conducted an audit of the token and badge contracts that will be used with the Digix DAO.

2.1 Audit Goals

2.1.1 Sound Architecture

This audit evaluates the architecture of these two contracts, focusing on whether they conform to established smart contract best practices. Additionally, they will be evaluated on their ability to provide a future proof platform for managing these token based assets.

2.1.2 Code Correctness and Quality

This audit includes a full review of the contract source code. The primary areas of focus include:

  • Correctness (does it do was it is supposed to do)
  • Easy to read and/or understand
  • Sections of code with high complexity.
  • Antipatterns
  • Adequate test coverage.

2.1.3 Security

This audit focused on identifying security related issues.

2.2 Source Code

The code being audited is from the digixdao-contracts repository under the DigixGlobal github account.

https://github.com/DigixGlobal/digixdao-contracts

2.2.1 Initial Review

The initial review of this source code was performed at commit sha 089ec81b319b423d13575b9cc7fb82b8c1139d17.

2.2.2 File List

The following source files were included in the audit.

  • contracts/Interfaces.sol
  • contracts/Token.sol

The SHA256 of these files at the time of the audit was as follows:

$ shasum -a 256 contracts/Interfaces.sol contracts/Token.sol
12ef2f5bf41d68551b76a1db01ac498c2614649da8a6ddc8662aa13d471ebae8  contracts/Interfaces.sol
5dfda3c5d009073ec1d25c7fdbb287c0a21990223826006effec2c0d20854f97  contracts/Token.sol

2.3 Quick Left and Piper Merriam

Quick Left is a web consultancy located in Boulder Colorado, USA.

I, Piper Merriam, am a full time Senior Engineer employed by Quick Left. I have been developing software as my full time profession since 2011.

Evidence of my expertise in this area can be found in the open source work available on my github profile (https://github.com/pipermerriam).

2.4 Terminology

The following terminology is used within this audit to categorize findings.

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 but do not represent actual bugs or security problems.

These issues 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, or may require a certain 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.

2.4.3 Subjective vs. Objective

Some issues may be entirely subjective, as they are the opinion of the reviewer and the choice between the current code and the suggested change does not have a clear objective improvement (eg. naming conventions, tabs vs spaces).

In these cases, they will be marked as follows.

  • Severity: minor
  • Subjective Issue

Section 3 - Main Findings

3.1 Primary Issues

3.1.1 The Token contract is the owner of the Badge contract

  • Severity: medium

The Token contract is written such that it is the owner of the Badge contract.

Recommendation: restructure these contracts such that both the Badge and Token contracts are directly owned by the DAO.

3.1.2 No Overflow Protection

  • Severity: major

There is no overflow protection or checking implemented in any of the addition operations. Combined with the ability for these contracts to mint new tokens, this presents an attack surface that would allow an attacker with access to the minting API to zero out the account balances of other users.

  • Bob has an account balance of 100 tokens.
  • Attacker mints 2**256 - 100 tokens into Bob's account.
  • Bob's account balance will overflow resulting in a zero balance.
  • The totalSupply value will likely overflow as well.

Recommendation: implement overflow protection for all addition operations.

3.1.3 Lack of upgrade paths may hinder the evolution of the DAO

  • Severity: major

The architecture of these two contracts has certain properties that are entirely immutable once initialized.

  • The owner address
  • The dao address

Not providing an API for changing these values may be a desired property. It will also restrict the ability for the current contracts to be carried forward as the DAO evolves.

Recommendation: implement a mechanism for updating these values.

3.2 Test Coverage

  • Coverage: untested
  • Severity: major

The Token and Badge contracts do not have any test coverage.

Recommendation: add tests. The benefits of testing in software are well documented.

3.3 General Thoughts

3.3.1 Use of public to explicitly indicate methods as being an exposed API

  • Severity: minor
  • Subjective Issue

It has improved the readability of contract source code to explicitly mark functions as public. While this is the default, it helps those reading the contract source code to more easily differentiate between what functions are exposed publicly and which ones are not.

3.3.2 Reusable overflow and underflow protection

  • Severity: medium
  • Subjective Issue

Each subtraction operation implements its own underflow protection.

It may be prudent to write this code once and reuse it in all places where this protection is needed. The following code could be used as either an abstract base contract or a library to accomplish this.

function safeToAdd(uint a, uint b) returns (bool) {
    return (a + b >= a);
}

function addSafely(uint a, uint b) returns (uint) {
    if (!safeToAdd(a, b)) throw;
    return a + b;
}

function safeToSubtract(uint a, uint b) returns (bool) {
    return (b <= a);
}

function subtractSafely(uint a, uint b) returns (uint) {
    if (!safeToSubtract(a, b)) throw;
    return a - b;
}

Section 4 - Detailed Findings

4.1 Source File: contracts/Token.sol

4.1.1 Token.approve and Badge.approve only allow for allowance up to current account balance

  • Severity: medium

The approve function of both the Token and Badge contracts only allow approvals up to the current account balance. This restriction may not be desired as it limits how users can use the approval/allowance API.

Consider a user who plans to have any tokens they acquire be immediately sold by a 3rd party. Without the ability to approve above the current account balance, each time new tokens are acquired, the user would need to re-approve the 3rd party in order for them to have access to the new tokens.

Recommendation: remove this logic in favor of allowing allowances to be set to any value.

4.1.2 Token.allowance and Badge.allowance do not return the actual approved amount

  • Severity: medium

The allowance function of both the Token and Badge contracts does not return the actual approval value. Instead, it computes the value that could be currently transferred based on the current account balance and returns this amount. This logic may not be desirable as it limits the way that users can use the approval/allowance API.

See the Token.approve and Token.allowance issue for an example use case.

The current functionality can still be achieved by querying both the allowance and balanceOf values instead of implementing the logic within the allowance method.

Recommendation: remove this logic in favor of directly returning the allowance value.

4.1.3 The authority to mint both tokens is handled by the same mechanism

  • Severity: medium

Both Token and Badge implement privileged APIs for minting new tokens. The mechanism that controls authorization for minting is contained in the Token contract. Any address that is set as a seller may mint tokens for either contract.

Coupling these two mechanisms into a single access control mechanism may end up problematic. By having the authorization for both contracts controlled by a single mechanism it is not possible to grant authority to mint only badges or only tokens.

Recommendation: decouple this into two separate authorization mechanisms.

4.1.4 The Events from the Token and Badge contracts do not conform to ERC20

** Severity major

ERC20 specifies the following two events.

  • event Transfer(address indexed _from, address indexed _to, uint256 _value)
  • event Approval(address indexed _owner, address indexed _spender, uint256 _value)

The Token and Badge implement the following versions of these events.

  • event Transfer(address indexed _from, address indexed _to, uint256 indexed _value)
  • event Approval(address indexed _owner, address indexed _spender, uint256 indexed _value)

In the Token and Badge implementations the _value argument is set as indexed. This may cause compatibility issues with 3rd party integrations which interact with contracts according to the official ERC20 specification.

Recommendation: remove the indexed attribute to be conformant with ERC20.

4.1.5 The Mint event from the Token and Badge contracts do not follow the pattern established in ERC20

** Severity minor

The Mint event is declared as:

  • event Mint(address indexed _recipient, uint256 indexed _amount)

While ERC20 does not specify a Mint event, the expected format can be extrapolated from the Transfer and Approve events which do not have the _amount value set as indexed.

Recommendation: remove the indexed attribute from these events.

4.1.6 The Token.locked and Badge.locked could removed

  • Severity: minor

The locked variable on both the Token and Badge contracts is only used in a single code path to ensure that once the address of the DAO has been set, it cannot be changed to another value.

Replacing the locked mechanism with if (dao == 0x0) would reduce the overall complexity of these contracts.

Recommendation: remove this variable in favor of if (dao == 0x0) until a separate code path requires a generic locking mechanism to be put into place.

4.1.7 The Token and Badge contracts do not implement a mechanism for transferring ownership

  • Severity: major

The Token and Badge contracts do not implement any mechanism for transferring ownership. This eliminates any upgrade path in the event that the owner needs to be updates.

Consider the situation where a bug is found in the DAO code. Ideally a new DAO contract could be deployed with this issue fixed, and the ownership of the token contracts could be transferred to the new DAO. This is not possible with the current code.

Recommendation: add an API for transferring ownership.

4.2 Contract: contracts/Token.sol:Badge

4.2.1 Constructor takes a _config value but doesn't use it

  • Severity: minor

The constructor for this contract takes an argument _config of type address which is not used or stored.

Recommendation: remove this argument.

4.2.2 The sellers mapping is unused

This contract does not make use of the sellers mapping.

Recommendation: remove this mapping.

4.3 Contract: contracts/Token.sol:Token

4.3.1 No mechanism for un-registering sellers

  • Severity: major

The Token contract presents the function registerSeller for adding addresses to the sellers mapping. Any address present in this mapping is granted permission to mint new tokens.

The Token contract does not present any API for unregistering addresses to remove them from this mapping. In the event that an address ends up in this mapping that is controlled by an attacker, the integrity of the the tokens controlled by this contract could be compromised.

Recommendation: add a mechanism for removing addresses from this mapping.

4.3.2 The noEther modifier is unused

  • Severity: minor

The modifier noEther is declared but not used anywhere in the contract.

Recommendation: remove this modifier.

4.3.3 The contract does not allow querying of whether an address is a seller

  • Severity: minor

The sellers mapping is not public and the contract does not implement any getter function for querying whether an address is set as a seller.

Recommendation: make the sellers mapping public or implementing an API for querying whether an address is a seller.

4.3.4 The owner and dao variables are not public

  • Severity: minor

The dao and owner variables are not marked as public. This makes auditing these contracts more difficult.

Recommendation: make these variables public.

4.3.5 The config variable is not used outside of the constructor

  • Severity: minor
  • Subjective Issue

The config variable is used to query certain values in the constructor function, but is not used in any of the the other contract logic.

Recommendation: remove this variable and/or modifying the constructor to explicitly take constructor arguments for the values queried from the config contract. Doing so simplifies this contract at no cost to functionality.

Section 5 - Resolution

This audit was performed on the codebase at commit sha 936cf20a6a06031f52c27b0a89b23f880b09b43e.

5.1 Terminology

5.1.1 Status: Resolved

Issues that are noted with the resolved status have been adequately addressed.

5.1.2 Status: Unchanged

Issues that are noted with the unchanged status have not been addressed.

5.1.3 Status: Needs Work

Something was done in response to this issue but it does not fully address the problem.

5.2 Primary Findings

5.2.1 The Token contract owns the Badge contract

Per a conversation with the Digix developers this is an intentional design decision.

5.2.2 No overflow protection

This issue has been resolved within the Token contract's mint function. This function now uses a addSafely function to explicitly that the addition operation cannot overflow.

This was the only location in the code where this attack vector was found to be exploitable. This effectively restricts the totalSupply from ever exceeding 2**256 which in turn should make it impossible for any other addition operations to overflow.

There are other locations in the code where an additional level of safety could be attained by using these new addSafely and subtractSafely functions but given the simplicity of the code it is suspect as to whether this safety is worth the tradeoff in code complexity.

5.2.3 No upgrade path for token and badge contracts

Both Token and Badge contracts now implement a setOwner function which allows for transferring ownership of these contracts, allowing an upgrade path in the event this is necessary.

5.3 Test Coverage

The codebase remains without tests. Per discussion with the Digix developers they will be executing manual tests against these contracts.

5.4 Minor Issues

5.4.1 The Mint event does not follow the ERC20 pattern

The _amount argument for this event is still set to be indexed in both the Token and Badge contract.

5.4.2 Token.locked and Badge.locked mechanism could be removed

This mechanism is still in place. This is assumed to be an intentional design decision.

5.4.3 Badge contract has unused constructor argument

The constructor still receives the _config argument and does not use it.

5.4.4 Badge contract has unused mapping seller

This variable is still in place on the Badge contract and is not used anywhere in the contract source code.

5.4.5 Unused noEther modifier

This modifier has been removed.

5.4.6 No way to query whether an address is a seller

No mechanism for querying whether an address is in the seller mapping has been implemented.

5.4.7 The dao and owner variables are not public

These variables are now marked as public.

5.4.8 The config variable is not used outside of the constructor

The use of this value remains contained to the constructor.

5.5 Medium Issues

5.5.1 Token minting authorization is handled by the same mechanism

This is unchanged, and assumed to be an intentional design decision.

5.5.2 Restrictive API for approval and allowance

These functions have been modified to remove the account balance restrictions.

5.6 Major Issues

5.6.1 ERC20 Conformant Events

The ERC20 events in both the Token and Badge contracts now conform to the ERC20 specification.

5.6.2 No mechanism to transfer ownership

See notes in Primary Findings section.

5.6.3 No mechanism for unregistering sellers

The Token contract now implements a unregisterSeller function which can be used to remove addresses from the seller mapping.

5.7 Critical Issues

No critical issues were found in this audit.

Raw
digix-token-review-final-2016-04-21.md.asc
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
# Section 1 - Table of Contents<a id="heading-0"/>
* 1 - [Table of Contents](#heading-0)
* 2 - [Introduction](#heading-2)
* 2.1 - [Audit Goals](#heading-2.1)
* 2.1.1 - [Sound Architecture](#heading-2.1.1)
* 2.1.2 - [Code Correctness and Quality](#heading-2.1.2)
* 2.1.3 - [Security](#heading-2.1.3)
* 2.2 - [Source Code](#heading-2.2)
* 2.2.1 - [Initial Review](#heading-2.2.1)
* 2.2.2 - [File List](#heading-2.2.2)
* 2.3 - [Quick Left and Piper Merriam](#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)
* 2.4.3 - [Subjective vs. Objective](#heading-2.4.3)
* 3 - [Main Findings](#heading-3)
* 3.1 - [Primary Issues](#heading-3.1)
* 3.1.1 - [The `Token` contract is the *owner* of the `Badge` contract](#heading-3.1.1)
* 3.1.2 - [No Overflow Protection](#heading-3.1.2)
* 3.1.3 - [Lack of upgrade paths may hinder the evolution of the DAO](#heading-3.1.3)
* 3.2 - [Test Coverage](#heading-3.2)
* 3.3 - [General Thoughts](#heading-3.3)
* 3.3.1 - [Use of `public` to explicitly indicate methods as being an exposed API](#heading-3.3.1)
* 3.3.2 - [Reusable overflow and underflow protection](#heading-3.3.2)
* 4 - [Detailed Findings](#heading-4)
* 4.1 - [Source File: `contracts/Token.sol`](#heading-4.1)
* 4.1.1 - [`Token.approve` and `Badge.approve` only allow for allowance up to current account balance](#heading-4.1.1)
* 4.1.2 - [`Token.allowance` and `Badge.allowance` do not return the actual approved amount](#heading-4.1.2)
* 4.1.3 - [The authority to *mint* both tokens is handled by the same mechanism](#heading-4.1.3)
* 4.1.4 - [The Events from the `Token` and `Badge` contracts do not conform to ERC20](#heading-4.1.4)
* 4.1.5 - [The `Mint` event from the `Token` and `Badge` contracts do not follow the pattern established in ERC20](#heading-4.1.5)
* 4.1.6 - [The `Token.locked` and `Badge.locked` could removed](#heading-4.1.6)
* 4.1.7 - [The `Token` and `Badge` contracts do not implement a mechanism for transferring ownership](#heading-4.1.7)
* 4.2 - [Contract: `contracts/Token.sol:Badge`](#heading-4.2)
* 4.2.1 - [Constructor takes a `_config` value but doesn't use it](#heading-4.2.1)
* 4.2.2 - [The `sellers` mapping is unused](#heading-4.2.2)
* 4.3 - [Contract: `contracts/Token.sol:Token`](#heading-4.3)
* 4.3.1 - [No mechanism for un-registering sellers](#heading-4.3.1)
* 4.3.2 - [The `noEther` modifier is unused](#heading-4.3.2)
* 4.3.3 - [The contract does not allow querying of whether an address is a *seller*](#heading-4.3.3)
* 4.3.4 - [The `owner` and `dao` variables are not public](#heading-4.3.4)
* 4.3.5 - [The `config` variable is not used outside of the constructor](#heading-4.3.5)
* 5 - [Resolution](#heading-5)
* 5.1 - [Terminology](#heading-5.1)
* 5.1.1 - [Status: Resolved](#heading-5.1.1)
* 5.1.2 - [Status: Unchanged](#heading-5.1.2)
* 5.1.3 - [Status: Needs Work](#heading-5.1.3)
* 5.2 - [Primary Findings](#heading-5.2)
* 5.2.1 - [The `Token` contract owns the `Badge` contract](#heading-5.2.1)
* 5.2.2 - [No overflow protection](#heading-5.2.2)
* 5.2.3 - [No upgrade path for token and badge contracts](#heading-5.2.3)
* 5.3 - [Test Coverage](#heading-5.3)
* 5.4 - [Minor Issues](#heading-5.4)
* 5.4.1 - [The `Mint` event does not follow the ERC20 pattern](#heading-5.4.1)
* 5.4.2 - [`Token.locked` and `Badge.locked` mechanism could be removed](#heading-5.4.2)
* 5.4.3 - [`Badge` contract has unused constructor argument](#heading-5.4.3)
* 5.4.4 - [`Badge` contract has unused mapping `seller`](#heading-5.4.4)
* 5.4.5 - [Unused `noEther` modifier](#heading-5.4.5)
* 5.4.6 - [No way to query whether an address is a `seller`](#heading-5.4.6)
* 5.4.7 - [The `dao` and `owner` variables are not public](#heading-5.4.7)
* 5.4.8 - [The `config` variable is not used outside of the constructor](#heading-5.4.8)
* 5.5 - [Medium Issues](#heading-5.5)
* 5.5.1 - [Token minting authorization is handled by the same mechanism](#heading-5.5.1)
* 5.5.2 - [Restrictive API for approval and allowance](#heading-5.5.2)
* 5.6 - [Major Issues](#heading-5.6)
* 5.6.1 - [ERC20 Conformant Events](#heading-5.6.1)
* 5.6.2 - [No mechanism to transfer ownership](#heading-5.6.2)
* 5.6.3 - [No mechanism for unregistering sellers](#heading-5.6.3)
* 5.7 - [Critical Issues](#heading-5.7)
# <a id="heading-2"/> Section 2 - Introduction
From April 5th through April 14th of 2016, Piper Merriam via Quick Left
conducted an audit of the token and badge contracts that will be used
with the Digix DAO.
## <a id="heading-2.1"/> 2.1 Audit Goals
### <a id="heading-2.1.1"/> 2.1.1 Sound Architecture
This audit evaluates the architecture of these two contracts, focusing on
whether they conform to established smart contract best practices.
Additionally, they will be evaluated on their ability to provide a future proof
platform for managing these token based assets.
### <a id="heading-2.1.2"/> 2.1.2 Code Correctness and Quality
This audit includes a full review of the contract source code. The primary
areas of focus include:
* Correctness (does it do was it is supposed to do)
* Easy to read and/or understand
* Sections of code with high complexity.
* Antipatterns
* Adequate test coverage.
### <a id="heading-2.1.3"/> 2.1.3 Security
This audit focused on identifying security related issues.
## <a id="heading-2.2"/> 2.2 Source Code
The code being audited is from the `digixdao-contracts` repository under the
**DigixGlobal** github account.
https://github.com/DigixGlobal/digixdao-contracts
### <a id="heading-2.2.1"/> 2.2.1 Initial Review
The initial review of this source code was performed at commit sha
`089ec81b319b423d13575b9cc7fb82b8c1139d17`.
### <a id="heading-2.2.2"/> 2.2.2 File List
The following source files were included in the audit.
* `contracts/Interfaces.sol`
* `contracts/Token.sol`
The SHA256 of these files at the time of the audit was as follows:
```
$ shasum -a 256 contracts/Interfaces.sol contracts/Token.sol
12ef2f5bf41d68551b76a1db01ac498c2614649da8a6ddc8662aa13d471ebae8 contracts/Interfaces.sol
5dfda3c5d009073ec1d25c7fdbb287c0a21990223826006effec2c0d20854f97 contracts/Token.sol
```
## <a id="heading-2.3"/> 2.3 Quick Left and Piper Merriam
Quick Left is a web consultancy located in Boulder Colorado, USA.
I, Piper Merriam, am a full time Senior Engineer employed by Quick Left. I
have been developing software as my full time profession since 2011.
Evidence of my expertise in this area can be found in the open source work
available on my github profile
([https://github.com/pipermerriam](https://github.com/pipermerriam)).
## <a id="heading-2.4"/> 2.4 Terminology
The following terminology is used within this audit to categorize findings.
### <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 but do not represent actual
bugs or security problems.
These issues 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, or may require a certain 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-2.4.3"/> 2.4.3 Subjective vs. Objective
Some issues may be entirely subjective, as they are the opinion of the reviewer
and the choice between the current code and the suggested change does not have
a clear objective improvement (eg. naming conventions, tabs vs spaces).
In these cases, they will be marked as follows.
* Severity: **minor**
* Subjective Issue
# <a id="heading-3"/> Section 3 - Main Findings
## <a id="heading-3.1"/> 3.1 Primary Issues
### <a id="heading-3.1.1"/> 3.1.1 The `Token` contract is the *owner* of the `Badge` contract
* Severity: **medium**
The `Token` contract is written such that it is the *owner* of the `Badge`
contract.
Recommendation: restructure these contracts such that both the `Badge` and
`Token` contracts are directly owned by the DAO.
### <a id="heading-3.1.2"/> 3.1.2 No Overflow Protection
* Severity: **major**
There is no overflow protection or checking implemented in any of the addition
operations. Combined with the ability for these contracts to *mint* new
tokens, this presents an attack surface that would allow an attacker with
access to the minting API to zero out the account balances of other users.
* Bob has an account balance of `100` tokens.
* Attacker mints `2**256 - 100` tokens into Bob's account.
* Bob's account balance will overflow resulting in a zero balance.
* The `totalSupply` value will likely overflow as well.
Recommendation: implement overflow protection for all addition operations.
### <a id="heading-3.1.3"/> 3.1.3 Lack of upgrade paths may hinder the evolution of the DAO
* Severity: **major**
The architecture of these two contracts has certain properties that are
entirely immutable once initialized.
* The `owner` address
* The `dao` address
Not providing an API for changing these values may be a desired property. It
will also restrict the ability for the current contracts to be carried forward
as the DAO evolves.
Recommendation: implement a mechanism for updating these values.
## <a id="heading-3.2"/> 3.2 Test Coverage
* Coverage: **untested**
* Severity: **major**
The `Token` and `Badge` contracts do not have any test coverage.
Recommendation: add tests. The benefits of testing in software are well
documented.
## <a id="heading-3.3"/> 3.3 General Thoughts
### <a id="heading-3.3.1"/> 3.3.1 Use of `public` to explicitly indicate methods as being an exposed API
* Severity: **minor**
* Subjective Issue
It has improved the readability of contract source code to explicitly mark
functions as `public`. While this is the *default*, it helps those reading the
contract source code to more easily differentiate between what functions are
exposed publicly and which ones are not.
### <a id="heading-3.3.2"/> 3.3.2 Reusable overflow and underflow protection
* Severity: **medium**
* Subjective Issue
Each subtraction operation implements its own underflow protection.
It may be prudent to write this code *once* and reuse it in all places where
this protection is needed. The following code could be used as either an
abstract base contract or a library to accomplish this.
```javascript
function safeToAdd(uint a, uint b) returns (bool) {
return (a + b >= a);
}
function addSafely(uint a, uint b) returns (uint) {
if (!safeToAdd(a, b)) throw;
return a + b;
}
function safeToSubtract(uint a, uint b) returns (bool) {
return (b <= a);
}
function subtractSafely(uint a, uint b) returns (uint) {
if (!safeToSubtract(a, b)) throw;
return a - b;
}
```
# <a id="heading-4"/> Section 4 - Detailed Findings
## <a id="heading-4.1"/> 4.1 Source File: `contracts/Token.sol`
### <a id="heading-4.1.1"/> 4.1.1 `Token.approve` and `Badge.approve` only allow for allowance up to current account balance
* Severity: **medium**
The `approve` function of both the `Token` and `Badge` contracts only allow
approvals up to the current account balance. This restriction may not be
desired as it limits how users can use the approval/allowance API.
Consider a user who plans to have any tokens they acquire be immediately sold
by a 3rd party. Without the ability to approve above the current account
balance, each time new tokens are acquired, the user would need to re-approve
the 3rd party in order for them to have access to the new tokens.
Recommendation: remove this logic in favor of allowing allowances to be set to any
value.
### <a id="heading-4.1.2"/> 4.1.2 `Token.allowance` and `Badge.allowance` do not return the actual approved amount
* Severity: **medium**
The `allowance` function of both the `Token` and `Badge` contracts does not
return the actual approval value. Instead, it computes the value that could be
currently transferred based on the current account balance and returns this
amount. This logic may not be desirable as it limits the way that users can
use the approval/allowance API.
See the `Token.approve` and `Token.allowance` issue for an example use case.
The current functionality can still be achieved by querying both the
`allowance` and `balanceOf` values instead of implementing the logic within the
`allowance` method.
Recommendation: remove this logic in favor of directly returning the allowance
value.
### <a id="heading-4.1.3"/> 4.1.3 The authority to *mint* both tokens is handled by the same mechanism
* Severity: **medium**
Both `Token` and `Badge` implement privileged APIs for *minting* new tokens.
The mechanism that controls authorization for minting is contained in the
`Token` contract. Any address that is set as a *seller* may mint tokens for
either contract.
Coupling these two mechanisms into a single access control mechanism may end up
problematic. By having the authorization for both contracts controlled by a
single mechanism it is not possible to grant authority to mint *only* badges or
*only* tokens.
Recommendation: decouple this into two separate authorization mechanisms.
### <a id="heading-4.1.4"/> 4.1.4 The Events from the `Token` and `Badge` contracts do not conform to ERC20
** Severity **major**
ERC20 specifies the following two events.
* `event Transfer(address indexed _from, address indexed _to, uint256 _value)`
* `event Approval(address indexed _owner, address indexed _spender, uint256 _value)`
The `Token` and `Badge` implement the following versions of these events.
* `event Transfer(address indexed _from, address indexed _to, uint256 indexed _value)`
* `event Approval(address indexed _owner, address indexed _spender, uint256 indexed _value)`
In the `Token` and `Badge` implementations the `_value` argument is set as
`indexed`. This may cause compatibility issues with 3rd party integrations
which interact with contracts according to the official ERC20 specification.
Recommendation: remove the `indexed` attribute to be conformant with ERC20.
### <a id="heading-4.1.5"/> 4.1.5 The `Mint` event from the `Token` and `Badge` contracts do not follow the pattern established in ERC20
** Severity **minor**
The `Mint` event is declared as:
* `event Mint(address indexed _recipient, uint256 indexed _amount)`
While ERC20 does not specify a `Mint` event, the *expected* format can be
extrapolated from the `Transfer` and `Approve` events which do **not** have the
`_amount` value set as `indexed`.
Recommendation: remove the `indexed` attribute from these events.
### <a id="heading-4.1.6"/> 4.1.6 The `Token.locked` and `Badge.locked` could removed
* Severity: **minor**
The `locked` variable on both the `Token` and `Badge` contracts is only used in
a single code path to ensure that once the address of the **DAO** has been set,
it cannot be changed to another value.
Replacing the `locked` mechanism with `if (dao == 0x0)` would reduce the
overall complexity of these contracts.
Recommendation: remove this variable in favor of `if (dao == 0x0)` until a separate
code path requires a generic locking mechanism to be put into place.
### <a id="heading-4.1.7"/> 4.1.7 The `Token` and `Badge` contracts do not implement a mechanism for transferring ownership
* Severity: **major**
The `Token` and `Badge` contracts do not implement any mechanism for
transferring ownership. This eliminates any upgrade path in the event that the
owner needs to be updates.
Consider the situation where a bug is found in the DAO code. Ideally a *new*
DAO contract could be deployed with this issue fixed, and the ownership of the
token contracts could be transferred to the new DAO. This is not possible with
the current code.
Recommendation: add an API for transferring ownership.
## <a id="heading-4.2"/> 4.2 Contract: `contracts/Token.sol:Badge`
### <a id="heading-4.2.1"/> 4.2.1 Constructor takes a `_config` value but doesn't use it
* Severity: **minor**
The constructor for this contract takes an argument `_config` of type `address`
which is not used or stored.
Recommendation: remove this argument.
### <a id="heading-4.2.2"/> 4.2.2 The `sellers` mapping is unused
This contract does not make use of the `sellers` mapping.
Recommendation: remove this mapping.
## <a id="heading-4.3"/> 4.3 Contract: `contracts/Token.sol:Token`
### <a id="heading-4.3.1"/> 4.3.1 No mechanism for un-registering sellers
* Severity: **major**
The `Token` contract presents the function `registerSeller` for adding
addresses to the `sellers` mapping. Any address present in this mapping is
granted permission to mint new tokens.
The `Token` contract does not present any API for *unregistering* addresses to
remove them from this mapping. In the event that an address ends up in this
mapping that is controlled by an attacker, the integrity of the the tokens
controlled by this contract could be compromised.
Recommendation: add a mechanism for removing addresses from this mapping.
### <a id="heading-4.3.2"/> 4.3.2 The `noEther` modifier is unused
* Severity: **minor**
The modifier `noEther` is declared but not used anywhere in the contract.
Recommendation: remove this modifier.
### <a id="heading-4.3.3"/> 4.3.3 The contract does not allow querying of whether an address is a *seller*
* Severity: **minor**
The `sellers` mapping is not `public` and the contract does not implement any
*getter* function for querying whether an address is set as a *seller*.
Recommendation: make the `sellers` mapping `public` or implementing an API for
querying whether an address is a *seller*.
### <a id="heading-4.3.4"/> 4.3.4 The `owner` and `dao` variables are not public
* Severity: **minor**
The `dao` and `owner` variables are not marked as `public`. This makes
auditing these contracts more difficult.
Recommendation: make these variables `public`.
### <a id="heading-4.3.5"/> 4.3.5 The `config` variable is not used outside of the constructor
* Severity: **minor**
* Subjective Issue
The `config` variable is used to query certain values in the constructor
function, but is not used in any of the the other contract logic.
Recommendation: remove this variable and/or modifying the constructor to
explicitly take constructor arguments for the values queried from the `config`
contract. Doing so simplifies this contract at no cost to functionality.
# <a id="heading-5"/> Section 5 - Resolution
This audit was performed on the codebase at commit sha
`936cf20a6a06031f52c27b0a89b23f880b09b43e`.
## <a id="heading-5.1"/> 5.1 Terminology
### <a id="heading-5.1.1"/> 5.1.1 Status: Resolved
Issues that are noted with the **resolved** status have been adequately
addressed.
### <a id="heading-5.1.2"/> 5.1.2 Status: Unchanged
Issues that are noted with the **unchanged** status have not been addressed.
### <a id="heading-5.1.3"/> 5.1.3 Status: Needs Work
Something was done in response to this issue but it does not fully address the
problem.
## <a id="heading-5.2"/> 5.2 Primary Findings
### <a id="heading-5.2.1"/> 5.2.1 The `Token` contract owns the `Badge` contract
* **Status:** unchanged
* [Issue 3.1.1](#heading-3.1.1)
Per a conversation with the Digix developers this is an intentional design
decision.
### <a id="heading-5.2.2"/> 5.2.2 No overflow protection
* **Status:** resolved
* [Issue 3.1.2](#heading-3.1.2)
This issue has been resolved within the `Token` contract's `mint` function.
This function now uses a `addSafely` function to explicitly that the addition
operation cannot overflow.
This was the only location in the code where this attack vector was
found to be exploitable. This effectively restricts the `totalSupply` from
ever exceeding `2**256` which in turn *should* make it impossible for any other
addition operations to overflow.
There are other locations in the code where an additional level of safety could
be attained by using these new `addSafely` and `subtractSafely` functions but
given the simplicity of the code it is suspect as to whether this safety is
worth the tradeoff in code complexity.
### <a id="heading-5.2.3"/> 5.2.3 No upgrade path for token and badge contracts
* **Status:** resolved
* [Issue 3.1.3](#heading-3.1.3)
* [Issue 4.1.7](#heading-4.1.7)
Both `Token` and `Badge` contracts now implement a `setOwner` function which
allows for transferring ownership of these contracts, allowing an upgrade path
in the event this is necessary.
## <a id="heading-5.3"/> 5.3 Test Coverage
* **Status:** unchanged
* [Issue 3.2](#heading-3.2)
The codebase remains without tests. Per discussion with the Digix developers
they will be executing manual tests against these contracts.
## <a id="heading-5.4"/> 5.4 Minor Issues
### <a id="heading-5.4.1"/> 5.4.1 The `Mint` event does not follow the ERC20 pattern
* **Status:** unchanged
* [Issue 4.1.5](#heading-4.1.5)
The `_amount` argument for this event is still set to be `indexed` in both the
`Token` and `Badge` contract.
### <a id="heading-5.4.2"/> 5.4.2 `Token.locked` and `Badge.locked` mechanism could be removed
* **Status:** unchanged
* [Issue 4.1.6](#heading-4.1.6)
This mechanism is still in place. This is assumed to be an intentional design
decision.
### <a id="heading-5.4.3"/> 5.4.3 `Badge` contract has unused constructor argument
* **Status:** unchanged
* [Issue 4.2.1](#heading-4.2.1)
The constructor still receives the `_config` argument and does not use it.
### <a id="heading-5.4.4"/> 5.4.4 `Badge` contract has unused mapping `seller`
* **Status:** unchanged
* [Issue 4.2.2](#heading-4.2.2)
This variable is still in place on the `Badge` contract and is not used
anywhere in the contract source code.
### <a id="heading-5.4.5"/> 5.4.5 Unused `noEther` modifier
* **Status:** resolved
* [Issue 4.3.2](#heading-4.3.2)
This modifier has been removed.
### <a id="heading-5.4.6"/> 5.4.6 No way to query whether an address is a `seller`
* **Status:** unchanged
* [Issue 4.3.3](#heading-4.3.3)
No mechanism for querying whether an address is in the `seller` mapping has
been implemented.
### <a id="heading-5.4.7"/> 5.4.7 The `dao` and `owner` variables are not public
* **Status:** resolved
* [Issue 4.3.4](#heading-4.3.4)
These variables are now marked as `public`.
### <a id="heading-5.4.8"/> 5.4.8 The `config` variable is not used outside of the constructor
* **Status:** unchanged
* [Issue 4.3.5](#heading-4.3.5)
The use of this `value` remains contained to the constructor.
## <a id="heading-5.5"/> 5.5 Medium Issues
### <a id="heading-5.5.1"/> 5.5.1 Token minting authorization is handled by the same mechanism
* **Status:** unchanged
* [Issue 4.1.3](#heading-4.1.3)
This is unchanged, and assumed to be an intentional design decision.
### <a id="heading-5.5.2"/> 5.5.2 Restrictive API for approval and allowance
* **Status:** resolved
* [Issue 4.1.1](#heading-4.1.1)
* [Issue 4.1.2](#heading-4.1.2)
These functions have been modified to remove the account balance restrictions.
## <a id="heading-5.6"/> 5.6 Major Issues
### <a id="heading-5.6.1"/> 5.6.1 ERC20 Conformant Events
* **Status:** resolved
* [Issue 4.1.4](#heading-4.1.4)
The ERC20 events in both the `Token` and `Badge` contracts now conform to the
ERC20 specification.
### <a id="heading-5.6.2"/> 5.6.2 No mechanism to transfer ownership
* **Status:** resolved
* [Issue 4.1.7](#heading-4.1.7)
See notes in **Primary Findings** section.
### <a id="heading-5.6.3"/> 5.6.3 No mechanism for unregistering sellers
* **Status:** resolved
* [Issue 4.3.1](#heading-4.3.1)
The `Token` contract now implements a `unregisterSeller` function which can be
used to remove addresses from the `seller` mapping.
## <a id="heading-5.7"/> 5.7 Critical Issues
No critical issues were found in this audit.
-----BEGIN PGP SIGNATURE-----
wsFcBAEBCAAQBQJXGQXzCRCNImXVzr6CLwAACgUQAJahmyLYhtByCAUs4GRnzjS1
0o5enyHxunZ/5bEblRyQ0Mh6fNzuZJIee33R5JabHW7dCWpVh24RYD79E53vyvdl
QfgeCizrfXpkePpZU4HZFLsh3xA6ESc6gfOIonMiRgunkzpOONH82OgNqYPaDDdB
Sq2Au/DOZqc0NiDkh75l+FJ0XDZNDinQkldf2KZilz6/fuPRYkW6N9LPePSITbJn
zYT3bqxpmQUJAzjBdHhHwvaaKShoFBEimCFpwRtjudeki30siYbmnD3YKSkRYrWP
Qd42rfVNnA8tXybrHM2TV6Y7Zv3rDe2urrj/bg4uedziyq6fdnL3Cds/yaz1/6Ar
ib/nyfhv1AKwpTELDZ0f1oDtW4u+zNZFJq6jBzd34zW6OTJVi923rYQUI7kItFQK
7Mi3z4SKvzx6EYasWGqRv5HVy5r2U99qi2Gncq/C+I8JmGGYIBi7p/SSE5bkMwIY
rj8FaeQfxwLbnMdbWn43Yp+OQbvOMPVUERrWcrya8md9M2ATfxEKofexoslqqt+H
YCxr06MSy2rHlxHQITzlmlf4afF3sRmZRkJoMuuFYrK4pwc4A5JJj9UficS/xQuK
TaCEWgqClY2FCX60Zf6xs0GY8ZVs9hslVhjrwOtqFrmO04ff4fMBZGmRudrrXfjA
r1EM/UJ0Di2o7djxI84n
=SHNF
-----END PGP SIGNATURE-----
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment