Moeda Token Audit

The Moeda team asked us to review and audit their new Moeda Token code. We looked at their contracts and now publish our results.

The audited contracts can be found in their moeda repo. The version used for this report is commit b2bf23119d563e251b6f16b29b642bac43e76a64. The main contracts are MoedaToken.sol and Crowdsale.sol.

Overall the code is good and has only minor issues. Here’s our assessment and recommendations, in order of importance.

Update: The Moeda team implemented most of our recommendations in the latest version of their code.

Severe

We haven’t found any severe security problems with the code.

Potential problems

Unnecessary complexity calculating token amount

The formula used to calculate the token amount in line 116 is unnecessarily complicated. It could be replaced by a multiplication by the amount of tokens bought with one wei for each given tier. Consider making this change by replacing the token creation rates by 1 ether divided by the current values. This has a number of benefits:

  1. Numbers are exact and there is no precision lost (current version has loss of precision due to chained integer operations).
  2. There is less risk of overflow.
  3. It’s simpler, which is always preferable. When code is irreducibly complex, consider using temporary variables to improve readability.

Update: Fixed in https://github.com/erkmos/moeda/commit/a894b2cccef0236e90326b84a0b3dde485e78cd1

Conditions for finalization

The crowdsale can be finalized at any moment, in particular before endBlock is reached. Please double check that this is the desired behavior.

Update: Now requiring that either cap or end block has been reached. Fixed in https://github.com/erkmos/moeda/commit/5eb52c0648d1af0db11f5eb883379d6c8054e2c0 and refactored in https://github.com/erkmos/moeda/commit/9edf6fe6e23aec7f6fcba6d0921436e435336048

Use of Transfer event to log token creation

The crowdsale contract uses the Transfer event to log the creation of tokens, with a from field set to the zero address. This is overloading the semantics of the Transfer event of the ERC20 standard, which might not be a good idea as other software (such as wallets or exchanges) could be relying on these semantics. This is an ongoing discussion in the ERC20 standardization process, but our stance on it is that a different event should be used.

Update: Created a new event, fixed in https://github.com/erkmos/moeda/commit/68ff6544976c2bfa655cae28e69ab09d813dbc46

Warnings

Use latest version of Solidity

The contracts are written for an old version of the Solidity compiler (0.4.8). We recommend changing the solidity version pragma to the latest version (pragma solidity ^0.4.11;) to enforce the use of an up to date compiler. (Please note that there has been a recent update to the Truffle framework to make it compatible with 0.4.11.)

Update: fixed in https://github.com/erkmos/moeda/commit/befe14a5769a7f5c3dcd964c1d042408ba4356e4

Unexpected throw

When the ether received adds up to exactly TIER3_CAP, getLimitAndRate throws when the comment says it shouldn’t. Consider changing all the comparisons in this function to inclusive ones (e.g. totalReceived <= TIER3_CAP), which is the standard behavior in these cases.

Redundant return value

processBuy function in line 126 is declared to return a boolean but never returns false. Instead, it always throws in case of error. Consider sticking to one error signaling convention. We recommend throwing to fail as early and loudly as possible, ensuring the propagation of the error. Same thing with unlock function in line 40 of MoedaToken.sol.

Enforce all preconditions

processBuy function in line 126 has a precondition written in the comments which is not enforced in the code: it’s only usable while the crowdsale is active. In the current state of the contract, the function is internal and only called from the fallback function, which is properly guarded with the onlyDuringSale modifier. However, it’s best to enforce preconditions independently on a function-by-function basis.

Consider using the onlyDuringSale modifier at the beginning of processBuy. Furthermore, since processBuy is only used once, and is the only thing the fallback function does, consider removing it altogether, moving the code inside the fallback function.

Update: The function has merged with the fallback function, was fixed in https://github.com/erkmos/moeda/commit/d4c8f85809ecd33852924aa71b0a233f4dbc2318

Naming suggestions

Use SafeMath in all math operations

Most math operations are safe, except a sum in the token contract at line 51. It’s always better to be safe and perform checked operations.
Update: The place it was missing has been fixed in https://github.com/erkmos/moeda/commit/5e1ccfb7b4f934c0588c84196e435e21f46c7de5

Additional Information and Notes

Conclusions

No severe security issues were found. Some additional changes were proposed to follow best practices and reduce potential attack surface.

Update: The Moeda team implemented most of our recommendations in the latest version of their code.

Overall the code is good and has only minor issues.

Thanks to Francisco Giordano for helping write 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 the Moeda Token contract. We have not reviewed the related Moeda project. The above should not be construed as investment advice or an offering of tokens. For general information about smart contract security, check out our thoughts here.