Instantly share code, notes, and snippets.

@pipermerriam /final-report-with-follow-up.md Secret
Created Jan 16, 2017

Embed
What would you like to do?
Download ZIP
Raw
final-report-with-follow-up.md

Section 1 - Table of Contents

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.

Piper Merriam on 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 be Transfer
  • EtherollToken.LogApproval should be Approval

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.

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

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.

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.

This function now returns false on failure conditions.

6.5.2.2 EtherollToken does not implement an ERC20 compliant Transfer event

This event has been renamed to comply with the ERC20 standard.

6.5.2.3 EtherollToken does not implement an ERC20 compliant Approval event

This event has been renamed to comply with the ERC20 standard.

6.5.2.4 Logging of incorrect data within the EtherollCrowdfund.safeWithdraw function.

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.

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.

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.

The EtherollCrowdfund.ownerkill function has been removed.

6.5.4 EtherollToken.priviledgedAddressBurnUnsoldCoins can be used to burn any tokens.

The EtherollToken.priviledgedAddressBurnUnsoldCoins now only allows burning of coins associated with the priviledgedAddress address account.

6.5.5 Critical Issues

None

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