Solidity – Potential Violation of Checks-Effects-Interaction Pattern

contract-debuggingcontract-developmentremixsoliditytokens

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:

// refund the remainder
msg.sender.transfer(refundableRemainderInWei);
Bought(msg.sender, numToDistribute);

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 the transfer 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.

Related Topic