dian.ivanov - Missing input validation/constraints on all setters

[sherlock-admin3]

dian.ivanov

medium

Missing input validation/constraints on all setters

Summary

Missing input validation may cause unexpected behaviour.

Vulnerability Detail

1. repossessor is not validated in _setRepossessor This may leave repossessorBid.bidder = address(0) if l.highestBids[tokenId][currentAuctionRound].bidder == address(0):

   function _closeAuction(uint256 tokenId) internal {
       ...
       if (l.highestBids[tokenId][currentAuctionRound].bidder == address(0)) {
           // No bids were placed, transfer to repossessor
           Bid storage repossessorBid = l.bids[tokenId][currentAuctionRound][
               l.repossessor
           ];
           repossessorBid.bidAmount = 0;
           repossessorBid.feeAmount = 0;
           repossessorBid.collateralAmount = 0;
           repossessorBid.bidder = l.repossessor;
   
           l.highestBids[tokenId][currentAuctionRound] = repossessorBid;
       }
       ...
   }

2. auctionLengthSeconds is not validated before being set _setAuctionLengthSeconds

This may not extend the length of the auction during _closeAuction if the value is too low or it may extend the auction too much:

function _closeAuction(uint256 tokenId) internal {
        ...
        if (
            auctionEndTime >= block.timestamp &&
            auctionEndTime - block.timestamp <
            _bidExtensionWindowLengthSeconds()
        ) {
            uint256 auctionLengthSeconds;
            if (l.currentAuctionLength[tokenId] == 0) {
                auctionLengthSeconds = _auctionLengthSeconds(); // this value can be too low or too high
            } else {
                auctionLengthSeconds = l.currentAuctionLength[tokenId];
            }
            // Extend auction
            l.currentAuctionLength[tokenId] =
                auctionLengthSeconds +
                _bidExtensionSeconds();
        }
        ...
}

3. minBidIncrement is not validated before being set _setMinBidIncrement

This may let bidders become the highest bid when the bid is equal to the older bid if the value is allowed to be 0 for example:

function _placeBid(
        uint256 tokenId,
        address bidder,
        uint256 bidAmount,
        uint256 collateralAmount
    ) internal {
        ...
        if (l.highestBids[tokenId][currentAuctionRound].bidAmount > 0) {
            require(
                bidAmount >=
                    l.highestBids[tokenId][currentAuctionRound].bidAmount +
                        l.minBidIncrement, // this can be 0
                'EnglishPeriodicAuction: Bid amount must be greater than highest outstanding bid'
            );
        }
        ...
}

4. bidExtensionWindowLengthSeconds is not validated before being set in _setBidExtensionWindowLengthSeconds

This may make the auction infinite if the value is too high:

function _closeAuction(uint256 tokenId) internal {
        ...
        if (
            auctionEndTime >= block.timestamp &&
            auctionEndTime - block.timestamp <
            _bidExtensionWindowLengthSeconds() // this value can be too high
        ) {
            uint256 auctionLengthSeconds;
            if (l.currentAuctionLength[tokenId] == 0) {
                auctionLengthSeconds = _auctionLengthSeconds();
            } else {
                auctionLengthSeconds = l.currentAuctionLength[tokenId];
            }
            // Extend auction
            l.currentAuctionLength[tokenId] =
                auctionLengthSeconds +
                _bidExtensionSeconds();
        }
        ...
}

5. bidExtensionSeconds is not validated before being set in _setBidExtensionSeconds.

This may not extend the length of the auction during _closeAuction if the value is too low or it may extend the auction too much:

function _closeAuction(uint256 tokenId) internal {
        ...
        if (
            auctionEndTime >= block.timestamp &&
            auctionEndTime - block.timestamp <
            _bidExtensionWindowLengthSeconds()
        ) {
            uint256 auctionLengthSeconds;
            if (l.currentAuctionLength[tokenId] == 0) {
                auctionLengthSeconds = _auctionLengthSeconds();
            } else {
                auctionLengthSeconds = l.currentAuctionLength[tokenId];
            }
            // Extend auction
            l.currentAuctionLength[tokenId] =
                auctionLengthSeconds +
                _bidExtensionSeconds(); // this value can be too low or too high
        }
        ...
}

6. startingBid is not validated before being set in _setStartingBid.

startingBid acts as a min bid amount allowed, if this value is allowed to be zero, this might lead to very cheap prices:

   function _placeBid(
        uint256 tokenId,
        address bidder,
        uint256 bidAmount,
        uint256 collateralAmount
    ) internal {
        EnglishPeriodicAuctionStorage.Layout
            storage l = EnglishPeriodicAuctionStorage.layout();

        uint256 currentAuctionRound = l.currentAuctionRound[tokenId];

        Bid storage bid = l.bids[tokenId][currentAuctionRound][bidder];

        // Check if higher than starting bid
        require(
            bidAmount >= l.startingBid,
            'EnglishPeriodicAuction: Bid amount must be greater than or equal to starting bid'
        );
   function _placeBid(
        uint256 tokenId,
        address bidder,
        uint256 bidAmount,
        uint256 collateralAmount
    ) internal {
        ...
        require(
            bidAmount >= l.startingBid, // startingBid can be very low
            'EnglishPeriodicAuction: Bid amount must be greater than or equal to starting bid'
        );
        ...
    }

Following variables in _initializeAuction are also set without being validated:

   function _initializeAuction(
        address repossessor,
        address initialBidder,
        uint256 initialPeriodStartTime,
        uint256 initialPeriodStartTimeOffset,
        uint256 startingBid,
        uint256 auctionLengthSeconds,
        uint256 minBidIncrement,
        uint256 bidExtensionWindowLengthSeconds,
        uint256 bidExtensionSeconds
    ) internal {
        ...
        l.initialBidder = initialBidder;
        l.initialPeriodStartTimeOffset = initialPeriodStartTimeOffset;
        l.initialPeriodStartTime = initialPeriodStartTime;
        ...
    }

Impact

Unexpected/weird behaviour in any function using the mentioned storage variables.

Code Snippet

Tool used

Manual Review

Recommendation

1. Add zero address check for repossessor in _setRepossessor
2. Add min/max value check for auctionLengthSeconds in _setAuctionLengthSeconds
3. Add min/max value check for minBidIncrement in _setMinBidIncrement
4. Add min/max value check for bidExtensionWindowLengthSeconds in _setBidExtensionWindowLengthSeconds
5. Add min/max value check for bidExtensionSeconds in _setBidExtensionSeconds.
6. Add min/max value check for bidExtensionSeconds in _setStartingBid.
7. Add input value checks for l.initialBidder, l.initialPeriodStartTimeOffset, l.initialPeriodStartTime before being set in _initializeAuction