sandy - Highest bidder can cancel his bids and withdraw all the collateral using ``_cancelAllBids()`` function.

[sherlock-admin2]

sandy

high

Highest bidder can cancel his bids and withdraw all the collateral using _cancelAllBids() function.

Summary

Highest bidder can cancel his bids and withdraw all the collateral using _cancelAllBids() function which causes potential implications.

Vulnerability Detail

_cancelBid() function is a function which cancels bid for the current round and makes the collateral available for the withdrawal. But there is a check to ensure the highest Bidder can't call this function.

./EnglishPeriodicAuctionInternal.sol

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

But highest bidder can still cancel his/her bid using _cancelAllBids() function:

./EnglishPeriodicAuctionInternal.sol

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

If the higher bidder is successfully able to cancel his/her bid and make the collateral he put up available for withdrawal, these can lead to potential implications.

Impact

When the auction is ended, in the _closeAuction() function,

./EnglishPeriodicAuctionInternal.sol

else if (l.highestBids[tokenId][currentAuctionRound].bidder != oldBidder) {
            // Transfer bid to previous bidder's collateral
            l.availableCollateral[oldBidder] += l.highestBids[tokenId][currentAuctionRound].bidAmount;
            l.highestBids[tokenId][currentAuctionRound].collateralAmount = 0;
            l.bids[tokenId][currentAuctionRound][l.highestBids[tokenId][currentAuctionRound].bidder].collateralAmount =
                0;
        } 

the bidAmount put up by the highest bidder is given to the oldBidder. And feeAmount calculated on the bidAmount of the highest bidder is transferred to the beneficiary.

./EnglishPeriodicAuctionInternal.sol#_closeAuction()

        if (l.highestBids[tokenId][currentAuctionRound].feeAmount > 0) {
            IBeneficiary(address(this)).distribute{value: l.highestBids[tokenId][currentAuctionRound].feeAmount}();
        }

Both of these bidAmount and feeAmount is covered by the collateralAmount that can be withdrawn from the contract by cancelling the bid.

Note: Even if the highest bid is cancelled, _closeAuction() function will run successfully.

Thus, This vulnerability can lead to a couple of potential impacts:

1. If the highest bidder exploits this vulnerability, he/she can get the token from the auction, without spending a dime(expect for gas cost).
2. The bidAmount and feeAmount won't be backed by any collateral amount, thus feeAmount transferred to the beneficiary and bidAmount going to the oldBidder from the highest bidder will be taken from the collateral amount sent by other non-highest bidders. Thus, loss of collateral funds for other bidders.
3. _closeAuction function may revert if the highest bidder is able to cancel his/her bid and withdraw the collateral before _closeAuction() function is called due to lack of collateral needed for feeAmount and cause DoS.

Code Snippet

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

Tool used

Manual Analysis

Recommendation

Change the _cancelAllBids() function to not allow the highest bidder to cancel his/her bid:

./EnglishPeriodicAuctionInternal.sol

    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++) {
+            require(
+               bidder != l.highestBids[tokenId][i].bidder,
+                "EnglishPeriodicAuction: Cannot cancel bid if highest bidder"
+            );
            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;
            }
        }
    }

Duplicate of #14