draft.md

In the dates of June 20th 2017 to June 30th 2017, CoinDash engaged Yaron Velner and Victor Tran from the SmartPool team to perform security audit for their ICO contracts. The audited contracts currently resides in CoinDash private repository. The audited code was timestamped with the hash 0b8fc008894dddc60d1233b84d877b5a76473d32 in the CoinDash repository.

Terminology

This audit uses the following terminology. Note that we only rank the likelihood, impact and $Severity$ for bug/security-related issues.

1. Likelihood

How likely a bug is to be encountered or exploited when deployed in practice, as specified by the OWASP risk rating methodology.

2. Impact

The impact a bug would have if exploited, as specified by the OWASP risk rating methodology.

3. Severity

How serious the issue is, derived from Likelihood and Impact as specified by the OWASP risk rating methodology.

Findings

Below is our audit results and recommendations, listed in order of importance.

1. Severe security flaws

We did not find any severe security problems with the code.

2. Architecture and design choices

Overall, the code was clearly written and well modularized. CoinDash team wisely decided to make full use of Open-Zeppelin existing Vested Token module as the core functionality of their token. This decision significantly simplifies the code and mitigate the risk for potential bugs.

3. Use of best practices

The contracts employ currently best practices like SafeMath and modifiers. The use of Open-Zeppeling code mitigate recent attacks as the short address attack and approve double spending attack.

However we are concerned about the next three issues:

3.1 Use of old compiler version

• Likelihood: low
• Impact: medium

Currently the CoinDash team is using compiler version 0.4.6. We encourage them to use recent 0.4.11 compiler version.

3.2 Test regression is not fully automated

• Likelihood: low
• Impact: low

Current test regression requires CoinDash team to manually run the tests one by one (otherwise, some of them fail). We recommend them to fully automated the process in order to mitigate potential human errors.

3.3 Open-Zeppelin code was not imported to the repository

• Likelihood: low
• Impact: low

Currently, CoinDash team has to (manually) fetch Open-Zeppelin latest code to local disk before deployment. This could give rise to potential human errors that will fetch the wrong code, or alternatively unexpected changes in Open-Zeppelin code could introduce integration bugs. We recommend CoinDash team to fix an Open-Zeppelin commit hash for their use and put it in their repository. Prior to the deployment of the contract they should manually query Open-Zeppelin github to see if significant changes were done.

4. Medium level bugs

4.1 totalSupply is always 0

• Likelihood: high
• Impact: medium

totalSupply is not set in CDTToken constructor. And will remain 0 forever. We also recommend to log the creation of the new tokens as a Transfer event from 0x0 to owner.

4.2 Incorrect check of allocated token cap

• Likelihood: low
• Impact: medium

The processPurchase function checks if cdtSold >= ALLOC_CROWDSALE before allocating tokens to the contributors, and doesn't take into account the current purchase amount. This check should be fixed to cdtSold + o_amount >= ALLOC_CROWDSALE.

However, the likelihood of this bug is low as allocating more tokens than ALLOC_CROWDSALE is expected to make transfer function to throw and revoke the contribution. In other words, it is likely impossible to allocate more tokens even in the presence of this error checking.

5. Suggestions

5.1 Rename token name "CoinDash Token" to "CoinDash"

Token name usually does not contain "token" (see here).

5.2 Allow token contract to drain its received funds

History suggests that users mistakenly transfer tokens or Ether to token contracts. Consider implementing a drain function like this. We note that this is not a standard at the moment and last minute implementation could introduce new bugs. Hence, we leave it to the CoinDash team to decide if they want to do it or not.

6. Low severity issue and comments

6.1 In CDTToken.sol, the variable creator serves the same purpose as owner variable (which is defined in VestedToken).

Consider removing it.

6.2 Consider having addresses like 0xfd6259c709Be5Ea1a2A6eC9e89FEbfAd4c095778 as constants or c'tor params.

6.3 Use of weeks key word

In allocateTokensWithVestingToTeam you could use 1 years instead of 52 weeks and 1 years / 2 instead of 26 weeks.

6.4 ethReceived variable actually holds the amount of received wei (and not Ether)

6.5 event PreBuy is defined but never used

6.6 emptyContribuitionPool does not use safe math

It is extremely important to use safe math here in light of a possible bug in processPurchase.

7. Additional Information and Notes

7.1 Tokens are transferable also before ICO ends

Contributers should be aware that tokens are transferable from the moment they are purchased.

7.2 CoinDash immediately controls the received Ether

Contributers should be aware that all contributed Ether are immediately available for CoinDash for any purpose.

7.3 Token distribution

TODO - victor

8 Tests

CoinDash team created tests to check the different behavior among stages in the crowd-sale, and to verify that tokens are correctly allocated to the team and company. Given the relative simplicity of the code, we find testing level adequate in general. However, we do have few concerns.

8.1 Lack of testing for Open-Zeppelin code

We recommend CoinDash team to include Open-Zeppelin tests in their regression. We recall that at least in one incident failing to test Open-Zeppelin supplied code entailed a post ICO redeployment of the token contract (as the developers failed to noticed they changed Open-Zeppelin code).

8.2 Regression is not fully automated

Current test regression requires CoinDash team to manually run the tests one by one (otherwise, some of them fail). We recommend them to fully automated the process in order to mitigate potential human errors.

8.3 Missing coverage

The following cases are not tested:

1. call to emptyContribuitionPool before end of crowd-sale period, and a call that is done not by the owner.
2. A call to toggleHalt not by the owner.
3. Purchasing of tokens after toggleHalt re-enables buying.
4. Transferring tokens after vesting period is over. Arguably, this is can be covered by Open-Zeppelin tests. However, we recommend to test it explicitly.
5. Value of totalSupply.
6. Trying to exceed maximum token cap.

8.4 Minor comments

1. Instead of assert.equal(1,0,err);, you could use assert(false, err) or assert.fail().
2. Testing transfer for negative amounts is meaningless, as uint cannot get negative values. -50 is actually 2^256 - 50.

9. Bug bounty

The time of writing, CoinDash did not launch a bug bounty program.

10. Conclusion

No severe security issues were found. Prior to the launch of the ICO, CoinDash team should address the issues we raised at Sections 3 and 4, and launch a bug bounty program (for a period of at least one week).