Jeiwan - An auction can never be extended due to an underflow

[sherlock-admin]

Jeiwan

medium

An auction can never be extended due to an underflow

Summary

An auction can never be extended due to an underflow

Vulnerability Detail

This operation will always revert with the arithmetic underflow error because maxDuration is a short period (3 days by default) and firstBidTime is a timestamp (AuctionHouse.sol#L141-L143):

auctions[tokenId].duration =
  auctions[tokenId].maxDuration -
  firstBidTime;

Also, the first branch in the if statement will never be reached during a first auction extension because newDuration will always be roughly duration * 2, thus greater than the maximal duration (duration is 2 days by default and the maximal duration is 3 days) (AuctionHouse.sol#L135-L144):

uint64 newDuration = uint256(
  duration + (block.timestamp + timeBuffer - firstBidTime)
).safeCastTo64();
if (newDuration <= auctions[tokenId].maxDuration) {
  auctions[tokenId].duration = newDuration;
} else {
  auctions[tokenId].duration =
    auctions[tokenId].maxDuration -
    firstBidTime;
}

Auction extension can be triggered only when an auction's duration has almost run out (AuctionHouse.sol#L127):

if (firstBidTime + duration - block.timestamp < timeBuffer) {

(Notice that firstBidTime is set during auction creation AuctionHouse.sol#L80.)

In such a situation, newDuration will always be roughly duration * 2:

1. (block.timestamp + timeBuffer - firstBidTime) will result in the remained time (roughly duration) + timeBuffer;
2. duration + the result of the above is roughly duration * 2.

Thus, the second branch will always be executed and there will always be a revert.

Impact

Bidders won't be able to place a bid when less than 15 minutes have left in an auction.

Code Snippet

AuctionHouse.sol#L93:

function createBid(uint256 tokenId, uint256 amount) external override {
  address lastBidder = auctions[tokenId].bidder;
  uint256 currentBid = auctions[tokenId].currentBid;
  uint256 duration = auctions[tokenId].duration;
  uint64 firstBidTime = auctions[tokenId].firstBidTime;
  require(
    firstBidTime == 0 || block.timestamp < firstBidTime + duration,
    "Auction expired"
  );
  require(
    amount > currentBid + ((currentBid * minBidIncrementPercentage) / 100),
    "Must send more than last bid by minBidIncrementPercentage amount"
  );

  // If this is the first valid bid, we should set the starting time now.
  // If it's not, then we should refund the last bidder
  uint256 vaultPayment = (amount - currentBid);

  if (firstBidTime == 0) { // @audit-info never true
    auctions[tokenId].firstBidTime = block.timestamp.safeCastTo64();
  } else if (lastBidder != address(0)) {
    uint256 lastBidderRefund = amount - vaultPayment; // @audit-info currentBid
    _handleOutGoingPayment(lastBidder, lastBidderRefund);
  }

  _handleIncomingPayment(tokenId, vaultPayment, address(msg.sender));

  auctions[tokenId].currentBid = amount;
  auctions[tokenId].bidder = address(msg.sender);

  bool extended = false;
  // at this point we know that the timestamp is less than start + duration (since the auction would be over, otherwise)
  // we want to know by how much the timestamp is less than start + duration
  // if the difference is less than the timeBuffer, increase the duration by the timeBuffer
  if (firstBidTime + duration - block.timestamp < timeBuffer) {
    // Playing code golf for gas optimization:
    // uint256 expectedEnd = auctions[auctionId].firstBidTime.add(auctions[auctionId].duration);
    // uint256 timeRemaining = expectedEnd.sub(block.timestamp);
    // uint256 timeToAdd = timeBuffer.sub(timeRemaining);
    // uint256 newDuration = auctions[auctionId].duration.add(timeToAdd);

    //TODO: add the cap to the duration, do not let it extend beyond 24 hours extra from max duration
    uint64 newDuration = uint256(
      duration + (block.timestamp + timeBuffer - firstBidTime)
    ).safeCastTo64();
    if (newDuration <= auctions[tokenId].maxDuration) {
      auctions[tokenId].duration = newDuration;
    } else {
      auctions[tokenId].duration =
        auctions[tokenId].maxDuration -
        firstBidTime;
    }
    extended = true;
  }

  emit AuctionBid(
    tokenId,
    msg.sender,
    amount,
    lastBidder == address(0), // firstBid boolean
    extended
  );

  if (extended) {
    emit AuctionDurationExtended(tokenId, auctions[tokenId].duration);
  }
}

Tool used

Manual Review

Recommendation

Consider setting duration to the maximal duration when newDuration is greater than the maximal one.

Duplicate of #18