Skip to content

Instantly share code, notes, and snippets.

@pipermerriam
Created May 10, 2017 16:14
Show Gist options
  • Save pipermerriam/7f36f9c9446d4fb8d0e6d842d7212177 to your computer and use it in GitHub Desktop.
Save pipermerriam/7f36f9c9446d4fb8d0e6d842d7212177 to your computer and use it in GitHub Desktop.

Section 1 - Table of Contents

From April 23rd through May 7th of 2017, Piper Merriam conducted an audit of the smart contracts that make up the Aragon service. The findings of the audit are presented in this document.

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

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

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 Aragon development team to determine the appropriate response to each issue.

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

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

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

This audit will look for exploitable security vulnerabilities.

This audit uses the following terminology.

Measurement of the testing code coverage.

No tests.

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

The tests cover all major functionality.

The tests cover all code paths.

Measurement of magnitude of an issue.

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.

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.

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.

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.

The source code under review can be found in the public github repository https://github.com/AragonOne/aragon-network-token under commit hash 9565691d8e39d06489a676159a6cc2104ac97dee.

The following source files were included in this audit.

  • ./contracts/ANTPlaceholder.sol
  • ./contracts/ANT.sol
  • ./contracts/AragonTokenSale.sol
  • ./contracts/MiniMeIrrevocableVestedToken.sol
  • ./contracts/MiniMeToken.sol
  • ./contracts/SaleWallet.sol

The sha256 hash of these files at the time of the audit is as follows:

$ shasum -a 256 contracts/ANPlaceholder.sol contracts/ANT.sol contracts/AragonTokenSale.sol contracts/MiniMeToken.sol contracts/MiniMeIrrevocableVestedToken.sol contracts/SaleWallet.sol
24a1bb5f3cc51d6c5df20acad56c4a594ec2873b1d9e904d864384f8272fc2a7  contracts/ANPlaceholder.sol
0ef806c724f82f6e7df64acd2ea25d18cda6d0c7aea7a92d3ce84bb041d97dbb  contracts/ANT.sol
eebbe2d6c647c7d4cd378448b6a0515dbe5d1395cdc37648c46b24504a247f46  contracts/AragonTokenSale.sol
f8ce477134111d7ed374cb4e8e9042bd0f84b94ecd350c36c0a35782e6eaf15c  contracts/MiniMeToken.sol
189b73679628836a23cc976040658999a0d637217164c4471d764f81cccd6e93  contracts/MiniMeIrrevocableVestedToken.sol
e1afc8285ce10b634e922d97addb63dbfeedbf41620546b40b1e195715b2b13c  contracts/SaleWallet.sol

The contracts make use of a small number of the re-usable contracts provided by Zeppelin. These contracts were not covered by this audit.

This section contains higher level issues and analysis.

This audit identified multiple issues that require patching prior to the contracts being deployed and used. The changes necessary to fix these issues are likely to change a sufficiently large amount of code to warrant an additional follow up audit.

Rather than spend additional time identifying additional issues, it was agreed to close this audit out early to allow those changes to be made prior to investing more time reviewing code.

The code reviewed in this audit is well written. It is dense with comments explaining various nuances of the code. The code is consistently styled and conforms to the code style that is generally used and accepted in the smart contract development community.

The contracts themselves are well architected and organized. Commonly used logic has been encapsulated in internal utility functions.

None

The MiniMeToken contract implements privileged functions destroyTokens and generateTokens which can be used to destroy tokens in any user account, or issue new tokens to any account respectively.

Those wishing to hold tokens controlled by this contract should be aware of this.

According to the Aragon team, this functionality will only be accessible through a multisignature account.

The IrrevokablyVestedToken token is an extension to the MiniMeToken which adds functionality to allow for granting tokens that will vest over some period of time. This adds roughly 200 lines of code to an already complex contract.

This vesting logic could instead be written as a stand-alone contract which holds the token balance that has not yet vested, releasing those tokens on the predetermined vesting schedule.

Recommend removing this functionality and replacing it with a stand-alone contract which handles the vesting logic independent from the token contract.

It should be noted that a stand-alone vesting contract would need to account for cloning mechanism.

The majority of the public functions do not include the public visibility specifier in their signature. While public is the default visibility specifier, it is the opinion of this audit that explicitlydeclaring visibility specifiers leads to more readable code.

Recommend that all functions have explicit visibility specifiers.

The IrrevokablyVestedToken contract provides a mechanism for granting accounts a number of tokens which will vest over some period of time. This mechanism works by transferring the full token amount into their account, but restricting the amount the account is allowed to transfer based on whether those tokens have fully vested.

If a token contract is cloned using the createCloneToken functionality, any tokens that have not yet vested will immediately become available in the cloned contract regardless of the declared vesting schedule. This is due to the fact that the balanceOf and balanceOfAt queries to the parent contract will return the full token balance included non-vested tokens.

Recommend full removal of this vesting logic from the contract into a stand-alone contract which actually holds the unvested tokens.

None

The test suite for this project is run using the truffle development framework. The project includes a ./test.sh shell script for running the tests.

Running the ./test.sh command resulting in a successful passing test suite.

Many of the tests involve use of a mock contract which overrides some basic internals necessary to test certain functionality such as overriding the current block number.

  • coverage: low

The MiniMeToken contract test suite is found in the file ./tests/StandardToken.js. The tests cover the most basic ERC20 functionality.

This suite does not cover various edge cases like attempting to transfer tokens to 0x0, historical balance queries, or token cloning.

  • coverage: low

The IrrevokablyVestedToken contract test suite is found in the file ./tests/IrrevokablyVestedToken.js. This suite appears to cover the token grant and vesting functionality as well as the basic underlying ERC20 functionality.

This suite does not cover various edge cases like attempting to transfer tokens to 0x0, historical balance queries, or token cloning.

  • coverage: good

The AragonTokenSale contract is tested across the ./test/TestTokenPresale.sol and ./test/TestTokenSale.sol files. The test suite appears to cover all major functionality of the contract before, during, and after the sale.

The MiniMeToken contract uses a variable transfersEnabled to determine whether or not the contract currently allows token transfers. Functions which check this value use the following code to do so.

if (!transfersEnabled) throw;

All such checks always check !transfersEnabled.

Recommend renaming this to transfersDisabled to remove the need to negate the value prior to checking. This should improve contract readability as well as a very small reduction in gas costs.

The MiniMeToken contract disallows transferring to the 0x0 address. This restriction seems arbitrary given that there are an effectively infinite number of possible addresses that could be used to burn tokens.

Recommend considering removal of this restriction, and potentially special casing it to delegate to the destroyTokens functionality when 0x0 is the target of a transfer.

Note: The intention of this blacklisting is to prevent user mistakes. It's not clear whether this is a good design choice so it will be up to the Aragon development team to determine the appropriate course of action.

The MiniMeToken contract contains the following code found at line 159

if (!Controller(controller).onTransfer(_from, _to, _amount))
throw;

This style of code is prone to allowing errors to be introduced in the event that an additional statement is being added to the if conditional. Someone refactoring this code could easily do the following.

if (!Controller(controller).onTransfer(_from, _to, _amount))
doExtraThing();
throw;

Since the conditional statement does not use braces, the throw statement is now outside of the conditional and will always be triggered.

Recommend either combining these to a single line, or wrapping the throw in braces to explicitlyblock off the portion of code that is contained within the conditional.

The createCloneToken function takes a _snapshotBlock parameter to determine what block the new token contract should begin at. If the number passed in with this block is greater than the current block number then the value is overridden to be the current block number.

This functionality does not pass the "Principle of least astonishment" litmus test.

Recommend making the following changes.

  1. If _snapshotBlock is in the future, throw an error.
  2. Add another signature for the createCloneToken function which defaults this value to the current block number (example below).
    function createCloneToken(
        address _parentToken,
        string _tokenName,
        uint8 _decimalUnits,
        string _tokenSymbol,
        bool _transfersEnabled
    ) returns (MiniMeToken) {
        return createCloneToken(_parentToken, block.number, _tokenName, _decimalUnits, _tokenSymbol, _transfersEnabled);
    }

See: https://en.wikipedia.org/wiki/Principle_of_least_astonishment

Line 95 of the IrrevokablyVestedToken checks that the number of tokens grants given to an account do not exceed a hard-coded threshold to avoid denial of service attacks due to the block gas limit.

Recommend changing this line to make use of the tokenGrantsCount helper function instead of checking the value directly.

The IrrevokablyVestedToken.tokenGrant function declares named return value in its function signature. Values are assigned to these variable within the function body. The function does not include a return statement. In these cases solidity will implicitly return the values assigned to the variable.

This implicit return style is more difficult to read since at a glance, the function does not appear to return any data.

Recommend changing this to use an explicit return statement to improve readability.

The SaleWallet.withdraw function sends all of the collected funds to the declared multisig address and then self destructs. Any funds received to this address after this function has been called will be forever lost.

Recommend removal of the self destruction or separating this mechanism from the withdraw mechanism.

In line 254 of the AragonTokenSale contract the following code is used to calculate the number of tokens being purchased.

uint256 boughtTokens = safeMul(msg.value, getPrice(getBlockNumber())); // Calculate how many tokens bought

Intuitively, number if tokens purchased would be calculated by dividing the amount paid by the price per token. The code above, instead multiplies the amount paid by the value returned from getPrice.

The getPrice method would intuitively return the unit-of-value-per-token. Instead, this function returns the inverse of this, the number of tokens-per-unit-of-value.

Recommend renaming this function to something which more accurately represents the returned value, such as getTokensPerWei.

The MiniMeToken contract includes functionality to fork the token contract via the createCloneToken function. This creates a new token contract which retains a reference to the contract from which it was cloned. In addition, the contract includes functionality for querying historical token balances for any historical block number via the balanceOfAt function.

This function will currently return 0 for all balance queries for block numbers prior to the creation of the token contract, even in situations where the token contract has a parent and there is historical balance data for the block in question. This is due to the following conditional statement.

if (_blockNumber < creationBlock) {
    return 0;

The creationBlock is always set to the block number during which the token contract was deployed.

Recommend modifying this conditional to account for the existence of a parent token contract, in which case the parent token contract should be queried.

The MiniMeToken contract includes functionality to fork the token contract via the createCloneToken function. This creates a new token contract which retains a reference to the contract from which it was cloned. In addition, the contract includes functionality for querying historical token supply for any historical block number via the totalSupplyAt function.

This function will currently return 0 for all supply queries for block numbers prior to the creation of the token contract, even in situations where the token contract has a parent and there is historical supply data for the block in question. This is due to the following conditional statement.

if (_blockNumber < creationBlock) {
    return 0;

The creationBlock is always set to the block number during which the token contract was deployed.

Recommend modifying this conditional to account for the existence of a parent token contract, in which case the parent token contract should be queried.

The IrrevokablyVestedToken is an extension of the MiniMeToken. The MiniMeToken implements a createCloneToken function which delegates to a factory contract to deploy a clone of the token contract. The codebase currently includes a MiniMeTokenFactory. There does not appear to be a IrrevokablyVestedTokenFactory present in the codebase.

Currently, it appears that any clones of the IrrevokablyVestedToken contract will be of type MiniMeToken.

Recommend ensuring that this is the desired functionality and ensuring that the IrrevokablyVestedToken contract is using an appropriate factory.

None

-----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 - [Terminology](#heading-2.3)
* 2.3.1 - [Code Coverage Terms](#heading-2.3.1)
* 2.3.1.1 - [**untested**](#heading-2.3.1.1)
* 2.3.1.2 - [**low**](#heading-2.3.1.2)
* 2.3.1.3 - [**good**](#heading-2.3.1.3)
* 2.3.1.4 - [**excellent**](#heading-2.3.1.4)
* 2.3.2 - [Severity Terms](#heading-2.3.2)
* 2.3.2.1 - [**minor**](#heading-2.3.2.1)
* 2.3.2.2 - [**medium**](#heading-2.3.2.2)
* 2.3.2.3 - [**major**](#heading-2.3.2.3)
* 2.3.2.4 - [**critical**](#heading-2.3.2.4)
* 3 - [Overview](#heading-3)
* 3.1 - [Source Code](#heading-3.1)
* 3.2 - [Contracts](#heading-3.2)
* 3.2.1 - [Zeppelin Base Contracts](#heading-3.2.1)
* 4 - [General Findings](#heading-4)
* 4.1 - [General Thoughts](#heading-4.1)
* 4.1.1 - [High Quality Code](#heading-4.1.1)
* 4.2 - [Minor Issues](#heading-4.2)
* 4.3 - [Medium Issues](#heading-4.3)
* 4.3.1 - [The `MiniMeToken.destroyTokens` and `MiniMeToken.generateTokens` functions give centralized control to the operators of the token contract.](#heading-4.3.1)
* 4.3.2 - [The `IrrevokablyVestedToken` extension to the `MiniMeToken` may not be justified by the added complexity.](#heading-4.3.2)
* 4.3.3 - [Missing visibility specifiers](#heading-4.3.3)
* 4.4 - [Major Issues](#heading-4.4)
* 4.4.1 - [Token grants will become fully vested when the token contract is cloned.](#heading-4.4.1)
* 4.5 - [Critical Issues](#heading-4.5)
* 4.6 - [Test Coverage Analysis](#heading-4.6)
* 4.6.1 - [`MiniMeToken`](#heading-4.6.1)
* 4.6.2 - [`IrrevokablyVestedToken`](#heading-4.6.2)
* 4.6.3 - [`AragonTokenSale`](#heading-4.6.3)
* 5 - [Detailed Findings](#heading-5)
* 5.1 - [Minor Issues](#heading-5.1)
* 5.1.1 - [The `transfersEnabled` variable in the `MiniMeToken` could be renamed to improve readability.](#heading-5.1.1)
* 5.1.2 - [Remove blacklist of `0x0` address as transfer target from `MiniMeToken`.](#heading-5.1.2)
* 5.1.3 - [Multiline `if` statement without braces.](#heading-5.1.3)
* 5.1.4 - [Non-intuitive behavior for `_snapshotBlock` parameter when cloning a token.](#heading-5.1.4)
* 5.1.5 - [Code duplication in `IrrevokablyVestedToken`](#heading-5.1.5)
* 5.1.6 - [`IrrevokablyVestedToken.tokenGrant` uses implicit return values.](#heading-5.1.6)
* 5.2 - [Medium Issues](#heading-5.2)
* 5.2.1 - [`SaleWallet.withdraw` function suicide could result in lost funds.](#heading-5.2.1)
* 5.2.2 - [`AragonTokenSale` contract uses non-intuitive variable and method names for token allocation during sale.](#heading-5.2.2)
* 5.3 - [Major Issues](#heading-5.3)
* 5.3.1 - [`MiniMeToken.balanceOfAt` always returns `0` for any cloned token contract for blocks prior to creation of the cloned token.](#heading-5.3.1)
* 5.3.2 - [`MiniMeToken.totalSupplyAt` always returns `0` for any cloned token contract for blocks prior to creation of the cloned token.](#heading-5.3.2)
* 5.3.3 - [Clones of the `IrrevokablyVestedToken` contract will be of type `MiniMeToken`.](#heading-5.3.3)
* 5.4 - [Critical Issues](#heading-5.4)
# <a id="heading-2"/> Section 2 - Introduction
From April 23rd through May 7th of 2017, Piper Merriam conducted an
audit of the smart contracts that make up the Aragon service. The findings of
the audit are presented in this document.
This audit was performed under a contracted hourly rate with no other compensation.
## <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 Aragon
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 exploitable security vulnerabilities.
## <a id="heading-2.3"/> 2.3 Terminology
This audit uses the following terminology.
### <a id="heading-2.3.1"/> 2.3.1 Code Coverage Terms
Measurement of the testing code coverage.
#### <a id="heading-2.3.1.1"/> 2.3.1.1 **untested**
No tests.
#### <a id="heading-2.3.1.2"/> 2.3.1.2 **low**
The tests do not cover some set of non-trivial functionality.
#### <a id="heading-2.3.1.3"/> 2.3.1.3 **good**
The tests cover all major functionality.
#### <a id="heading-2.3.1.4"/> 2.3.1.4 **excellent**
The tests cover all code paths.
### <a id="heading-2.3.2"/> 2.3.2 Severity Terms
Measurement of magnitude of an issue.
#### <a id="heading-2.3.2.1"/> 2.3.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.3.2.2"/> 2.3.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.3.2.3"/> 2.3.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.3.2.4"/> 2.3.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 source code under review can be found in the public github repository
[https://github.com/AragonOne/aragon-network-token](https://github.com/AragonOne/aragon-network-token)
under commit hash `9565691d8e39d06489a676159a6cc2104ac97dee`.
## <a id="heading-3.2"/> 3.2 Contracts
The following source files were included in this audit.
* `./contracts/ANTPlaceholder.sol`
* `./contracts/ANT.sol`
* `./contracts/AragonTokenSale.sol`
* `./contracts/MiniMeIrrevocableVestedToken.sol`
* `./contracts/MiniMeToken.sol`
* `./contracts/SaleWallet.sol`
The sha256 hash of these files at the time of the audit is as follows:
```
$ shasum -a 256 contracts/ANPlaceholder.sol contracts/ANT.sol contracts/AragonTokenSale.sol contracts/MiniMeToken.sol contracts/MiniMeIrrevocableVestedToken.sol contracts/SaleWallet.sol
24a1bb5f3cc51d6c5df20acad56c4a594ec2873b1d9e904d864384f8272fc2a7 contracts/ANPlaceholder.sol
0ef806c724f82f6e7df64acd2ea25d18cda6d0c7aea7a92d3ce84bb041d97dbb contracts/ANT.sol
eebbe2d6c647c7d4cd378448b6a0515dbe5d1395cdc37648c46b24504a247f46 contracts/AragonTokenSale.sol
f8ce477134111d7ed374cb4e8e9042bd0f84b94ecd350c36c0a35782e6eaf15c contracts/MiniMeToken.sol
189b73679628836a23cc976040658999a0d637217164c4471d764f81cccd6e93 contracts/MiniMeIrrevocableVestedToken.sol
e1afc8285ce10b634e922d97addb63dbfeedbf41620546b40b1e195715b2b13c contracts/SaleWallet.sol
```
### <a id="heading-3.2.1"/> 3.2.1 Zeppelin Base Contracts
The contracts make use of a small number of the re-usable contracts provided by
Zeppelin. These contracts were not covered by this audit.
# <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
This audit identified multiple issues that require patching prior to the
contracts being deployed and used. The changes necessary to fix these issues
are likely to change a sufficiently large amount of code to warrant an
additional follow up audit.
Rather than spend additional time identifying additional issues, it was
agreed to close this audit out early to allow those changes to be made prior to
investing more time reviewing code.
### <a id="heading-4.1.1"/> 4.1.1 High Quality Code
The code reviewed in this audit is well written. It is dense with comments
explaining various nuances of the code. The code is consistently styled and
conforms to the code style that is generally used and accepted in the smart
contract development community.
The contracts themselves are well architected and organized. Commonly used
logic has been encapsulated in internal utility functions.
## <a id="heading-4.2"/> 4.2 Minor Issues
None
## <a id="heading-4.3"/> 4.3 Medium Issues
### <a id="heading-4.3.1"/> 4.3.1 The `MiniMeToken.destroyTokens` and `MiniMeToken.generateTokens` functions give centralized control to the operators of the token contract.
The `MiniMeToken` contract implements privileged functions `destroyTokens` and
`generateTokens` which can be used to destroy tokens in any user account, or
issue new tokens to any account respectively.
Those wishing to hold tokens controlled by this contract should be aware of this.
According to the Aragon team, this functionality will only be accessible
through a multisignature account.
### <a id="heading-4.3.2"/> 4.3.2 The `IrrevokablyVestedToken` extension to the `MiniMeToken` may not be justified by the added complexity.
The `IrrevokablyVestedToken` token is an extension to the `MiniMeToken` which
adds functionality to allow for granting tokens that will vest over some period
of time. This adds roughly 200 lines of code to an already complex contract.
This *vesting* logic could instead be written as a stand-alone contract which
holds the token balance that has not yet vested, releasing those tokens on the
predetermined vesting schedule.
Recommend removing this functionality and replacing it with a stand-alone
contract which handles the vesting logic independent from the token contract.
It should be noted that a stand-alone vesting contract would need to account
for cloning mechanism.
### <a id="heading-4.3.3"/> 4.3.3 Missing visibility specifiers
The majority of the `public` functions do not include the `public` visibility
specifier in their signature. While `public` is the *default* visibility
specifier, it is the opinion of this audit that explicitlydeclaring
visibility specifiers leads to more readable code.
Recommend that all functions have explicit visibility specifiers.
## <a id="heading-4.4"/> 4.4 Major Issues
### <a id="heading-4.4.1"/> 4.4.1 Token grants will become fully vested when the token contract is cloned.
The `IrrevokablyVestedToken` contract provides a mechanism for granting
accounts a number of tokens which will vest over some period of time. This
mechanism works by transferring the *full* token amount into their account, but
restricting the amount the account is allowed to transfer based on whether
those tokens have fully vested.
If a token contract is cloned using the `createCloneToken` functionality, any
tokens that have not yet vested will immediately become available in the cloned
contract regardless of the declared vesting schedule. This is due to the fact
that the `balanceOf` and `balanceOfAt` queries to the parent contract will
return the full token balance included non-vested tokens.
Recommend full removal of this vesting logic from the contract into a
stand-alone contract which actually holds the unvested tokens.
## <a id="heading-4.5"/> 4.5 Critical Issues
None
## <a id="heading-4.6"/> 4.6 Test Coverage Analysis
The test suite for this project is run using the truffle development framework.
The project includes a `./test.sh` shell script for running the tests.
Running the `./test.sh` command resulting in a successful passing test suite.
Many of the tests involve use of a *mock* contract which overrides some basic
internals necessary to test certain functionality such as overriding the
current block number.
### <a id="heading-4.6.1"/> 4.6.1 `MiniMeToken`
* **coverage**: *low*
The `MiniMeToken` contract test suite is found in the file
`./tests/StandardToken.js`. The tests cover the most basic ERC20
functionality.
This suite does not cover various edge cases like attempting to transfer tokens
to `0x0`, historical balance queries, or token cloning.
### <a id="heading-4.6.2"/> 4.6.2 `IrrevokablyVestedToken`
* **coverage**: *low*
The `IrrevokablyVestedToken` contract test suite is found in the file
`./tests/IrrevokablyVestedToken.js`. This suite appears to cover the token
grant and vesting functionality as well as the basic underlying ERC20
functionality.
This suite does not cover various edge cases like attempting to transfer tokens
to `0x0`, historical balance queries, or token cloning.
### <a id="heading-4.6.3"/> 4.6.3 `AragonTokenSale`
* **coverage**: *good*
The `AragonTokenSale` contract is tested across the
`./test/TestTokenPresale.sol` and `./test/TestTokenSale.sol` files. The test
suite appears to cover all major functionality of the contract before, during,
and after the sale.
# <a id="heading-5"/> Section 5 - Detailed Findings
## <a id="heading-5.1"/> 5.1 Minor Issues
### <a id="heading-5.1.1"/> 5.1.1 The `transfersEnabled` variable in the `MiniMeToken` could be renamed to improve readability.
The `MiniMeToken` contract uses a variable `transfersEnabled` to determine
whether or not the contract currently allows token transfers. Functions which check this value use the following code to do so.
```javascript
if (!transfersEnabled) throw;
```
All such checks always check `!transfersEnabled`.
Recommend renaming this to `transfersDisabled` to remove the need to negate the
value prior to checking. This should improve contract readability as well as a
very small reduction in gas costs.
### <a id="heading-5.1.2"/> 5.1.2 Remove blacklist of `0x0` address as transfer target from `MiniMeToken`.
The `MiniMeToken` contract disallows transferring to the `0x0` address. This
restriction seems arbitrary given that there are an effectively infinite number
of possible addresses that could be used to *burn* tokens.
Recommend *considering* removal of this restriction, and potentially special
casing it to delegate to the `destroyTokens` functionality when `0x0` is the
target of a transfer.
> Note: The intention of this blacklisting is to prevent user mistakes. It's not
clear whether this is a good design choice so it will be up to the Aragon
development team to determine the appropriate course of action.
### <a id="heading-5.1.3"/> 5.1.3 Multiline `if` statement without braces.
The `MiniMeToken` contract contains the following code found at line 159
```javascript
if (!Controller(controller).onTransfer(_from, _to, _amount))
throw;
```
This style of code is prone to allowing errors to be introduced in the event
that an additional statement is being added to the `if` conditional. Someone
refactoring this code could easily do the following.
```javascript
if (!Controller(controller).onTransfer(_from, _to, _amount))
doExtraThing();
throw;
```
Since the conditional statement does not use braces, the `throw` statement is
now outside of the conditional and will always be triggered.
Recommend either combining these to a single line, or wrapping the `throw` in
braces to explicitlyblock off the portion of code that is contained within
the conditional.
### <a id="heading-5.1.4"/> 5.1.4 Non-intuitive behavior for `_snapshotBlock` parameter when cloning a token.
The `createCloneToken` function takes a `_snapshotBlock` parameter to determine
what block the new token contract should begin at. If the number passed in
with this block is greater than the current block number then the value is
overridden to be the current block number.
This functionality does not pass the "Principle of least astonishment" litmus
test.
Recommend making the following changes.
1. If `_snapshotBlock` is in the future, throw an error.
2. Add another signature for the `createCloneToken` function which defaults this value to the current block number (example below).
```javascript
function createCloneToken(
address _parentToken,
string _tokenName,
uint8 _decimalUnits,
string _tokenSymbol,
bool _transfersEnabled
) returns (MiniMeToken) {
return createCloneToken(_parentToken, block.number, _tokenName, _decimalUnits, _tokenSymbol, _transfersEnabled);
}
```
See: https://en.wikipedia.org/wiki/Principle_of_least_astonishment
### <a id="heading-5.1.5"/> 5.1.5 Code duplication in `IrrevokablyVestedToken`
Line 95 of the `IrrevokablyVestedToken` checks that the number of tokens grants
given to an account do not exceed a hard-coded threshold to avoid denial of
service attacks due to the block gas limit.
Recommend changing this line to make use of the `tokenGrantsCount` helper
function instead of checking the value directly.
### <a id="heading-5.1.6"/> 5.1.6 `IrrevokablyVestedToken.tokenGrant` uses implicit return values.
The `IrrevokablyVestedToken.tokenGrant` function declares named return value in
its function signature. Values are assigned to these variable within the
function body. The function does not include a `return` statement. In these
cases solidity will implicitly return the values assigned to the variable.
This implicit return style is more difficult to read since at a glance, the
function does not appear to return any data.
Recommend changing this to use an explicit `return` statement to improve
readability.
## <a id="heading-5.2"/> 5.2 Medium Issues
### <a id="heading-5.2.1"/> 5.2.1 `SaleWallet.withdraw` function suicide could result in lost funds.
The `SaleWallet.withdraw` function sends all of the collected funds to the
declared `multisig` address and then self destructs. Any funds received to
this address after this function has been called will be forever lost.
Recommend removal of the self destruction or separating this mechanism from the
withdraw mechanism.
### <a id="heading-5.2.2"/> 5.2.2 `AragonTokenSale` contract uses non-intuitive variable and method names for token allocation during sale.
In line 254 of the `AragonTokenSale` contract the following code is used to
calculate the number of tokens being purchased.
```
uint256 boughtTokens = safeMul(msg.value, getPrice(getBlockNumber())); // Calculate how many tokens bought
```
Intuitively, number if tokens purchased would be calculated by dividing the
amount paid by the price per token. The code above, instead *multiplies* the
amount paid by the value returned from `getPrice`.
The `getPrice` method would intuitively return the unit-of-value-per-token.
Instead, this function returns the inverse of this, the number of
tokens-per-unit-of-value.
Recommend renaming this function to something which more accurately represents
the returned value, such as `getTokensPerWei`.
## <a id="heading-5.3"/> 5.3 Major Issues
### <a id="heading-5.3.1"/> 5.3.1 `MiniMeToken.balanceOfAt` always returns `0` for any cloned token contract for blocks prior to creation of the cloned token.
The `MiniMeToken` contract includes functionality to fork the token contract
via the `createCloneToken` function. This creates a new token contract which
retains a reference to the contract from which it was cloned. In addition, the
contract includes functionality for querying historical token balances for any
historical block number via the `balanceOfAt` function.
This function will currently return `0` for all balance queries for block
numbers prior to the creation of the token contract, even in situations where
the token contract has a parent and there is historical balance data for the
block in question. This is due to the following conditional statement.
```
if (_blockNumber < creationBlock) {
return 0;
```
The `creationBlock` is always set to the block number during which the token
contract was deployed.
Recommend modifying this conditional to account for the existence of a parent
token contract, in which case the parent token contract should be queried.
### <a id="heading-5.3.2"/> 5.3.2 `MiniMeToken.totalSupplyAt` always returns `0` for any cloned token contract for blocks prior to creation of the cloned token.
The `MiniMeToken` contract includes functionality to fork the token contract
via the `createCloneToken` function. This creates a new token contract which
retains a reference to the contract from which it was cloned. In addition, the
contract includes functionality for querying historical token supply for any
historical block number via the `totalSupplyAt` function.
This function will currently return `0` for all supply queries for block
numbers prior to the creation of the token contract, even in situations where
the token contract has a parent and there is historical supply data for the
block in question. This is due to the following conditional statement.
```
if (_blockNumber < creationBlock) {
return 0;
```
The `creationBlock` is always set to the block number during which the token
contract was deployed.
Recommend modifying this conditional to account for the existence of a parent
token contract, in which case the parent token contract should be queried.
### <a id="heading-5.3.3"/> 5.3.3 Clones of the `IrrevokablyVestedToken` contract will be of type `MiniMeToken`.
The `IrrevokablyVestedToken` is an extension of the `MiniMeToken`. The
`MiniMeToken` implements a `createCloneToken` function which delegates to a
factory contract to deploy a clone of the token contract. The codebase
currently includes a `MiniMeTokenFactory`. There does not appear to be a
`IrrevokablyVestedTokenFactory` present in the codebase.
Currently, it appears that any clones of the `IrrevokablyVestedToken` contract
will be of type `MiniMeToken`.
Recommend ensuring that this is the desired functionality and ensuring that the
`IrrevokablyVestedToken` contract is using an appropriate factory.
## <a id="heading-5.4"/> 5.4 Critical Issues
None
-----BEGIN PGP SIGNATURE-----
Version: Keybase OpenPGP v2.0.70
Comment: https://keybase.io/crypto
wsFcBAABCgAGBQJZEzxMAAoJEI0iZdXOvoIvCk8P/jJDQLsv8i/KwsY6ofVUQGIj
CYt4JCjUB3P7xUZ4XvilBSlAUVlP9oqTJBubdRic4DdXsDrODKydSGF4QBjYP+s9
cM+D4fMVJUAeVDnjWVr573Bar0+W6YkKXokpEFTh79XOXuqg96vB80Fw4LnczWA4
RmOvuyLqnRGHzzp1uy/MkLK+7y7f1RQtNtooEYHydbqG61GFcUmpVUjQjOFIbDXX
MdqiB79bc9+Wr1iSg0QJbtWAmzPpFJ1YUeCYBSclXjoRpzJJAKE2sp+I2dOJnlHD
iKUNqSNx9Igi4aTLP/eBb3D0NqT0Mclnvt+V/P7ym5/DjJLQbUU6dar5JkWRCOPi
B+6fZFlDw08nOzLhrVCamJcF3rU3r/UlZ/oj1s0+Brxt2nBwE0o0vi5qAA9LvzaH
pvo4DfbyqBJ9ZF+BG2kPMYOHrYSiOntOFF0WoAhnRHC5glq1jknXgF04sRhO3qMx
7ifm3BPGc8DOqsgE9rjM+yCY3gIKjFcTINeACBxR7W+vUtONTOm0NxrRrM/XQr33
aIco712cVGgTUTdSDgRYTOxr/IAViFbEkUN2xLu35Hc3dHx0TFX4QA1fFu5t1OPq
a8p2SFwKXPN4g6ocAMct0UkXiKwhi5NQclLfNKTgS0hwenz3xYUzfDHWZGw/SRIH
uW1JvN70wgoSx/oLApyX
=xBUA
-----END PGP SIGNATURE-----
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment