CORIONplatform / solidity

GNU General Public License v3.0
12 stars 9 forks source link

ico: usage of send() #101

Closed gundas closed 7 years ago

gundas commented 7 years ago

send() function is used 4 times in ICO contract. It might run out of gas if the recipient is a contract which implements the fallback function (see http://www.blunderingcode.com/writing-secure-solidity/ for detailed explanations).

1. https://github.com/CORIONplatform/solidity/blob/master/ico.sol#L244 should be save to replace call.value(x)() since foundation can be trusted.

2. https://github.com/CORIONplatform/solidity/blob/master/ico.sol#L285 should be save to replace call.value(x)() since the caller is paying for gas and the call to the caller is at the end of the method.

3. https://github.com/CORIONplatform/solidity/blob/master/ico.sol#L319 is a more difficult case. The caller is paying for gas, but potentially could create a re-entry call back to buy. Not sure.

4. https://github.com/CORIONplatform/solidity/blob/master/ico.sol#L328 should be save to replace call.value(x)() since foundation can be trusted.

gundas commented 7 years ago

Similar issue is in a token constructor - if a genesis address is a contract, it might fail to receive the 0.2 ether. But if you provide unlimited gas to that call, it could just consume it all....

Dexaran commented 7 years ago

1, 2, 4 - I assume that foundationAddress and a person who will claim receiveFunds() should care about possibility of receiving funds.

Dexaran commented 7 years ago

https://github.com/CORIONplatform/solidity/blob/master/ico.sol#L319-L320

I think that L320 should be placed BEFORE L319 to avoit re-entry.

    _value = safeSub(_value, 0.2 ether);
    require( beneficiaryAddress.send(0.2 ether) );        
iFA88 commented 7 years ago

For https://github.com/CORIONplatform/solidity/blob/master/ico.sol#L319-L320 https://github.com/CORIONplatform/solidity/pull/103