Security implications in solidity

Question

I am reading through the solidity docs and came across this comment in the code.

Can some one please explain how this is a security risk? Is it talking about calling a new contract using delegate which re-enters recursively ? I suppose i still fail to understand the security implications.

function bid() payable {
    // No arguments are necessary, all
    // information is already part of
    // the transaction. The keyword payable
    // is required for the function to
    // be able to receive Ether.

    // Revert the call if the bidding
    // period is over.
    require(now <= (auctionStart + biddingTime));

    // If the bid is not higher, send the
    // money back.
    require(msg.value > highestBid);

    if (highestBidder != 0) {
        // Sending back the money by simply using
        // highestBidder.send(highestBid) is a security risk
        // because it can be prevented by the caller by e.g.
        // raising the call stack to 1023. It is always safer
        // to let the recipients withdraw their money themselves.
        pendingReturns[highestBidder] += highestBid;
    }
    highestBidder = msg.sender;
    highestBid = msg.value;
    HighestBidIncreased(msg.sender, msg.value);
}

In the below function, it talks about setting a storage variable to 0 , in case the function is called again, I fail to understand how is this again a security issue. Is this something similar to multithreading in C++ environment (for lack of a better comparison i could think of).

function withdraw() returns (bool) {
    var amount = pendingReturns[msg.sender];
    if (amount > 0) {
        // It is important to set this to zero because the recipient
        // can call this function again as part of the receiving call
        // before `send` returns.
        pendingReturns[msg.sender] = 0;

        if (!msg.sender.send(amount)) {
            // No need to call throw here, just reset the amount owing
            pendingReturns[msg.sender] = amount;
            return false;
        }
    }
    return true;
}

Since each call is more or less a transaction, shouldn't there be no problems with them being re-entrant?

Thanks!

Answer 1

This has to do with re-entrance attacks.

There is a lot of information about that out there so I'll summarize briefly.

.send() and .transfer() transfer funds but they also transfer flow control. This is because the receiver may be a contract, and it may have contract code to run upon receipt of funds (fallback function). Whatever it does, it happens before the caller's contract continues with the next step.

Security implication: The next step may never happen. Why? Because it's an oversight to naively assume the called contract will return control as/when expected. It may very well loop around and invoke the called contract again.

This danger has been partially addressed with a gas stipend of 2,300 gas for send() which is aimed at ensuring receivers run out of gas before they get up to mischief. Still, it is best practice to code so that re-entrance attacks will fail.

The principle defensive idea is to ensure a complete contract state before transferring control. In other words, get all the state variables in order first, so looping around is not a threat.

In practice:

1. Optimistic accounting. Just update all state variables as if success is assured.
2. Try the send() and check the result.
3. On fail, revert the state changes. throw does the trick.

Steps 2 and 3 are addressed by the new .transfer() method which is similar to if(!address.send(amount)) throw;

Related: Treat send()/transfer() and payble as messages to/from untrusted contracts and avoid designing functions that talk to more than one untrusted contract at a time. That will help prevent DoS attacks by contracts that throw; in the middle of your process just to jam up the works.

Here's a (hopefully) easy to parse example of re-entrance in action (not intended as a how-to for thieves.)

Simple re-entrance attack example with fallback function

Hope it helps.