Skip to content

Instantly share code, notes, and snippets.

@yuriy77k
Forked from RideSolo/ETH_Natmin_audit_report.md
Created September 16, 2018 19:06
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save yuriy77k/7a8c8bdd49b85fe6c60266fe89c46e77 to your computer and use it in GitHub Desktop.
Save yuriy77k/7a8c8bdd49b85fe6c60266fe89c46e77 to your computer and use it in GitHub Desktop.

Natmin Token Audit Report.

1. Summary

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

Token Description:

  • Name: Natmin
  • Symbol: NAT
  • Decimals: 18
  • Standard: ERC20/ERC223
  • Total Supply: 400,000,000

2. In scope

  • NatminToken.sol github commit hash 63e07c75ee81f39dbaa5fec6b0fd940f3ce8f7b0.

3. Findings

4 issues were reported including:

  • 3 medium severity issues.

  • 1 low severity issues.

3.1. Vesting Logic

Severity: medium

Description

If the owner set a vesting amount and an end time for a user using addVesting function, and if the user receives tokens from another address, he won't be able to transfer the extra amount even if balancesOf[user] > vestings[user].amount.

Code snippet

https://github.com/NatminPureEscrow/Token/blob/master/contracts/NatminToken.sol#L274#L277

https://github.com/NatminPureEscrow/Token/blob/master/contracts/NatminToken.sol#190

https://github.com/NatminPureEscrow/Token/blob/master/contracts/NatminToken.sol#209

Recommendation

Depending on the contract design, the above issue can be considered as not relevant. In the other case, the devs should consider allowing users to spend the difference between balancesOf[user] and vestings[user].amount at anytime.

3.2. ERC223 Standard Compliance

Severity: medium

Description

The reviewed token contract is not ERC223 fully compliant, transferToContract function member of NatminToken contract call tokenFallback external function on the receiver contract before adding the value to balances[_to]. The original implementation adds the token value to the balance before making the external call (check the link below).

https://github.com/Dexaran/ERC223-token-standard/blob/master/token/ERC223/ERC223_token.sol#L63#L68

Different issues can be raised depending on tokenFallBack implementation on the receiver contract. A good example is if the contract tries to move the tokens from its balance when the tokens are not yet added to it.

Code snippet

https://github.com/NatminPureEscrow/Token/blob/master/contracts/NatminToken.sol#L188#L224

3.3. Token Transfer to Address 0x0

Severity: medium

Description

The implemented version of ERC20/ERC223 Token do not require the _to address in transferToAddress and transferFrom to be non null before token transfer. Accidental token loss to address 0x0 can be applicable.

Code snippet

https://github.com/NatminPureEscrow/Token/blob/master/contracts/NatminToken.sol#L207

https://github.com/NatminPureEscrow/Token/blob/master/contracts/NatminToken.sol#L220

3.4. Known Issue of ERC20 Standard

Severity: low

Description

This is just a reminder for the contract developers (the described ERC20 issue is well-known and well documented).

Approve + transferFrom mechanism allows double Withdrawal attack.

4. Conclusion

The contract is safe to be deployed, However the current implementation of Natmin risks to conflict with contract when transferring token to them and throw the transfer transaction.

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