- Introduction
- Terminology
- Findings
- 1. Severe security flaws
- 2. Architecture and design choices
- 3. Use of best practices
- 4. Medium level bugs
- 5. Suggestions
- 6. Low severity issue and comments
- 6.1 In CDTToken.sol, the variable
creatorserves the same purpose asownervariable (which is defined inVestedToken). - 6.2 Consider having addresses like 0xfd6259c709Be5Ea1a2A6eC9e89FEbfAd4c095778 as constants or c'tor params.
- 6.3 Use of weeks key word
- 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
- 6.1 In CDTToken.sol, the variable
- 7. Additional Information and Notes
- 8 Tests
- 9. Bug bounty
- 10. Conclusion
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.
Update: On July 10th CoinDash asked us to review a modified version of their code timestamped with 88788009130eea6e404e23613081c4abe26ae3b5.
This audit uses the following terminology. Note that we only rank the likelihood, impact and Severity for bug/security-related issues.
How likely a bug is to be encountered or exploited when deployed in practice, as specified by the OWASP risk rating methodology.
The impact a bug would have if exploited, as specified by the OWASP risk rating methodology.
How serious the issue is, derived from Likelihood and Impact as specified by the OWASP risk rating methodology.
Below is our audit results and recommendations, listed in order of importance.
We did not find any severe security problems with the code.
Overall, the code is 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 simplified the code and mitigated the risk for potential bugs.
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:
- 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.
Update: compiler version was changed to 0.4.8. Coindash team should be aware that when compiling with 0.4.11 version, additional warnings occur.
In particular, the initialization of constant variables with non-constant function is deprecated in current solidity compiler versions.
- 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.
- 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, fetch it to wrong directory, and 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 critical changes were done.
- 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.
Update: totalSupply is properly initialized in the new code.
- Likelihood: low
- Impact: medium
The processPurchase function checks if cdtSold >= ALLOC_CROWDSALE before allocating tokens to the contributors, and does not 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 buggy checking.
Update: problem was fixed in the new code.
Token name usually does not contain "token" (see here).
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.
Update: CoinDash has implemented a function that can drain only Ether funds. However, it does not solve the case where tokens are stuck inside the contract. Moreover, the function is not needed, as Ether cannot be deposited into the token contract (all functions are non-payable).
Finally, the function makes use of a new modifier, namely, only_owner, instead of using Zeppelin onlyOwner existing modifier.
Nevertheless, this is a minor issue with very low chances of producing an actual bug. Hence, it is likely OK to leave this code.
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.
In allocateTokensWithVestingToTeam you could use 1 years instead of 52 weeks and 1 years / 2 instead of 26 weeks.
Update: variable name was changed to weiReceived.
Update: event was removed from code.
It is extremely important to use safe math here in light of a possible bug in processPurchase.
Update: code was fixed to use safe math.
Contributers should be aware that tokens are transferable from the moment they are purchased. Even before ICO ends.
Contributers should be aware that all contributed Ether are immediately available for CoinDash for any purpose.
Token Distribution
+----------------------+-----------------------------+
| Holder | Token |
+ +-----------------------------+
| | Quantity | Percentage (%) |
+----------------------+------------+----------------+
| Team | Liquid Team | 100000000 | 10 |
+ +---------------+------------+----------------+
| | Illiquid Team | 100000000 | 10 |
+------+---------------+------------+----------------+
| Company | 290000000 | 29 |
+----------------------+------------+----------------+
| Bounties | 10000000 | 1 |
+----------------------+------------+----------------+
| Crowdsale | 500000000 | 50 |
+----------------------+------------+----------------+
| Total | 1000000000 | |
+----------------------+------------+----------------+
Vested Token Distribution
+-------------+--------------------------------------------+--------------------------------------------+----------+--------------+
| | Address | Granted token | Cliff | Vesting time |
+ + +--------------------------------------------+----------+--------------+
| | | Quantity | Percentage to total supply (%) | | |
+-------------+--------------------------------------------+-----------+--------------------------------+----------+--------------+
| Liquid Team | 0xfd6259c709Be5Ea1a2A6eC9e89FEbfAd4c095778 | 20000000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-----------+--------------------------------+----------+--------------+
| | 0xC09544dA6F50441c024ec150eCEDc72De558ce94 | 20000000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-----------+--------------------------------+----------+--------------+
| | 0xa900191B0542e27A0022a05c45c152DFa98DB026 | 20000000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-----------+--------------------------------+----------+--------------+
| | 0x05b481E52e1Ca0A21C147016C4df729764615Afb | 20000000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-----------+--------------------------------+----------+--------------+
| | 0xc6bFce8cEad4EcC595bA227b9527AFA914dD8183 | 20000000 | 2 | 6 months | 1 year |
+-------------+--------------------------------------------+-----------+--------------------------------+----------+--------------+
| Company | Initialized when contract is deployed | 290000000 | 29 | 6 months | 1 year |
+-------------+--------------------------------------------+-----------+--------------------------------+----------+--------------+
| Total | 390000000 | 39 | | |
+----------------------------------------------------------+-----------+--------------------------------+----------+--------------+
Update: CoinDash team has made following token distribution and vesting token distribution updates:
- Allocate 1.25% of total token supply to WINGS DAO participants.
- Change all addresses of the team.
- Company's token is set to 1 year cliff and no vesting time after the cliff.
The current distribution is presented in the tables below. We note that in the time of writing (10/7/2017), the tables are consistent with CoinDash statement as it appear in their website.
Token Distribution
+----------------+----------------------------+
| Holder | Token |
+ +----------------------------+
| | Quantity | Percentage |
+----------------+---------------+------------+
| Liquid contrib | 100,000,000 | 10% |
+----------------+---------------+------------+
| Illiquid team | 100,000,000 | 10% |
+----------------+---------------+------------+
| Bounty program | 10,000,000 | 1% |
+----------------+---------------+------------+
| Wings | 12,500,000 | 1.25% |
+----------------+---------------+------------+
| Company | 290,000,000 | 29% |
+----------------+---------------+------------+
| Crowdsale | 487,500,000 | 48.75% |
+----------------+---------------+------------+
| Total | 1,000,000,000 | 100% |
+----------------+---------------+------------+
Vested Token Distribution
+---------+--------------------------------------------+----------------------------------------------+----------+-----------------+
| | Address | Granted token | Cliff | Vesting time |
+ + +----------------------------------------------+----------+-----------------+
| | | Quantity | Percentage to total supply (%) | | |
+---------+--------------------------------------------+-------------+--------------------------------+----------+-----------------+
| Team | 0x9c160d7450400b59AA3e7D1a8cc4Bf664859aB4B | 20,000,000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-------------+--------------------------------+----------+-----------------+
| | 0x97251AA8f0a71b10E90077AebabEd0c1e2626455 | 20,000,000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-------------+--------------------------------+----------+-----------------+
| | 0xBA361d8b9A6D7CE1603Cf526604ce5431ecc0E76 | 20,000,000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-------------+--------------------------------+----------+-----------------+
| | 0x0C60180e5F1dEf7Daa947F88bF840dCeF8A27f53 | 20,000,000 | 2 | 6 months | 1 year |
+ +--------------------------------------------+-------------+--------------------------------+----------+-----------------+
| | 0x3f0C1028d5F55CaA11208173D8AE09d42c3ff5B0 | 20,000,000 | 2 | 6 months | 1 year |
+---------+--------------------------------------------+-------------+--------------------------------+----------+-----------------+
| Company | Initialized when contract is deployed | 290,000,000 | 29 | 1 year | no vesting time |
+---------+--------------------------------------------+-------------+--------------------------------+----------+-----------------+
40,000 Ether
Update: hard cap is expected to change according to ETH price at the time of ICO.
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.
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).
Current test regression requires CoinDash team to manually run the tests one by one (otherwise, some of them fail). We recommend them to fully automate the process in order to mitigate potential human errors.
The following cases are not tested:
- call to
emptyContribuitionPoolbefore end of crowd-sale period, and a call that is done not by the owner. - A call to
toggleHaltnot by the owner. - Purchasing of tokens after
toggleHaltre-enables buying. - Transferring tokens after vesting period is over. Arguably, this is covered by Open-Zeppelin tests. However, we recommend to test it explicitly.
- Value of
totalSupply. - Trying to exceed maximum token cap.
Update: Tests for items 1,2,3 and 4 were added.
- Instead of
assert.equal(1,0,err);, you could useassert(false, err)orassert.fail(). - Testing
transferfor negative amounts is meaningless, asuintcannot get negative values.-50is actually2^256 - 50.
The time of writing, CoinDash did not launch a bug bounty program.
Update: CoinDash is planning to make their repository public and announced a bug bounty program.
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).
Update: CoinDash team has addressed all the issues we raised for Section 4. Issues from Section 3 were only partially addressed, however the remaining issues are of low likelihood, and we leave the decision of whether they should be addressed to the team. In addition, CoinDash launched a bug bounty program.