o0ragman0o / itt Goto Github PK

If attacker makes many buy/sell orders differentiating slight price gap and requesting very small amount of (bidding/asking) tokens, contract might be in denial of service state in which every transaction would get Out-Of-Gas exception (beyond block gas limit), and ITT contract will be halted..

vulnerable code for DoS attack :

• CLL.seek in make function
• while loop in take function

How do you think about this?

Pls dear, thanks for your product you are doing a great job, but please how do one install your codes or script on a site so it works perfectly? thanks

It seems that next order's price is logged on "Sale" event

line 440 of ITT.sol
// prep for next order
bestPrice = spread(!tmsg.side);
Sale (bestPrice, takeAmount, msg.sender, maker);

I think Sale event should be fired before going to next order

Sale (bestPrice, takeAmount, msg.sender, maker);
// prep for next order
bestPrice = spread(!tmsg.side);

If Token issuer make Token with decimal places 18 (many real tokens such as Golem GNT use same decimal places with Ether, 18), ether prices used in trade should be multiple of 1 ether (1,2,3,4 ether, ... but not 0.1 ether for 1.000000000000000000 Token exchange)

So I suggest ether price in contract code should be "ether price per 1 base unit of target Token"
1 base unit of target token = 1*10^(decimals) tokens (1.000000000000000000 token)

is supporting multiple orders simultaneously hard to implement in contract?
is there good reason for supporting only singe price than multiple order prices?

I know I'm a pain in the neck :)
One thing that confused me in the ITTDesk is having an ether balance in the ITT contract.
What do you think of having a call to a new function to withdraw the ether balance after each successful trade operation?

This new function uses the xxxx.send(...) function, not .call, to avoid OOG. The .send function only send about 2.300 gas. If that withdraw fails (The recipient is a contract with a fallback function, etc..) you continue with the execution keeping the ether_balance and the user needs to withdraw manually.

Example:

    function safeNewSend(address _recipient, uint _ether)
        internal
        preventReentry()
        returns (bool)
    {
        return _recipient.send(_ether);
    }

function trade (uint _price, uint _amount, bool _side, bool _make) internal {
        ...
        balance[msg.sender] = tmsg.balance;
        //etherBalance[msg.sender] = tmsg.etherBalance;
        etherBalance[msg.sender] = safeNewSend(msg.sender, tmsg.etherBalance) ? 0 : tmsg.etherBalance;
}

Edit:
The same in the cancel function:

function cancel(uint _price) external canEnter returns (bool)
    {
        ...
        etherBalance[msg.sender] = safeNewSend(msg.sender, tmsg.etherBalance) ? 0 : tmsg.etherBalance;
        return true;
    }

I was reviewing the LIbCLL.sol and I saw that to find the next or the previous node, you have to look up into a mapping of booleans,

struct CLL{
    mapping (uint => mapping (bool => uint)) cll;
}

I was wondering why you didn't use a structure for that:

struct CLL_Node {
   uint prev;
   uint next;
}
struct CLL {
    mapping (uint => CLL_Node) cll;
}

Then I changed the code and I tested a bunch of operations with the new structure. It's slightly cheaper, about 2% less gas cost. I have the code in my computer.
If you thing it's important and your are interested in this change I can do a Pull Request with that.