Highest bidder can still cancel bid with cancelAllBidsAndWithdrawCollateral()

Summary

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/auction/facets/EnglishPeriodicAuctionFacet.sol#L185-L190 https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L378-L434 According to the dev comments on the cancelBid function, the highest bidder is not allowed to cancel their bid. Although this is adequately implemented in the internal _cancelBid function. It however can by bypassed when the highest bidder calls the cancelAllBidsAndWithdrawCollateral() which fails to check if the msg.sender is the highest bidder

Vulnerability Detail

In the EnglishPeriodicAunctionFacet.sol, three functions can be used to cancel a bid which are the cancelBid(), thecancelBidAndWithdrawCollateral() and cancelAllBidsAndWithdrawCollateral(). The vulnerability lies in the cancelAllBidsAndWithdrawCollateral() which further calls an internal _cancelAllBids(). Unlike the internal _cancelBid function which adequately checks if msg.sender is an highest bidder, the internal function _cancelAllBids() executed by the cancelAllBidsAndWithdrawCollateral() fails to check if msg.sender is the highest bidder allowing them to cancel bit and withdraw collateral

Impact

The protocol fails in its expected sequence, Highest bidder cancel bids during an aunction and withdraw collateral.

Code Snippet

The internal _cancel() bid which correctly assess if the bidder is 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'
    );

This however calls an internal _cancelAllBids function which fails to check if the bidder is the highest bidder.

function cancelAllBidsAndWithdrawCollateral(uint256 tokenId) external {
    _cancelAllBids(tokenId, msg.sender);
    _withdrawCollateral(msg.sender);
}

This is the internal _cancelAllBids() that fails to veto if bidder is highest bidder

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

Tool used

Manual Review

Recommendation

Require statement to check if bidder is highest bidder should be added to the _cancelAllBids() function too

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

Duplicate of #14

sherlock-audit / 2024-02-radicalxchange-judging

aycozynfada - Highest bidder can still cancel bid with cancelAllBidsAndWithdrawCollateral() #84