- 1 - Table of Contents
- 2 - Introduction
- 2.1 - Authenticity
- 2.2 - Audit Goals and Focus
- 2.2.1 - Sound Architecture
- 2.2.2 - Smart Contract Best Practices
- 2.2.3 - Code Correctness
- 2.2.4 - Code Quality
- 2.2.5 - Security
- 2.3 - Terminology
- 2.3.1 - Code Coverage Terms
- 2.3.2 - Severity Terms
- 3 - Overview
- 3.1 - Source Code
- 3.2 - Contracts
- 3.2.1 - Zeppelin Base Contracts
- 4 - General Findings
- 4.1 - General Thoughts
- 4.1.1 - High Quality Code
- 4.2 - Minor Issues
- 4.3 - Medium Issues
- 4.4 - Major Issues
- 4.5 - Critical Issues
- 4.6 - Test Coverage Analysis
- 4.6.1 -
MiniMeToken - 4.6.2 -
IrrevokablyVestedToken - 4.6.3 -
AragonTokenSale
- 4.6.1 -
- 4.1 - General Thoughts
- 5 - Detailed Findings
- 5.1 - Minor Issues
- 5.1.1 - The
transfersEnabledvariable in theMiniMeTokencould be renamed to improve readability. - 5.1.2 - Remove blacklist of
0x0address as transfer target fromMiniMeToken. - 5.1.3 - Multiline
ifstatement without braces. - 5.1.4 - Non-intuitive behavior for
_snapshotBlockparameter when cloning a token. - 5.1.5 - Code duplication in
IrrevokablyVestedToken - 5.1.6 -
IrrevokablyVestedToken.tokenGrantuses implicit return values.
- 5.1.1 - The
- 5.2 - Medium Issues
- 5.3 - Major Issues
- 5.3.1 -
MiniMeToken.balanceOfAtalways returns0for any cloned token contract for blocks prior to creation of the cloned token. - 5.3.2 -
MiniMeToken.totalSupplyAtalways returns0for any cloned token contract for blocks prior to creation of the cloned token. - 5.3.3 - Clones of the
IrrevokablyVestedTokencontract will be of typeMiniMeToken.
- 5.3.1 -
- 5.4 - Critical Issues
- 5.1 - Minor Issues
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.
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
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.
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.
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.
- If
_snapshotBlockis in the future, throw an error. - Add another signature for the
createCloneTokenfunction 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.
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.
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.
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.
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