Extra value in bids is silently kept by the auction contract

[Arachnid]

Description

SaleClockAuction.sol:58 places the bid and computes the price, but does nothing with the price other than record it if the auction is a gen0 auction.

_bid in ClockAuctionBase:104 requires that the bid amount be at least as high as the current price. It then transfers the current price less the auctioneer's cut to the seller, and (implicitly) keeps the remainder.

In the case that the amount sent to the contract is more than the current price, this results in the auction contract keeping not just the auctioneer's cut, but also any excess funds. Instead, any excess should be returned to the buyer.

Scenario

Any time a user bids on an auction with a larger than required amount, they will be shortchanged by the auction contract. Because the dutch auction model continuously reduces the price, and because there is an inevitable delay between sending a transaction and it being mined, there will almost always be excess funds.

Impact

All bidders can be shortchanged on sale prices.

Reproduction

See "description" and "scenario".

Fix

Modify either _bid or bid to return excess funds to the caller.

[Arachnid]

Note this applies to SiringClockAuction, too.

[Arachnid]

Also note that fixing this will require modifying bidOnSiringAuction so KittyCore doesn't simply end up keeping excess funds in that case as well.

[dete]

Thanks for reporting this, @Arachnid!

This behaviour was actually intentional. Because of the nature of how our clock auction works, we anticipate that virtually every bid will have some small excess amount associated with it. However, we expected that in most cases, the gas cost of tracking and returning that excess would actually be higher than the amount in question. We figured that any non-trivial amount of excess would be something we could return manually.

It's worth pointing out that this logic was based on the assumption that the only safe way to return excess payment is to keep track of a withdrawal amount for each user, and include a withdrawal function, which is a ton of state. If it's safe to just call msg.sender.transfer(excess), that'd be a different story...

[Arachnid]

It's definitely safe to call msg.sender.transfer. They're the only ones who can cause that call to fail... in which case, why did they send the transaction in the first place?

[kimcope]

Thanks for your participation, @Arachnid! Our team has reviewed your submission, and we are pleased to reward you for your report.

Impact: Medium
Likelihood: Low
Points: 125

Please see the final leaderboard here.