Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AMO Project #162

Closed
MillianoConti opened this issue Feb 26, 2019 · 11 comments
Closed

AMO Project #162

MillianoConti opened this issue Feb 26, 2019 · 11 comments
Labels
approved Auditors can begin to audit this smart contract. Low priority (DEPRECATED) DEPRECATED: Use priority -1 or priority -2 to decrease the priority of a contract audit. solidity Smart-contract is written in solidity (Ethereum) language.

Comments

@MillianoConti
Copy link

MillianoConti commented Feb 26, 2019

Audit request

AMO a security solution for connected cars, autonomous vehicles, and smart cars.

Source code

https://github.com/AMO-Project/AMO-Contracts/tree/master/contracts

Disclosure policy

support@amo.foundation

Platform

ETH

Number of lines:

399

@Dexaran Dexaran added solidity Smart-contract is written in solidity (Ethereum) language. approved Auditors can begin to audit this smart contract. labels Feb 27, 2019
@yuriy77k yuriy77k removed the approved Auditors can begin to audit this smart contract. label Feb 27, 2019
@yuriy77k
Copy link
Contributor

yuriy77k commented Mar 1, 2019

@MillianoConti I have no response from support@amo.foundation
Please, check email.

@MillianoConti
Copy link
Author

MillianoConti commented Mar 5, 2019 via email

@yuriy77k
Copy link
Contributor

yuriy77k commented Mar 5, 2019

Callisto Network provides security audits free of charge for smart-contract developers and development teams. And by our disclosure policy, we need to inform developers about found issues. Therefore we have to contact with them.

As far as we didn't receive confirmation from smart contract developer team, the request should be closed.

@yuriy77k yuriy77k closed this as completed Mar 5, 2019
@yuriy77k yuriy77k reopened this Mar 5, 2019
@yuriy77k yuriy77k added Low priority (DEPRECATED) DEPRECATED: Use priority -1 or priority -2 to decrease the priority of a contract audit. approved Auditors can begin to audit this smart contract. labels Mar 5, 2019
@MrCrambo
Copy link

MrCrambo commented May 1, 2019

Auditing time 2 days

@yuriy77k
Copy link
Contributor

yuriy77k commented May 1, 2019

@MrCrambo assigned

@danbogd
Copy link

danbogd commented May 1, 2019

Auditing time 5 days.

@yuriy77k
Copy link
Contributor

yuriy77k commented May 1, 2019

@danbogd assigned

@RideSolo
Copy link

RideSolo commented May 1, 2019

Auditing time: 3 days.

@yuriy77k
Copy link
Contributor

yuriy77k commented May 1, 2019

@RideSolo assigned

@danbogd
Copy link

danbogd commented May 6, 2019

My report is finished.

@yuriy77k
Copy link
Contributor

AMO Project Security Audit Report

1. Summary

AMO Project smart contract security audit report performed by Callisto Security Audit Department

2. In scope

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

3. Findings

In total, 7 issues were reported including:

  • 1 medium severity issues.

  • 2 low severity issues.

  • 2 notes.

  • 2 owner privileges (the ability of an owner to manipulate contract, may be risky for investors).

No critical security issues were found.

3.1. Multiple Token Transfers

Severity: medium

Description

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

Code snippet

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

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

3.2. Multiple Token Allocation

Severity: notes

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/AMO-Project/AMO-Contracts/blob/master/contracts/AMOCoinSale.sol#L400#L407

3.3. Owner Privileges

Severity: Owner privilege

Description

  1. 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).
  1. AMO sale owner privileges:
  • Token destinated to be sold through the crowdsale contract are approved and not transferred 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.
  • 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 transferred 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.
  • Owner can change minContribution, maxContribution, rate and hardCap for each sale round before starting sale.

3.4. Zero address

Severity: notes

Description

There is possibility of setting zero address as admin in function AMOCoin and as contract address in function setTokenSaleAmount.

Code snippet

https://github.com/AMO-Project/AMO-Contracts/blob/master/contracts/AMOCoin.sol#L85

https://github.com/AMO-Project/AMO-Contracts/blob/master/contracts/AMOCoin.sol#L101

3.5. Modifier will block correct working

Severity: low

Description

The modifier onlyValidDestination from AMOCoin contract will fail the function in case of transferring funds to sale address. For example in function setTokenSaleAmount there are approving funds for tokenSaleAddr. And after it should be transferred from to this address, but transferFrom function checks with using onlyValidDestination(to) modifier.

Code snippet

https://github.com/AMO-Project/AMO-Contracts/blob/master/contracts/AMOCoin.sol#L67

https://github.com/AMO-Project/AMO-Contracts/blob/master/contracts/AMOCoin.sol#L101

3.6. Known vulnerability of ERC-20 token

Severity: low

Description

It is possible to double withdrawal attack. More details here.

4. Conclusion

The audited smart contract must not be deployed. Reported issues must be fixed prior to the usage of this contract.

5. Revealing audit reports

https://gist.github.com/yuriy77k/2fc552fd10cb2f77139068f29cfdedb5

https://gist.github.com/yuriy77k/618cb51beedeebb119dd37991a77238f

https://gist.github.com/yuriy77k/23b820cc0dbcced4d173dcbd105b9464

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Auditors can begin to audit this smart contract. Low priority (DEPRECATED) DEPRECATED: Use priority -1 or priority -2 to decrease the priority of a contract audit. solidity Smart-contract is written in solidity (Ethereum) language.
Projects
None yet
Development

No branches or pull requests

6 participants