That's what Remix is telling me. Now when I look at the documentation as to what this means it seems to be talking about, as I understand it, recursively entering a control flow (if):
Ether transfer can always include code execution, so the recipient
could be a contract that calls back into withdraw
Looking at my code I cannot see how this could be exploited, I don't even add the tokens to the receiving account until the end, after requirements, the only thing I can see is that could then go wrong or be exploited is that they wouldn't get their change:
/*
* Allows a user to buy tokens for Ether.
*/
function buy() public payable {
// check that the amount of ether sent is greater than 0
require(msg.value > 0);
// check the amount sent is enough for at least one token
require(msg.value >= tokenPriceInWei);
// NOTE: balance is automatically updated by the payable modifier
// calculate tokens to be distributed as integer
uint numToDistribute = msg.value / tokenPriceInWei;
// calculate remainder to be refunded, e.g. 7.1eth sent, 1.1eth to be refunded
uint refundableRemainderInWei = msg.value - (toWei(numToDistribute));
// distribute token to sender
distribute(msg.sender, numToDistribute);
// refund the remainder
msg.sender.transfer(refundableRemainderInWei);
Bought(msg.sender, numToDistribute);
}
/*
* Distribute tokens from distributor to a receiver.
*/
function distribute(address _receiver, uint _amount) private {
// check for at least one token, if not something went wrong
require(_amount > 0);
// check for balance overflow, causing incorrect balances value
require(balances[_receiver] + _amount > balances[_receiver]);
// decrement from distributor and increment receiver
balances[distributor] -= _amount;
balances[_receiver] += _amount;
}
Additionally if anyone can tell me why the gas cost for this method is high to infinite and why it says the same when I try to deploy the contract through Mist and cannot deploy, that'd really make my day!
Thankyou if you read this far and for any advice you can give to this budding smart contract engineer!
Best Answer
Change this:
To this:
// refund the remainder Bought(msg.sender, numToDistribute); msg.sender.transfer(refundableRemainderInWei);
Re-entrancy attack can be thought of as changing the flow in an unexpected way before an important state change. So, a good defense is to get your house in order before transfering control to another contract, which
transfer
does.Placing the event emitter ahead of the
transfer
ensures the log is updated and correct. It's "optimistic accounting". Should thetransfer
fail for some reason, it will cause a revert and there will be no event emitted.The gas is unknown or possibly infinite because the
transfer
receivers are unknown. They may be externally owned accounts that just accept funds. On the other hand, they may be contracts with fallback functions that run when funds are received (this is the flow control transfer).At best, those functions will use an unknown amount of gas. It could be infinite if they outright refuse to accept ether (by accident or by design).
The warning indicates that the estimate just isn't consistent or knowable, and the developer is warned of the possibility of complete failure in some cases.
Hope it helps.