Skip to content

Instantly share code, notes, and snippets.

@yuriy77k
Forked from RideSolo/ETH_AMO_report.md
Created May 3, 2019 16:33
Show Gist options
  • Save yuriy77k/2fc552fd10cb2f77139068f29cfdedb5 to your computer and use it in GitHub Desktop.
Save yuriy77k/2fc552fd10cb2f77139068f29cfdedb5 to your computer and use it in GitHub Desktop.

AMO Project Audit Report.

1. Summary

This document is a security audit report performed by RideSolo, where AMO Project has been reviewed.

2. In scope

  • AMOCoin.sol github commit hash 51b441202ba09186d2aec530a88f9e898205027c.
  • AMOCoinSale.sol github commit hash 390f49f16827d272ce19ddd8a90757c45a974ff0.

3. Findings

5 issues were reported:

  • 1 high severity issue.
  • 1 medium severity issue.
  • 3 low severity issue.

3.1. Multiple Token Transfers

Severity: High

Description

When allocateTokens is called the tokenAmount to be transfered to to address is not substracted from allocationList[to].allowedAmount, meaning that if the function is called again by the owner a higher amount than allocationList[to].allowedAmount can be transfered to the to.

Code snippet

https://github.com/RideSolo/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L463#L476

https://github.com/RideSolo/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L400#L407

3.2. Multiple Token Allocation

Severity: medium

Description

If Multiple allocations are made to user address using addToAllocationList function member of AMOCoinSale, the amount allocated is not cumulated in allocationList[user].allowedAmount. Please note that addToAllocationList is meant to be used when EarlyInvestment is set, meaning that real payment are done offchain and this can lead to major errors from the team since this function is only used by the owner.

Code snippet

https://github.com/RideSolo/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L400#L407

3.3. Owner Privileges

Severity: Owner privilege

Description

AMO token owner privileges:

  • Enable/Disable transfers/burn/approval for all tokens holders (onlyWhenTransferAllowed,enableTransfer,disableTransfer) (check here).
  • Selectively freeze/unfreeze tokens by addresses (onlyAllowedAmount,lockAccount,unlockAccount) (check here).

AMO sale owner privileges:

  • Token destinated to be sold through the crowdsale contract are approved and not transfered and locked in the sale contract, meaning that the owner will still be able to handle them without backing them up by ether (check here).
  • Every round (EarlyInvestment, PreSale,CrowdSale) has three stages (SetUp,Started,Ended), the sale contract logic do not allow to go to a previous stage of a round but a round can be selected once the the previous round has ended using setUpSale, meaning that the owner can reset the round even to a previous one. owner should not be able to reselect round in setUpSale but round has to be incremented directly inside the function, (check here).
  • A sale round can be ended by the owner at any moment, (check here.

3.4. Allocation Mechanism

Severity: Owner privilege

Description

addToAllocationList function member of AMOCoinSale add tokens amount to mapping for a dedicated users since it is allowed only when EarlyInvestment round then we can guess that the token sale was done offchain through fiat or any other way.

However removeFromAllocationList can also cancel the allowed tokens before that allocateTokens is called and the tokens are transfered to the user address. The main issue here is when is the fiat or crypto payment is done and why an address can be removed knowing that a user has maybe didn't get his payment back.

Code snippet

https://github.com/RideSolo/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L400

https://github.com/RideSolo/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L432

https://github.com/RideSolo/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L463

3.5. Known vulnerabilities of ERC-20 token

Severity: low

Description

  1. It is possible to double withdrawal attack. More details here
  2. Lack of transaction handling mechanism issue. WARNING! This is a very common issue and it already caused millions of dollars losses for lots of token users! More details here

Conclusion

The audited contracts issues should be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment