CEO could transfer entire balance to a seller

Description

There are actually not full guarantees that the cut is no greater than 100%. A complex auction contract could overwrite the ownerCut state to a number that sends the whole balance of the auction to a seller.

Scenario

CEO sets an auction (either sire or selling) that overwrites the ownerCut state after the require(_cut <= 10000) in the ClockAuction constructor has completed. This would presumably be an accident, but could also be a malicious CEO who is also the seller. The transfer: https://github.com/axiomzen/cryptokitties-bounty/blob/282e43bcefad4b9446a281855eef7d63495c2d53/contracts/Auction/ClockAuctionBase.sol#L148 would be a very large number due to storing a negative number in an unsigned integer. It could be the exact balance of the auction.

Impact

Seller drains auction account(s) and can continue to drain them by placing more auctions.

Reproduction

function WhoopsClockAuction(address _nftAddr, uint256 _cut) public ClockAuction(_nftAddr, _cut) {
  //the require(_cut <= 10000) has completed at this point
  ownerCut = 9999999999;  //malicious value.
}

The bad state could also be caused by "complex math" in the future. i.e. you want to autonomously continuously deploy new contracts that have different cuts based on some external factors. If there is a mistake in this math, the number could be bad. I think malicious is more likely but who knows how complex futures contracts could be?

Fix

Add a require(ownerCut <= 10000) in function _computeCut(uint256 _price).

dapperlabs / cryptokitties-bounty