Questions about SimpleAuction contract example

Question

Here is the official sample of solidity:

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;
}

Question 1: What is the meaning of

because it can be prevented by the caller by e.g. raising the call stack to 1023*

if (now <= auctionStart + biddingTime)
    throw; // auction did not yet end
if (ended)
    throw; // this function has already been called

// 2. Effects
ended = true;
AuctionEnded(highestBidder, highestBid);

// 3. Interaction
if (!beneficiary.send(highestBid))
    throw;

Question 2: If beneficiary.send(highestBid) failed, ended has been modified to true, and beneficiary.send(highestBid) will never be executed again. Is that a bug?

The title of this question is very confusing. It should say "Questions about SimpleAuction contract" And, if the contract pointed to by the link is not the "official" version of the SimpleAuction, then the title should not say it is. I'm not sure what official even means in this context. — Thomas Jay Rush, Dec 28 '16 at 12:10

score 9 · Accepted Answer · edited Jun 16 '20 at 10:58

9

Q1 : this a protection measure against call depth stack attack. a well known attack against the smart contracts in which the attacker (which is a malicious contract) exploits the fact that the calls with a depth of 1024 fails.

Therefore, a malicious contract could raise the stack call to change the previous information (highestBidder ..) to reach the max stack at the call of highestBidder.send(highestBid) which will fail provoking in this scenario a prevention of sending back the money the highest bidder. so an attacker could make a new high bid (becomes new highestBidder ) and prevent the previous high bidder to get its money back.

Q2 : beneficiary.send(highestBid) has to be executed (manualy) only once (there is one Highestbidder) at the end of the auction. that's why the ended is set to true before.

edited Jun 16 '20 at 10:58

Community

1

answered Dec 28 '16 at 11:10

Badr Bellaj

18,780
4
58
75

1

about Q2: if beneficiary.send(highestBid) failed, but ended has been changed to true. Because of beneficiary.send(highestBid) could be executed only once, so the winner can never get its ether back?? – user2781581 Dec 28 '16 at 12:36
1

I've looked at the original code, if send fails then the code(auctionEnd) will throw so the states will not change ('ended' remain false) – Badr Bellaj Jan 03 '17 at 13:08

score 4 · Answer 2 · answered Dec 28 '16 at 19:09

Q1. It might helpful to describe the problem the code solves. Consider an intuitive but badly constructed function along these lines (naive function):

Record new high bidder
Return funds to the old high bidder.

If anything goes wrong in step 2, then step 1 doesn't happen either. This is because an incomplete transaction will revert to the pre-transaction state. It's "all, or nothing".

Suppose Alice bids, and then Bob outbids her. The naive function would try to send a refund to Alice. But, nothing is known about Alice. Alice could be a contract that doesn't want the money back. Alice could call itself until stack depth or gas is used up causing the whole transaction to fail. Alice would be able to make small bids and win every auction, because no one would be able to successfully place a bid against her.

The solution is to separate the concerns. Record the highest bid and the resulting liabilities (e.g. pendingReturns[]). Refund the pendingReturns[] in a separate step. Let those interactions succeed or fail as separate transactions. Nothing Alice can do to interfere with Bob's bid.

Q2. "Throw" on failed send (Step 3) reverts state changes, so "ended=true" (step 2) doesn't happen in that case, even though it came first. It's still false until the whole transaction completes without "throw". Consider it in this order: 1) account for state changes optimistically, 2) try it, 3) revert state changes unless the transaction is completely successful.

score 3 · Answer 3 · answered Dec 28 '16 at 12:07

Q1: If the winner of the auction is a malicious smart contract, it might be written so that it repeatedly calls itself until the call-depth limit is reached. If this happens, the 'send' fails. If the auction contract tries to send ether to a potential winner, and that winner repeatedly (or purposefully) fails, the ether gets locked in the contract. Therefore, it's better to force the winner to withdraw the ether themselves.

Questions about SimpleAuction contract example

3 Answers3

Linked