This review was conducted on 2017/10/17-2017/10/21 on initial code retrieved from the client on the day 2017/10/17 from official github repository https://github.com/FollowCoin/followcoin-contract of revision 5c1f025ae10edbe67934bbe14d25ad97209e1b2b.
The review process was done in stages where on each stage were identified vulnerabilities, flaws in logic and general improvements for the smart contract codes. Discovered obstacles were reported to the members of team of client with recommendations and code examples for available solutions. Then after smart contract codes fixes and updates, the review process takes next coil of testing.
The most of serious problems were found in the first round of smart contract code review. The complete list of all concerns:
1. Vulnerability in transferFrom
The implementation does not have a check of allowances for the caller of the method. This leads to possibilities to withdraw tokens from any users to other address by attacker. Even in case of lock of transfers (contributorsLockdown is active), the attacker still have possibilities to withdraw tokens from the account of owner (all tokens of initial supply) or from users of allowedAccounts.
2. Vulnerability by public method setSold
The declaration of setSold method has no onlyOwner modified. It allows for attacker to manipulate of the number of amount of sold token to stop or continue the ICO process.
The smart contract does not have all ERC20 API methods - it does not implement allowances logic and missing appropriate event.
The smart contracts do not use SafeMath.sol for operations on the type uint, uint256. Although, there are no obvious flaws in current math logic, it is mine opinion to have all math operations covered with SafeMath.sol until the EVM can handle bounds checks on its own.
5. The method burn gives possibilities to destroy user coins
Though the method has onlyOwner modifier, it gives possibilities to the contract owner to destory tokens from any account. My recommendation to have only one special account which shall be used for destorying tokens. It provides confidence to users about the ownership of their tokens.
There is logic to make any account frozen (user can not transfer his token to another address). My recommendation is to modify it in a way that it accepts additional parameter for address of arbitration party which will get approve() of all amount of frozen account. Or remove the logic about freezing (this recommendation is accepted).
Consideration about making setLockDown irreversible - so once transfers (trades) will be enabled, it will be impossible to lock down trades of all users again. So even in cases if wallet/keys are stolen, there will be no way to impact existing users - their ownership of tokens and possibilities of trading them.
There is a recommendation to keep the list of addresses of all holders of tokens. It gives possibilities to have simple access to the array of all holder addresses. This feature is needed for cases when another smart contract need to get the list of holders/balances and iterate over them. Other way to get list of all holders is to use etherscan.io to extract the list but it makes it hard to use by another contracts (through oraclise)
There is a recommendation to implement simple logic that allows to migrate to new more advanced smart contract in future. It can be done by declaration of migrationAgent API and methods to set new contract address and actulally migrate methdos which transfer token balances to new contract.
The updated smart contract code fully implement ERC20 API and logic. It makes sense to have explicit declaration in code to show ERC20 compliance.
The Transfer event is not emited in transfer(). The events are needed for another smart contracts watching for events and automated system.
- Solidity defaults all functions to public, but it is recommended to add an explicity
publickeyword to outward facing functions or evenexternalsince it's not used inside of the contract.
Updated contract 2017/10/21, ref d313c300f9ff18bef5c7d65bcf1d0ef9a5ce63e1
There are no vulnerabilities found. Most of my recommendations are implemented.