Bidder can always win an auction for an amount less than his winning bid

Summary

As we know, canceling a bid can be done by calling _cancelBid; inside the function it ensures that the bidder is not the highest bidder.

    /**
     * @notice Cancel bid for current round if not highest bidder
     */
    function _cancelBid(
        uint256 tokenId,
        uint256 round,
        address bidder
    ) internal {
        EnglishPeriodicAuctionStorage.Layout
            storage l = EnglishPeriodicAuctionStorage.layout();

        address currentBidder;
        if (IStewardLicense(address(this)).exists(tokenId)) {
            currentBidder = IStewardLicense(address(this)).ownerOf(tokenId);
        } else {
            currentBidder = l.initialBidder;
        }

        require(
 ->           bidder != l.highestBids[tokenId][round].bidder,
            'EnglishPeriodicAuction: Cannot cancel bid if highest bidder'
        );

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

        require(
            bid.collateralAmount > 0,
            'EnglishPeriodicAuction: No bid to cancel'
        );

        // Make collateral available to withdraw
        l.availableCollateral[bidder] += bid.collateralAmount;

        // Reset collateral and bid
        bid.collateralAmount = 0;
        bid.bidAmount = 0;
    }

And as the docs state:

Only bids that are no longer the highest bid can be canceled and their associated collateral withdrawn.

Vulnerability Detail

However, a bidder also has the option to call _cancelAllBids instead.

    /**
     * @notice Cancel bids for all rounds
     */
    function _cancelAllBids(uint256 tokenId, address bidder) internal {
        EnglishPeriodicAuctionStorage.Layout
            storage l = EnglishPeriodicAuctionStorage.layout();

        uint256 currentAuctionRound = l.currentAuctionRound[tokenId];

        for (uint256 i = 0; i <= currentAuctionRound; i++) {
            Bid storage bid = l.bids[tokenId][i][bidder];

            if (bid.collateralAmount > 0) {
                // Make collateral available to withdraw
                l.availableCollateral[bidder] += bid.collateralAmount;

                // Reset collateral and bid
                bid.collateralAmount = 0;
                bid.bidAmount = 0;
            }
        }
    }

In this function, there is no validation to confirm whether the bidder currently is the highest bidder. Consequently, this enables the bidder to bypass the highest bidder check by calling _cancelAllBids rather than _cancelBid.

Impact

With this in mind the following scenario can be done:

Auction A has startprice = 0.1 eth
Bob will meet this startprice by bidding 0.1 eth
Bob creates a 2nd account and bids an enormous amount to ensure no one outbids him
once auction gets close to end time, Bob will cancel the bid on his 2nd account
his 1st account will now be the highest bidder
Bob ends up winning the auction by paying the bare minimum every time

Code Snippet

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/459dfbfa73f74ed3422e894f6ff5fe2bbed146dd/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L413-L434

Tool used

Manual Review

Recommendation

Make sure that when _cancelAllBids gets called it applies the highest bidder check

Duplicate of #14

sherlock-audit / 2024-02-radicalxchange-judging

FassiSecurity - Bidder can always win an auction for an amount less than his winning bid #62