We’ve been asked by the Arcade City team to review and audit their new token code. We conducted the research and now publish our results.
The audited contracts are at their ac-token GitHub repo. The version used for this report is commit dd4207e1538f96eb3cbbf9e714eb38015bfe7c5a. The main contract file is ARCToken.sol.
Here’s our assessment and recommendations, in order of importance:
We have not found any severe security problems with the code.
Code quality of the ac-token project is low, which made auditing it hard. We recommend a refactor to improve code quality (more recommendations given next), and maybe doing a second security audit. Simpler code makes functionality more apparent and reduces attack surface. Given the high degree of test coverage, making changes to improve code quality will have a very low risk of introducing regressions.
Current code is written for old versions of solc. While this is not a security problem, it’s recommended to use latest version of Solidity for any new contract. Using old versions of Solidity also has some consequences that bring potential security problems. For example, all functions can have msg.value > 0, which can cause the balance of the contract to be non-zero (which doesn’t seem to be desired, based on the multisig forwarding account).
We recommend:
pragma solidity ^0.4.2; to the top of each contract file. (edit: fixed by ArcadeCity team)payable function modifier to the fallback function andthe buyRecipient function. These are the only functions that need to be able to handle incoming payments.call.value()Using call.value() is potentially dangerous, and was responsible for the TheDAO hack. We couldn’t find a reason to use multisig.call.value(msg.value) instead of the simplermultisig.send(msg.value). We recommend making this change. (edit: fixed by ArcadeCity team)
The code seems to intend a linear price function, but this is the actual behavior:
There is an initial period where 1 ether = 125 ARC, and then four steps of decreasing price. We recommend reviewing if the price curve meets the desired shape.
There are several magic constants in the contract code. Some examples are:
Use of magic constants reduces code readability and makes it harder to understand code intention. We recommend extracting magic constants into contract constants.
The uint public presaleEtherRaised variable seems to be unnecessary. Consider using multisig.balance in its place unless funds in the multisig address will be moved (and thus balance changed) before endBlock. Having unneeded extra variables and code increases risk and attack surface for contract’s invariants to be broken.
Duplicate code makes it harder to understand the code’s intention and thus, auditing the code correctly. It also increases the risk of introducing hidden bugs when modifying one of the copies of some code and not the others. We recommend the following to remove duplicate code:
price and testPrice functions repeat the same code. price could doreturn testPrice(block.number);. (edit: fixed by ArcadeCity team)msg.sender checks for authentication repeat the same patterns in many functions. Examples are this, this and this. We recommend extracting those to function modifiers.buy function seems redundant to the fallback function for ARCToken.sol. (edit: fixed by ArcadeCity team)Commented code just adds clutter for the reader and creates unncessary confusion. Remove the commented allocateBountyAndEcosystemTokens function. (edit: fixed by ArcadeCity team)
Some tests depend on each other. This makes testing specific functions in isolation more difficult. We recommend making each test independent by using a new token contract for each test case, instead of using the same token instance for every test.
The sendTokens function in TokenVesting.sol reuses the name vestAmount, which is also a contract public variable. Using the same name for two different things is confusing and can bring unintended behaviors.
owner can change the multisig address, and thus, has full control of where the contract funds go, even after crowdsale starts. (edit: fixed by ArcadeCity team)price function should not work outside of the [startBlock, endBlock] period. Line 190 in ARCToken.sol is not used, given that the price function is only called from buyRecipient, and buyRecipient only works in that interval. We recommend removing that line, and adding precondition checks to price.No severe security issues were found. Some changes were recommended to follow best practices and reduce potential attack surface. Above all, we recommend a refactor to improve code quality.
Update: Since publishing this post, ArcadeCity team has implemented most recommendations found in this report.
Note that as of the date of publishing, the above review reflects the current understanding of known security patterns as they relate to ARC token contract. We have not reviewed the related Arcade City project. The above should not be construed as investment advice or an offering of ARC. For general information about smart contract security, check out our thoughts here.