Section 1 - Table of Contents
- 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 - About EtherRoll
- 2.4 - Terminology
- 3 - Overview
- 3.1 - Source Code
- 3.2 - Contracts
- 3.2.1 -
EtherollToken
- 3.2.2 -
EtherollCrowdfund
- 3.2.1 -
- 4 - Main Findings
- 5 - Detailed Findings
- 5.1 - Minor Issues
- 5.2 - Medium Issues
- 5.2.1 -
EtherollToken.transfer
throws on failure conditions. - 5.2.2 -
EtherollToken
does not implement an ERC20 compliantTransfer
event - 5.2.3 -
EtherollToken
does not implement an ERC20 compliantApproval
event - 5.2.4 - Logging of incorrect data within the
EtherollCrowdfund.safeWithdraw
function. - 5.2.5 -
EtherollCrowdfund.safeWithdraw
will withdraw user funds to theetherollBeneficiary
address.
- 5.2.1 -
- 5.3 - Major Issues
- 5.4 - Critical Issues
- 6 - Follow Up Audit
- 6.1 - Overview
- 6.2 - Source Code
- 6.3 - Terminology
- 6.3.1 - Status: Resolved
- 6.3.2 - Status: Unchanged
- 6.3.3 - Status: Partially Resolved
- 6.4 - Main Findings
- 6.4.1 - Minor Issues
- 6.4.2 - Medium Issues
- 6.4.3 - Major Issues
- 6.4.4 - Critical Issues
- 6.5 - Detailed Findings
- 6.5.1 - Minor Issues
- 6.5.2 - Medium Issues
- 6.5.2.1 -
EtherollToken.transfer
throws on failure conditions. - 6.5.2.2 -
EtherollToken
does not implement an ERC20 compliantTransfer
event - 6.5.2.3 -
EtherollToken
does not implement an ERC20 compliantApproval
event - 6.5.2.4 - Logging of incorrect data within the
EtherollCrowdfund.safeWithdraw
function. - 6.5.2.5 -
EtherollCrowdfund.safeWithdraw
will withdraw user funds to theetherollBeneficiary
address.
- 6.5.2.1 -
- 6.5.3 - Major Issues
- 6.5.4 -
EtherollToken.priviledgedAddressBurnUnsoldCoins
can be used to burn any tokens. - 6.5.5 - Critical Issues
Section 2 - Introduction
From January 9th through January 12th of 2017, Piper Merriam conducted an audit of the primary Crowdsale smart contract for the Etheroll service. The findings of the audit are presented in this document.
I, Piper Merriam have no stake in the success or failure of EtherRoll. This audit was performed under a contracted hourly rate with no other compensation.
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.
2.2 Audit Goals and Focus
2.2.1 Sound Architecture
This audit includes assessments of the overall architecture and design choices. Given the subjective nature of these assessments it will be up to the EtherRoll development team to determine whether any changes should be made.
2.2.2 Smart Contract Best Practices
This audit will evaluate whether the codebase follows the current established best practices for smart contract development.
2.2.3 Code Correctness
This audit will evaluate whether the code does what it is intended to do.
2.2.4 Code Quality
This audit will evaluate whether the code has been written in a way that ensures readability and maintainability.
2.2.5 Security
This audit will look for any exploitable security vulnerabilities, or other potential threats to either the operators of EtherRoll or its users.
2.3 About EtherRoll
EtherRoll is a gambling service on the Ethereum blockchain.
2.4 Terminology
This audit uses the following terminology.
2.4.1 Severity
Measurement of magnitude of an issue.
2.4.1.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.1.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.1.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.1.4 critical
Critical issues are directly exploitable bugs or security vulnerabilities.
Left unaddressed these issues are highly likely or guaranteed to cause major problems or potentially a full failure in the operations of the contract.
Section 3 - Overview
3.1 Source Code
The Crowdsale contract source code was made available through a private
repository on github. The code being evaluated can be found under the commit
hash ecf962d20dfcf190d462018dc61c7eec8b2c2790
The code being evaluated is spread across the following three solidity source files
./contracts/crowdsale-limited.sol
../contracts/limited-token.sol
.
The sha256 of that contract source is as follows.
$ shasum -a 256 contracts/crowdsale-limited.sol contracts/limited-token.sol
a67adf04735bdd331b7920aae04213ede60fb5ce975dc911664a72f895c0fa6e contracts/crowdsale-limited.sol
490e732a3234a0279d3ad80b13331f5dc33cd87d60a3710893952ce0eff8a371 contracts/limited-token.sol
3.2 Contracts
The codebase contains two primary contracts.
3.2.1 EtherollToken
An implementation of the ERC20 token standard with some additional functionality.
The added features are:
- Ability to freeze individual token assets.
- Ability to freeze all token assets.
- Ability to
selfdestruct
the contract. - Ability to destroy tokens.
3.2.2 EtherollCrowdfund
A standard crowdfunding contract with the following features.
- Runs for a duration specified in the constructor by
_durationInMinutes
. Code comments suggest this will be set to two weeks time. - No cap on the overall funding. Code comments suggest funding goal is 10,000 ether.
- Disbursement of tokens to those that contribute to the crowdsale.
- Escape hatch feature which will abort the crowdsale and allow contributors to withdraw their contribution.
Section 4 - Main Findings
General findings of the audit
4.1 Minor Issues
No minor issues were found.
4.2 Medium Issues
4.2.1 The EtherollToken.priviledgedAddressBurnUnsoldCoins
function is likely not necessary.
The EtherollToken.priviledgedAddressBurnUnsoldCoins
could be removed. The
same functionality can be accomplished by transfering any remaining tokens to
the null address (0x0
).
This approach will cause the totalSupply
value to not be representative of
the actual circulating supply but this is also the case with other token
systems as inevitably some tokens will become unrecoverable through loss of
private keys.
4.3 Major Issues
4.3.1 Deviation from the ERC20 Standard
The token contract deviates from the ERC20 standard in the following places.
EtherollToken.transfer
does not return a boolean (and throws exceptions on transfer failures)EtherollToken.LogTransfer
should beTransfer
EtherollToken.LogApproval
should beApproval
There are already numerous services and applications built on top of the ERC20 standard. By not complying with the standard this contract will likely not function correctly within these applications. This will likely result in an inability to get the token listed on exchanges or to be used in any number of other applications which rely on this standard.
4.3.2 Freeze and Self Destruct functionality on the Token contract can be abused.
The EtherollCrowdfund
implements privileged methods for the following.
- Freezing all tokens
- Freezing tokens belonging to a specific account.
- Self destructing the contract.
The existence of these APIs gives significant centralized control over the token system and may adversely affect the desirability of holding this token.
Recommend removing these APIs entirely. Contracts such as this one that only house data can be replaced simply by deploying a new token contract which initializes it's data in the desired state. Even in situations where a defect is found that allows an attacker to compromise the token contract in significant ways such as minting new tokens or the ability to alter other account balances, the new contract can easily be initialized at the last good state prior to the attackers actions.
Given that removal of these APIs will remove most, if not all of the non-standard ERC functionality, the Etheroll development team should consider switching over to an existing ERC20 implementation that has been subjected to security audits.
4.4 Critical Issues
No critical issues were found.
Section 5 - Detailed Findings
Detailed findings of the audit
5.1 Minor Issues
No minor issues were found.
5.2 Medium Issues
5.2.1 EtherollToken.transfer
throws on failure conditions.
The current implementation throws errors on failure conditions. While this is
not a deviation from the ERC20 standard, it is a deviation from the established
convention which is to return false
in failure conditions.
Recommend modifying this function to not throw on failure conditions and
instead gracefully fail by returning false
.
5.2.2 EtherollToken
does not implement an ERC20 compliant Transfer
event
The EtherollToken
contract uses the event LogTransfer
which deviates from
the ERC20 standard. The ERC standard specifies that the event signature should
be as follows.
event Transfer(address indexed _from, address indexed _to, uint256 _value)
Recommend modifying the contract's event logging to comply with the standard.
5.2.3 EtherollToken
does not implement an ERC20 compliant Approval
event
The EtherollToken
contract uses the event LogApproval
which deviates from
the ERC20 standard. The ERC standard specifies that the event signature should
be as follows.
event Approval(address indexed _owner, address indexed _spender, uint256 _value)
Recommend modifying the contract's event logging to comply with the standard.
5.2.4 Logging of incorrect data within the EtherollCrowdfund.safeWithdraw
function.
On line 280 in the ./contracts/crowdsale-limited.sol
source file the
LogFundTransfer
event is used to record the transfer of funds to the
etherollBeneficiary
address. The code for this section is as follows.
if(!etherollBeneficiary.send(this.balance)) throw;
LogFundTransfer(etherollBeneficiary, this.balance, false);
The previous statement sends the full contract balance to the
etherollBeneficiary
address which will result in this.balance
always being
0
at the time the event is logged.
Recommend swapping the order of these two statements so that the event data will be correct.
5.2.5 EtherollCrowdfund.safeWithdraw
will withdraw user funds to the etherollBeneficiary
address.
The EtherollCrowdfund.safeWithdraw
function transfers the raised funds to the
etherollBeneficiary
and bankrollBeneficiary
accounts. The funds are
distributed across the two accounts with 80% going to the bankrollBeneficiary
account and the remaining 20% going to the etherollBeneficiary
account.
The token contract being used has a decimal value of 0
meaning that the
tokens are not divisible. The crowdfunding contract is designed to refund
any extra ether that is leftover after allocating tokens. In the event that
the sending of the refund fails, the extra funds are set aside to allow the
user to trigger the refund themselves with the
EtherollCrowdfund.withdrawChange
function.
The manner in which the raised funds are sent to the two beneficiary addresses will empty the crowdfunding contract of any change which has been set aside.
Recommend investigating whether this issue can be fixed without adding unwanted complexity to the contract. The amount of funds which is likely to be incorrectly allocated in this situation is likely to be small.
One possible approach would be to remove the change withdrawal mechanism entirely. This should probably be accompanied by clear instructions to users to send exact ether amounts and that additional change leftover after allocation of tokens will be forfeit.
5.3 Major Issues
5.3.1 EtherollToken.transfer
does not conform to the ERC20 standard.
The EtherollToken.transfer
function deviates from the ERC20 standard by not
returning a boolean. This will likely limit the usefulness of this token as it
will be incompatible with many applications which rely on this standard.
Recommend modifying this function to comply with the standard and return a boolean indicating the success or failure of the transfer.
Specific care should be taken to also adjust the logic within the
EtherollCrowdfund
contract which transfers the tokens to check the return
value of the transfer
function call. The current behavior relies on the
current behavior of the function throwing an error on failure conditions.
5.3.2 EtherollCrowdfund.ownerkill
allows owner
to bypass contract rules.
The EtherollCrowdfund.ownerkill
function will self destruct the contract and send all funds to
the owner
address. This function can be called at anytime by the owner
account.
This would allow the owner
address to collect all raised funds from the
crowdsale at any point during or after the crowdsale regardless of whether the
funding goal was reached.
It is not clear what the appropriate change, if any, is for this issue. It may
be appropriate change the beneficiary address for this function to a
multisignature account whose keys are held by multiple parties who can be
expected to act in the best interest of the crowdsale participants. This will
allow the escape hatch to remain in place while removing the ability for the
owner
address to wholly bypass the crowdsale contract's parameters.
5.3.3 EtherollToken.priviledgedAddressBurnUnsoldCoins
can be used to burn any tokens.
The EtherollToken.priviledgedAddressBurnUnsoldCoins
allows the
priviledgedAddress
to destroy the tokens belonging to whatever address is
provided in the function call. Given the name of this function, it does not
appear that the actual behavior of the function matches the intended behavior
of burning the coins which were not purchased during the crowdsale.
Recommend removal of the address
argument from the function signature and
modifying the function to burn the coins allocated to the priviledgedAddress
.
5.4 Critical Issues
No critical issues were found.
Section 6 - Follow Up Audit
After submitting the above report to the Etheroll development team a follow up
audit was performed on the updated codebase. The follow up audit was performed
on the codebase at commit hash 615ccd2c6a4661eec176c367bce8a1292526530d
.
The follow up audit involved a very brief review of the source as a whole followed by individual assessment of each issue covered in the initial audit.
6.1 Overview
After reviewing the updated source all issues have been satisfactorily addressed. The one unaddressed issue was left as-is intentionally and does not appear to pose any threat to the security of the contracts.
6.2 Source Code
The SHA256 of the three contact files is as follows.
$ shasum -a 256 contracts/crowdsale-limited.sol contracts/limited-token.sol
1d3a627f24d4122a7ad8ffdadf92071d7cfcbd247682017373a3d0d4370b7c74 contracts/crowdsale-limited.sol
shasum: contracts/limited-beneficiary.sol:
04254c533641b6c9730ab499906b73b7590cfeaf204a61c228ea90e44087056d contracts/limited-token.sol
6.3 Terminology
6.3.1 Status: Resolved
Issues that are noted with the resolved status have been adequately addressed.
6.3.2 Status: Unchanged
Issues that are noted with the unchanged status have not been addressed.
6.3.3 Status: Partially Resolved
Something was done in response to this issue but it may not fully address the issue.
6.4 Main Findings
6.4.1 Minor Issues
None
6.4.2 Medium Issues
6.4.2.1 The EtherollToken.priviledgedAddressBurnUnsoldCoins
function is likely not necessary.
- 4.2.1
- Status: Unchanged
Per information from the Etheroll development team there is future
functionality in dividend dispursement that relies on the accuracy of
totalSupply
.
6.4.3 Major Issues
6.4.3.1 Deviation from the ERC20 Standard
- 4.3.1
- Status: Resolved
The EtherollToken
contract is now compliant with the ERC20 standard.
6.4.3.2 Freeze and Self Destruct functionality on the Token contract can be abused.
- 4.3.2
- Status: Resolved
All APIs recommended for removal have been removed with the exception of the ability to freeze all token assets. The Etheroll development team has stated that this is required for dividend payouts.
The remaining API for freezing tokens has been adjusted in such a way that token freezes are not immediate but rather recurring scheduled events. This adjustment removes the ability for this API to be abused by the privileged addresses.
6.4.4 Critical Issues
None
6.5 Detailed Findings
6.5.1 Minor Issues
None
6.5.2 Medium Issues
6.5.2.1 EtherollToken.transfer
throws on failure conditions.
- 5.2.1
- Status: Resolved
This function now returns false
on failure conditions.
6.5.2.2 EtherollToken
does not implement an ERC20 compliant Transfer
event
- 5.2.2
- Status: Resolved
This event has been renamed to comply with the ERC20 standard.
6.5.2.3 EtherollToken
does not implement an ERC20 compliant Approval
event
- 5.2.3
- Status: Resolved
This event has been renamed to comply with the ERC20 standard.
6.5.2.4 Logging of incorrect data within the EtherollCrowdfund.safeWithdraw
function.
- 5.2.4
- Status: Resolved
The event now logs the correct amount by using a local cached value populated prior to transferring the funds.
6.5.2.5 EtherollCrowdfund.safeWithdraw
will withdraw user funds to the etherollBeneficiary
address.
- 5.2.5
- Status: Resolved
The token issuance has been reworked so that there are no longer any use funds held within the contract that would be withdrawn using this mechanism.
6.5.3 Major Issues
6.5.3.1 EtherollToken.transfer
does not conform to the ERC20 standard.
- 5.3.1
- Status: Resolved
The transfer function now returns a boolean in both success and failure conditions.
6.5.3.2 EtherollCrowdfund.ownerkill
allows owner
to bypass contract rules.
- 5.3.2
- Status: Resolved
The EtherollCrowdfund.ownerkill
function has been removed.
6.5.4 EtherollToken.priviledgedAddressBurnUnsoldCoins
can be used to burn any tokens.
- 5.3.3
- Status: Resolved
The EtherollToken.priviledgedAddressBurnUnsoldCoins
now only allows burning
of coins associated with the priviledgedAddress
address account.
6.5.5 Critical Issues
None