Bills of exchange factory

[ageyev]

Audit request

'Bills Of Exchange Factory' is a smart contracts based service that allows user to draw electronic bills or exchange.

'Bills Of Exchange Factory' creates new smart contracts for every new issue of bills.
Bills of exchange are created as tokens in separate smart contracts (separate smart contract for each new issue of bills). Every token (ERC20) is a bill of exchange in blank - payable to bearer (bearer is the owner of the Ethereum address witch holds the tokens, or the person he/she represents), but not to order - that means no endorsement is possible and the token holder can only transfer the token (bill of exchange in blank) itself.

Thus, all token bills in one smart contract have the same conditions (date of issue, drawer, drawee, amount, term, and so on). In one smart contract can exist one or many tokens (bills).
Tokens can be burned. For example, after making a payment on bills, the payer can "burn" these bills.

Every new created smart contract with bills of exchange get its number, and 'Bills Of Exchange Factory' smart contract maintains a ledger with numbers and addresses of smart contracts created via factory.

There is a web interface for smart contract (for now works on Ropsten only):
https://cryptonomica.net/bills-of-exchange/ , where you can find a more detailed description of the service and test smart contract with mock up data.

Source code

https://ropsten.etherscan.io/address/0x74eB4DBD3124D41B6775701FD1821571EAd5cf9A#code

Disclosure policy

We'd like to publish the report.

Platform

Ethereum

Number of lines:

445

[danbogd]

Auditing time: 3 days.

[yuriy77k]

@danbogd assigned

[MrCrambo]

Auditing time 2 days

[ghost]

Estimated audit time 3 days.

[yuriy77k]

@codeblcks assigned.
Please, don't take more then one audit at time.

[yuriy77k]

@MrCrambo assigned

[ghost]

@codeblcks assigned.
Please, don't take more then one audit at time.

Ok. Noted.

[gorbunovperm]

Estimated auditing time is 4 days.

[yuriy77k]

@gorbunovperm assigned

[danbogd]

My report is finished.

[gorbunovperm]

My report is finished.

[ghost]

My audit report is ready.

[yuriy77k]

@codeblcks where is your audit report? Please, send it to yuri@callisto.network

[ghost]

@codeblcks where is your audit report? Please, send it to yuri@callisto.network

I didn't have your email id. I have shared the audit report. Please check.

[yuriy77k]

Bills of exchange factory Security Audit Report

1. Summary

Bills of exchange factory smart contract security audit report performed by Callisto Security Audit Department

'Bills Of Exchange Factory' is a smart contracts based service that allows user to draw electronic bills or exchange.

https://cryptonomica.net/bills-of-exchange/

2. In scope

1. BillsOfExchangeFactory.sol

3. Findings

In total, 9 issues were reported including:

• 1 medium severity issues.

• 3 low severity issues.

• 1 notes.

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

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.

• initToken at line 187,
• changeCryptonomicaVerificationContractAddress at line 430,
• signDisputeResolutionAgreementFor at line 737,
• initBillsOfExchange at line 786,
• setLegal at line 851,
• createBillsOfExchange at line 981.

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:

1. 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)

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

3. change price (line 614)

4. Any admin can remove the contract creator from admin list. (line 487) Together with the ability to change the withdrawal address by admin, this can be quite dangerous.

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/26e2ee2e96ecb93091c5efbc2bddc7a4

https://gist.github.com/yuriy77k/adbf6e55c290b1382bf9c9dfea2c9ad2

https://gist.github.com/yuriy77k/135a6003cd890ee02a3edd90e566388a

https://gist.github.com/yuriy77k/fa0bcf86093ada4e7ba06e9bedb1bd81

[ageyev]

Remarks

• 3.1. Token Uses No Decimals

While the specification defined the number of token decimals to be 18

ERC-20 standard does not require of token decimals to be 18.
See discussion on ethereum/EIPs#724
There is no sense in a fraction of a bill of exchange, like no sense in 'half unicorn' (see comment ethereum/EIPs#724 (comment))

• 3.2. ERC223 Compliance: 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].

This is not true.

See the code:

    /**
    * overloaded transfer (like in ERC-223)
    * see: https://github.com/ethereum/EIPs/issues/223
    * https://github.com/Dexaran/ERC223-token-standard/blob/Recommended/ERC223_Token.sol
    */
    function transfer(address _to, uint _value, bytes calldata _data) external returns (bool success){
        if (transfer(_to, _value)) {
            ERC223ReceivingContract receiver = ERC223ReceivingContract(_to);
            receiver.tokenFallback(msg.sender, _value, _data);
            emit DataSentToAnotherContract(msg.sender, _to, _data);
            return true;
        }
        return false;
    }

'transfer(_to, _value)' called before '.tokenFallback' and it does add the token value to the balance before making the external call.

• 3.2. ERC223 Compliance: 1

ERC223 does not implement an approve/transferFrom mechanism.

But this in no way means that our contract can not or should not implement 'approve' as required by ERC-20 or 'transferFrom' as described in ERC-677

• 3.3. Known vulnerabilities of ERC-20 token

As described in comments in code, we are aware of the imperfection of the ERC-20 standard.
This is why we implement ERC-677, overloaded transfer like in ERC-223, overloaded 'approve' function as described on https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/

But we want our tokens to be ERC-20 compliant, so we keep legacy 'transfer' and 'approve' functions.

• 3.4. No checking for zero address

'initToken at line 187' -- can be called by factory contract only, one time. No possibility to have zero address here.

'changeCryptonomicaVerificationContractAddress at line 430' -- Can be called by admin only. If admin will set wrong address (any wrong address), the factory contract will stop working (not affecting already deployed bills of exchange contracts). But admin can easily change it to correct address. So we do not consider this an issue.

'signDisputeResolutionAgreementFor at line 737' -- entered address must be previously verified via cryptonomica.net smart contract, so there is no possibility it can be zero.

'initBillsOfExchange at line 786' - like 'initToken at line 187' can be called by factory contract only, one time only. No possibility to have zero address here.

'setLegal at line 851' - can be called by factory contract only, one time only. No possibility to have zero address here.

'createBillsOfExchange at line 981' - there is no argument of 'address' type in this function

So none of this cases really needs checking for zero address.

• 3.5. Missing event call.

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.

Will be changed in production by adding the following line:

emit Transfer(address(0), tokenOwner, totalSupply);

to 'initToken' function

• 3.6. Owner Privileges

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)

This is not true. Admin can change identity verification contract address used by factory. And it does not affect bills of exchange contracts already deployed via factory. And identity verification contract provides only information if identity of an Ethereum address was verified and this verification still valid and not revoked. Nothing like "implement any logic in the new contract"

fix or not fix withdraw address depends from owner.(line 541) ; change price (line 614)

Yes, admin can manage price for the service and address to withdraw funds. No issue there.

Any admin can remove the contract creator from admin list. (line 487) Together with the ability to change the withdrawal address by admin, this can be quite dangerous.

Any admin must be a person with valid identity verification (in implemented in the contract). It adds personal responsibility for admin actions.

So as you can see admins can change parameters of the factory contract providing service to users, such as price, identity verification contract address etc., and this was made intentionally. But admins have absolutely no control over smart contracts created by users using factory smart contracts.

---

Please, adjust audit conclusions/findings accordingly.