Skip to content

Instantly share code, notes, and snippets.

@yuriy77k
Forked from danbogd/ETH_Billsexchangefactory.md
Created June 1, 2019 05:57
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/135a6003cd890ee02a3edd90e566388a to your computer and use it in GitHub Desktop.
Save yuriy77k/135a6003cd890ee02a3edd90e566388a to your computer and use it in GitHub Desktop.

Bills of exchange factory audit report.

1. Summary

This document is a security audit report performed by danbogd, where Bills of exchange factory has been reviewed.

2. In scope

Сommit hash .

3. Findings

In total, 8 issues were reported including:

  • 1 medium severity issues
  • 3 low severity issues
  • 3 owner privileges (ability of owner to manipulate contract, may be risky for investors)..
  • 1 notes.

No critical security issues were found.

3.1. Token Uses No Decimals

Severity: note

Description

While the specification defined the number of token decimals to be 18, no decimals were found to be used. This can cause problems when interacting with other smart contracts as tokens with 0 decimals can cause rounding errors. For example, many exchanges charge a small fee based on the tokens exchanged. As such, using no decimals will either make it impossible to list the token on these exchanges or it will result in having expensive fees compared to other tokens.

Code snippet

Line 153.

3.2. ERC223 Compliance

Severity: medium

Description

The reviewed token contract is not ERC223 fully compliant.

  1. The function transfer(address _to, uint _value, bytes _data) call tokenFallback external function on the receiver contract without adding the value to balances[_to]. The original implementation adds the token value to the balance before making the external call here

  2. ERC223 does not implement an approve/transferFrom mechanism.

3.3. 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.

Recommendation

Add into a function transfer(address _to, ... ) following code:

require( _to != address(this) );

3.4. No checking for zero address

Severity: low

Description

In this functions there are no checking for zero address.

Code snippet

Lines: 430, 451.

3.5. Missing event call.

Severity: low

Description

According to ERC20 standard, when initializing a token contract if any token value is set to any given address a transfer event should be emitted. An event isn't emitted when assigning the initial supply to the msg.sender.

Code snippet

Line 200.

3.6. Owner Privileges

Severity: owner previliges

Description

Contract owner allow himself to:

  • Owner can upgrade contract and implement any logic in the new contract. And even if the new contract will be audited, at any time possible to change the address of the new contract again to not audited and insecure. (line 430)

  • fix or not fix withdraw address depends from owner.(line 541)

  • change price (line 614)

4. Conclusion

The audited smart contract is not safe to deploy.

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