Users can cancel the highest bid for the current round

Summary

The highest bid for the ongoing current round can be canceled by users though cancelAllBidsAndWithdrawCollateral(), allowing bid manipulation and an inconsistent state that could lead to multiple severe problems.

Vulnerability Detail

Bids can be withdrawn allowing users to recover the bidded collateral. There are two internal functions that handle this, _cancelBid() and _cancelAllBids().

While _cancelBid() takes special care of checking that the canceled bid is not the highest bid for the round, its sibling _cancelAllBids() does not.

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;
        }
    }
}

We can how see the code loops through all rounds, including the current one, withdrawing the collateral and resetting the bid structure for all bids related to the given bidder.

Impact

This is a critical issue than can cause multiple problems:

Bidders can artificially inflate the auction price by placing a bid and then canceling it.
The highest bid won't have any collateral, ETH for the bid and fee amount can be withdrawn from the contract as the collateral is released.
If the auction is closed with a canceled highest bid, then not only the bidder will get the NFT transferred, but the old bidder and the fees will be eventually paid using funds from other bidders (or brick the process if there isn't available ETH).

Code Snippet

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

Tool used

Manual Review

Recommendation

Check that the bid is not the highest bid for the round, and skip cancellation if it is.

    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) {
+           if (bid.collateralAmount > 0 && bidder != l.highestBids[tokenId][i].bidder) {
                // Make collateral available to withdraw
                l.availableCollateral[bidder] += bid.collateralAmount;

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

Duplicate of #14

sherlock-audit / 2024-02-radicalxchange-judging

fugazzi - Users can cancel the highest bid for the current round #29