Solidity: Understanding the security risk of sending back money with send

Question

I'm looking at the auction example here https://solidity.readthedocs.io/en/develop/solidity-by-example.html

I'm confused by this code

    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 recipient withdraw their money themselves.
        pendingReturns[highestBidder] += highestBid;
    }

Can someone explain this risk in more depth? This seems kind of annoying that a smart contract would need to have the recipient make a call to get their money instead of automatically sending it but maybe I'm misunderstanding this.

Answer 1

There are some non-obvious concepts here and some heavily loaded terms all of which can lead to confusion about the safe way to proceed.

Either the new bidder or the outbid bidder could be hostile contracts. There are various things they could do to interfere with the correct/expected execution of this function.

As a general heuristic, we can think of them as "untrusted contracts".

There are a number of defensive habits we can employ to defeat mischief by untrusted contracts. Among these are:

1. Avoid dealing with more than one untrusted contract at a time.
2. Avoid mixing up multiple concerns in a single function/transaction.
3. The withdraw() function offers a good example of a safe send pattern that's also important.*

Immediate refund would violate #1 & #2 by 1) interacting with both the untrusted sender and the untrusted receiver in a single transaction and 2) mixing up the concern of processing the new bid with the concern of refunding the previous highest bid.

As alluded to in the remarks, there is a call stack attack that can cause a .send() to silently fail. In the context of this contract, if bidding and refunds were intermingled that would mean the bidder could possibly outbid the high bid and prevent the refund. That would be bad.

Handled as separate concerns, it's harder to imagine how the bidder can interfere with refunds, or how the refund can interfere with the bid.

The withdrawal pattern does indeed insert an extra step. It's not necessarily a human-facing step because much can be done in the front-end to call the right function(s) when needed. In any case, security must take priority over convenience.

Hope it helps.

// Withdraw a bid that was overbid.
    function withdraw() {
        uint 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 (see the remark above about
            // conditions -> effects -> interaction).
            pendingReturns[msg.sender] = 0;

            msg.sender.transfer(amount);
        }
    }

Answer 2

This exploit is summarized in this paper that details Solidity vulnerabilities, on page 11:

Stack size limit. Each time a contract invokes another contract (or even itself via this.f()) the call stack associated with the transaction grows by one frame. The call stack is bounded to 1024 frames: when this limit is reached, a further invocation throws an exception.

Until October 18th 2016, it was possible to exploit this fact to carry on an attack as follows. An adversary starts by generating an almost-full call stack (via a sequence of nested calls), and then he invokes the victim’s function, which will fail upon a further invocation. If the exception is not properly handled by the victim’s contract, the adversary could manage to succeed in his attack. This vulnerability could be exploited together with others: e.g., in Section 4.5 we implement a malicious contract by exploiting the “exception disorder” and “stack size limit” vulnerabilities.

Note that this exploit has been eliminated from Ethereum as of EIP150, and the fix is also detailed in the paper:

This cause of vulnerability has been addressed by an hard-fork of the Ethereum blockchain [1]. The fork changed the cost of several EVM instructions, and redefined the way to compute the gas consumption of call and delegatecall. After the fork, a caller can allocate at most 63/64 of its gas: since, currently, the gas limit per block is ∼4,7M units, this implies that the maximum reachable depth of the call stack is always less than 1024 [10].