search

dapperlabs / cryptokitties-bounty

Bounty program for CryptoKitties smart contract
https://www.cryptokitties.co/
Other
482 stars 228 forks source link

Extra value in bids is silently kept by the auction contract #2

Open Arachnid opened 6 years ago

Arachnid commented 6 years ago

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 commented 6 years ago

Note this applies to SiringClockAuction, too.

Arachnid commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.