In the dates of June 20th 2017 to June 21st 2017, OpenANX engaged Yaron Velner and Loi Luu
to perform security audit for their new OpenANX token contracts.
The audited contracts can be found here.
The audited code was timestamped with the hash 0d8cb22a3008100c112f46279fcf1d0bc81e9747 in the said repository.
- Table of Contents
- Terminology
- Findings
- 1. Severe security flaws
- 2. Architecture and design choices
- 3. Use of best practices
- 4. Comments and Suggestions
- 4.1 Mint token logic is duplicated
- 4.2 Increase in locked supply is not logged with an event
- 4.3 Use of send instead of
transfer - 4.4 transferAnyERC20Token input type could be
ERC20Interfaceinstead ofaddress - 4.5 Consider checking kyc in
approve - 4.6 burnFrom should only be called when
finalisedistrue - 4.7 addPrecommitment could exceed the hard cap
- 4.8 Lack of SafeMath in
uint tokens = msg.value * tokensPerKEther / 10**uint(18 - decimals 3);
- 5. Additional Information and Notes
- 6. Tests
- 7. Bug bounty
- 8. Conclusion
This audit uses the following terminology. Note that we only rank the likelihood, impact and
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 was clearly written and well modularized. However, it looks that the code was written from scratch rather than using publicly available and well-tested code bases such as Open-Zeppelin. In particular, relying on Open-Zeppelin existing Vested Token could have saved a lot of implementation efforts and would potentially help avoid bugs.
The contracts employ currently best practices like SafeMath and modifiers, except in the two following cases.
- Likelihood: low
- Impact: high
The team can consider mitigate the so called short address attack by verifying
payload size in the transfer, transferFrom, and approve functions (possibly also in burnFrom).
- Likelihood: low
- Impact: low
Current implementation does not mitigate the known attack in the approve and transferFrom functions.
Below are some notes, warnings that can be taken into consideration to improve the contracts.
Consider adding a mint-token function.
At present implementation, tokens are minted in 3 places, namely proxyPayment, finalise and addPrecommitment. Lack of minting function generates an inconsistent behavior in finalise where minting event is not logged.
When remaining tokens are locked in finalise, the amount is not logged, making it hard to trace on the blockchain.
Code like if (!wallet.send(msg.value)) throw; can be replaced by wallet.transfer(msg.value).
kyc is checked in transferFrom, however it could help the user if lack of kyc is detected even earlier in the approve function.
Consider adding require(finalised) in the burnFrom() function to avoid accidental behaviors, given that anyone can call burnFrom. If require(fianlised) is already
added in the approve function then its not needed here.
There is no validation in addPrecommitment that hard cap is not exceeded.
The severity is low as only OpenANX can call this function.
Nevertheless, a validation could prevent human errors.
Consider using safe math in line 249 in OpenANXToken.sol.
Users should be aware that they could transfer the token they hold only after completing a kyc process with OpenANX. OpenANX should take into consideration that KYC process is required also for smart contracts and DAOs that hold the contract.
Users should be aware that in the current ICO at most 30% of the tokens are for sale. 40% of the tokens are locked for a period of 1-2 years and will be given to OpenANX team at the end of the lock time. The reminder is frozen for a 1 year period after which it will be in the possession of OpenANX which they could use for a second ICO.
OpenANX should be aware that limiting contribution size to CONTRIBUTIONS_MAX does not prevent users from doing larger contribution in a single transaction.
A smart contract can do several contributions in a single transaction.
This is reasonable and guarantees that funds are never locked in the contract.
OpenANX could pre-mine (in return to any kind of contribution) tokens until the very last second before the ICO.
This gives very little time for contributors to evaluate the actual pre-sale contribution before they have to decide on their contribution.
A possible mitigation is to change the implementation of addPrecommitment so it will stop issuing tokens at START_DATE-N hours (or any other reasonable time frame).
Alternatively, OpenANX could publicly declare they will not call addPrecommitment at least N hours prior to ICO.
Such deceleration could be quickly verified by potential contributers before they contribute.
OpenANX are not using the standard truffle framework for their testing.
Hence, it is not easy for us to appreciate the coverage of the tests.
The tests do seem to cover many scenarios, however we suspect that in some cases the output is only printed to screen (by calling printBalances) rather than verified.
OpenANX declared a bug bounty program on June 19th. Only 4 days before the planned ICO. Best practice is to have at least a one week bug bounty.
No severe security issues were found.