Malicious user can win auction and become owner of tokenId without any collateral to pay original old owner.

Summary

Malicious user can win auction and become owner of tokenId without any collateral to pay original old owner. This is possible via function EnglishPeriodicAuctionInternal.sol::_cancelAllBids().

Vulnerability Detail

There are two functions to cancel bid, one is EnglishPeriodicAuctionInternal.sol::_cancelBid() and another one is EnglishPeriodicAuctionInternal.sol::_cancelAllBids(). The function _cancelBid() does not allow to cancel bid if it's highest bid

    function _cancelBid(
        uint256 tokenId,
        uint256 round,
        address bidder
    ) internal {
...............
        require(
            bidder != l.highestBids[tokenId][round].bidder,
            'EnglishPeriodicAuction: Cannot cancel bid if highest bidder'
        );
...............
        // Make collateral available to withdraw
        l.availableCollateral[bidder] += bid.collateralAmount;

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

here availableCollateral is the amount that can be withdrawed by bidder. _cancelBid() prevent highest bid's collateralAmount added to this availableCollateral. BTW, _cancelAllBids() does not prevent it and allow all along with highest bid.

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

So malicious user bid as highest amount until endTime of round, and right before end EnglishPeriodicAuctionFacet.sol::cancelAllBidsAndWithdrawCollateral() to get all deposited funds refunded. But the malicious user is highest bidder, and with closeAuction he wins without any collateral.

Impact

Malicious user can win auction and become owner of tokenId without any collateral to pay original old owner. and protocol can't pay old owner because there is not collateral in it.

Code Snippet

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

Tool used

Manual Review

Recommendation

Recommend that we need to check to prevent cancelling highest bid in _cancelAllBids() function.

Duplicate of #14

sherlock-audit / 2024-02-radicalxchange-judging

AgileJune - Malicious user can win auction and become owner of tokenId without any collateral to pay original old owner. #69