Bills of exchange factory v.2

[ageyev]

Audit request

Previous audit: https://github.com/EthereumCommonwealth/Auditing/issues/261

'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.... Briefly describe your smart-contract and its main purposes here ...

There is a web interface for smart contract: 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://etherscan.io/address/0xAA2CB1a1b014b92EDb93B1f163C7CA40a07EEcAa#contracts

Disclosure policy

We want to publish the report.

Platform

Ethereum

Number of lines:

223 (446 * 0.5 reaudit)

Previous audit

Previous audit request: https://github.com/EthereumCommonwealth/Auditing/issues/261

Changes after first audit and notes to first audit report

• [x] 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 https://github.com/ethereum/EIPs/issues/724 There is no sense in a fraction of a bill of exchange, like no sense in 'half unicorn' (see comment https://github.com/ethereum/EIPs/issues/724#issuecomment-332855002)

• [x] 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.

• [x] 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

• [x] 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.

• [x] 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.

• [x] 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.

Was changed by adding the following line:

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

to 'initToken' function

• [x] 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 owner was verified and if this verification is 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 provided by "bills of exchange factory" smart contract 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 (checked via Cryptonomica verification smart 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, but admins have absolutely no control over smart contracts created by users using factory smart contracts.

[MrCrambo]

Auditing time 1 day

[yuriy77k]

@MrCrambo assigned

[RideSolo]

Auditing time: 1 day.

[yuriy77k]

@RideSolo assigned

[MrCrambo]

My report is finished

[gorbunovperm]

Estimated auditing time is 2 days.

[yuriy77k]

@gorbunovperm assigned

[gorbunovperm]

My report is finished.

[yuriy77k]

Bills of exchange factory v.2 Security Audit Report

1. Summary

Bills of exchange factory v.2 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

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

3. Findings

In total, *2 issues** were reported including:

• 1 medium severity issues.

• 1 low severity issues.

3.1. 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 the following code to the transfer(_to address, ...) function:

require( _to != address(this) );

3.2. ERC223 Compliance: The tokenFallback function is not called when it should be

Severity: medium

Description

The main idea of ERC223 is to protect the tokens from loss by sending to a contract that is not intended to work with ERC20 tokens. Look at Motivation of ERC223. But here the main transfer(address, uint256) function is not implement tokenFallback() mechanism.

Code snippet

• transfer(), line 251

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/adce899cb0b55cb4d52f34a0909ff965

https://gist.github.com/yuriy77k/9b945659e8dff3dd4b2ccb19725d0421

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