vkonst
commented
6 years ago For easier reference, the original post is broken down to individual issues.
5.1 inconsistent comments and documentation
The comments are not consistent, at line 29 of the UpgradableToken.sol you have written * parameter instead of @Param which is the correct version if you're using natspec documentation style.
The documentation of the functions in that file is inconsistant. The fallback function uses the triple forward dash type of comment /// when the others use the dash with asterisk type of comment /* . This adds confusion and distracts the users from understanding the code. Some functions like setBounty(), create(), returnWei(), adjustPhaseBasedOnTime(), setDefaultParamsIfNeeded(), computeOversoldAndAdjustPhase() and so on, are undocumented. It's bad practice to leave the functions without explaining how they are supposed to work and their goal.
5.2 unrestricted "setUpgradeMaster" method
The fact that you can change the upgradeMaster with setUpgradeMaster() anytime is worrying because you could change the address of the updrade master anytime without notices potentially damaging the contract.
5.3 Confusing name for "Withdrawal" event
In the Withdrawable the name of the event Withdrawal is almost equal to the name of the function withdrawal(). This could add confusion when executing the function so it's a good practice to name the events with a Log keyword in front just to avoid confusing naming. For instance LogWithdrawal and withdrawal() is cleaner.
5.4 Unsafe big number addition
In the same Withdrawable file, at line 14 you're adding 2 numbers with the plus (+) operator. This could lead to overflows since the balance of the users could grow to a huge number. And it's a good practice to simply add the SafeMath library and use it there to prevent undesirable problems.
5.5 Unsafe big number multiplication
In the CtdToken file, at line 22 you're multiplying 650 million by the exponent of 10 to 18. Because this is such a huge number, 650e24, is recommended to use the method .mul() from SafeMath to avoid overflows in case you decide to change the limit of tokens. Same at line 24 with the pre ico limit of 130 million. However I'd simply recommend to write the resulting number from that multiplication with this format: 650e24 which is cleaner and easier to understand. ... At lines 58, 59 and 60 I recommend to use the .mul() function for the multiplications.
5.6 Unlimited maximum participation
There aren't checks to limit the maximum amount of ether to participate because in the function computeOversoldAndAdjustPhase() you simply return the oversold tokens without limiting the participation per user. This could lead to whales buying 50% or more of the total amount of tokens available in the smart contract. Something similar happened with Decentraland where only 5 big investors bought all the tokens leaving the thousands of smaller investors our of the company. Which is undesirable to the community and could cause a very negative response.
I recommend to set a maximum contribution amount to 1000 ETH in total per user to avoid this problem allowing token distribution accross the thousands of interested investors. You probably don't want to suffer the same attack as the Decentraland ICO from the users because of this problem.
The comments are not consistent, at line 29 of the UpgradableToken.sol you have written * parameter instead of @param which is the correct version if you're using natspec documentation style.
The fact that you can change the upgradeMaster with setUpgradeMaster() anytime is worrying because you could change the address of the updrade master anytime without notices potentially damaging the contract.
In the Withdrawable the name of the event Withdrawal is almost equal to the name of the function withdrawal(). This could add confusion when executing the function so it's a good practice to name the events with a Log keyword in front just to avoid confusing naming. For instance LogWithdrawal and withdrawal() is cleaner.
In the same Withdrawable file, at line 14 you're adding 2 numbers with the plus (+) operator. This could lead to overflows since the balance of the users could grow to a huge number. And it's a good practice to simply add the SafeMath library and use it there to prevent undesirable problems.
In the CtdToken file, at line 22 you're multiplying 650 million by the exponent of 10 to 18. Because this is such a huge number, 650e24, is recommended to use the method .mul() from SafeMath to avoid overflows in case you decide to change the limit of tokens. Same at line 24 with the pre ico limit of 130 million. However I'd simply recommend to write the resulting number from that multiplication with this format: 650e24 which is cleaner and easier to understand.
At lines 58, 59 and 60 I recommend to use the .mul() function for the multiplications.
The documentation of the functions in that file is inconsistant. The fallback function uses the triple forward dash type of comment /// when the others use the dash with asterisk type of comment /* . This adds confusion and distracts the users from understanding the code. Some functions like setBounty(), create(), returnWei(), adjustPhaseBasedOnTime(), setDefaultParamsIfNeeded(), computeOversoldAndAdjustPhase() and so on, are undocumented. It's bad practice to leave the functions without explaining how they are supposed to work and their goal.
There aren't checks to limit the maximum amount of ether to participate because in the function computeOversoldAndAdjustPhase() you simply return the oversold tokens without limiting the participation per user. This could lead to whales buying 50% or more of the total amount of tokens available in the smart contract. Something similar happened with Decentraland where only 5 big investors bought all the tokens leaving the thousands of smaller investors our of the company. Which is undesirable to the community and could cause a very negative response.
I recommend to set a maximum contribution amount to 1000 ETH in total per user to avoid this problem allowing token distribution accross the thousands of interested investors. You probably don't want to suffer the same attack as the Decentraland ICO from the users because of this problem.