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.
Here’s our assessment and recommendations, in order of importance:
We have not found any severe security problems with the code.
General code quality
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.
Use latest version of Solidity
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).
pragma solidity ^0.4.2;to the top of each contract file. (edit: fixed by ArcadeCity team)
payablefunction modifier to the fallback function andthe
buyRecipientfunction. These are the only functions that need to be able to handle incoming payments.
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 simpler
multisig.send(msg.value). We recommend making this change. (edit: fixed by ArcadeCity team)
Price function is not linear
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.
Usage of magic constants
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.
Remove unnecessary code
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.
Remove duplicate code
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:
- Extract StandardToken, SafeMath and Token code into separate files, and import them from main contract files, instead of having exact copies of each in every file. We recommend using OpenZeppelin’s implementation of StandardToken.
testPricefunctions repeat the same code.
return testPrice(block.number);. (edit: fixed by ArcadeCity team)
msg.senderchecks for authentication repeat the same patterns in many functions. Examples are this, this and this. We recommend extracting those to function modifiers.
buyfunction seems redundant to the fallback function for ARCToken.sol. (edit: fixed by ArcadeCity team)
- The lines calculating allocations have the same code duplicated to arrive at the allocation amount. We recommend calculating the amount first and then updating balances and totalSupply.
Remove commented code
Commented code just adds clutter for the reader and creates unncessary confusion. Remove the commented
allocateBountyAndEcosystemTokens function. (edit: fixed by ArcadeCity team)
Tests should be independent
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.
Avoid name reuse
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.
Additional Information and notes
- This comment is wrong. It should say “it should only continue if endBlock has passed OR presaleEtherRaised has reached the cap”
ownercan change the
multisigaddress, and thus, has full control of where the contract funds go, even after crowdsale starts. (edit: fixed by ArcadeCity team)
pricefunction should not work outside of the [startBlock, endBlock] period. Line 190 in ARCToken.sol is not used, given that the
pricefunction is only called from
buyRecipientonly works in that interval. We recommend removing that line, and adding precondition checks to
- Using block heights is preferred to using timestamps for time-related logic. OK
- Some code indentation is inconsistent. For example, see this and this.
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.