TokenTrader Smart Contracts security audit.

ETH_TokenTrader_audit_report.md

Ethereum TokenTrader Smart Contracts security audit

Summary

This is the report from a security audit performed on TokenTrader by gorbunovperm.

Decentralized trustless ether/ERC20-compliant token exchange contract on the Ethereum blockchain.

In scope

1. TestERC20Token.sol
2. TokenSellerFactory.sol
3. TokenTraderFactory.sol

Excluded

Findings

In total, 6 issues were reported including:

• 0 high severity issue.

• 3 medium severity issues.

• 0 low severity issues.

• 3 minor observations.

Security issues

Issues for TestERC20Token.sol

1. Double spending attack is possible.

Severity: medium

Code snippet

function approve(
    address _spender,
    uint256 _amount
) returns (bool success) {
    allowed[msg.sender][_spender] = _amount;
    Approval(msg.sender, _spender, _amount);
    return true;
}

Description

When changing the allowance value is it possible to spend previous allowance funds(with high gas price for example) and after it changed spend already new allowance funds.

Recommendation

Change the implementation of approve mechanism. For example, before change allowance you can check if the funds already spent.

Issues for TokenSellerFactory.sol

2. Overflowing is possible.

Severity: medium

Code snippet

Lines 186-189

if (msg.value > (can_sell * sellPrice)) {
    change  = msg.value - (can_sell * sellPrice);
    order = can_sell;
}

Line 194

if (!ERC20Partial(asset).transfer(msg.sender, order * units)) throw;

Description

In some cases can_sell * sellPrice may be more than max possible value for uint256. For example, if sellPrice is 2^248 and can_sell is 256. This can lead to execution of the order almost for free.

Recommendation

Use SafeMath library.

Issues for TokenTraderFactory.sol

3. Overflowing is possible.

Severity: medium

Code snippet

Line 319

if (!msg.sender.send(order * buyPrice)) throw;

Line 317

if (!ERC20(asset).transferFrom(msg.sender, address(this), order * units)) throw;

Lines 272-275

if (msg.value > (can_sell * sellPrice)) {
    change  = msg.value - (can_sell * sellPrice);
    order = can_sell;
}

Lines 279-282

if (order > 0) {
    if (!ERC20(asset).transfer(msg.sender, order * units)) throw;
}
TakerBoughtAsset(msg.sender, msg.value, change, order * units);

Description

In some cases there may be more than max possible value for uint256. This can lead to execution of the order almost for free.

Recommendation

Use SafeMath library.

Issues for all files

4-6. Code standards

Severity: minor observations

Code snippet

1.

function verify(address tradeContract) constant returns ( /* ... */ ) { /* ... */ }

2.

if (!msg.sender.send(change)) throw;

3.

pragma solidity ^0.4.4;

Description

1. constant label for functions is deprecated.
2. throw is deprecated.
3. Compiler version is set as "more than"(pragma solidity ^0.4.4;). Future versions of the compiler may use constructions not intended by the developer.

Recommendation

1. Use view label instead constant for functions.
2. Use require() instead if-throw constructions.
3. Set fixed version of the compiler, like this pragma solidity 0.4.4; (without ^).

Conclusion

There is several issues in these contracts. One high severity, but in token for testing(is why I marked it as medium severity). And one type of issue in work contracts with medium severity. It is recommended to fix it.