search

sherlock-audit / 2024-02-radicalxchange-judging

3 stars 1 forks source link

neon2835 - The _cancelAllBids function has a vulnerability, as it does not verify whether the user at this time is the highest bidder #21

Closed sherlock-admin3 closed 8 months ago

sherlock-admin3 commented 8 months ago

neon2835

high

The _cancelAllBids function has a vulnerability, as it does not verify whether the user at this time is the highest bidder

Summary

The _cancelAllBids function has a vulnerability, as it does not verify whether the user at this time is the highest bidder.

Vulnerability Detail

Suppose there is the following scenario:

  1. Alice participates in bidding for a token. In the final round of bidding, Alice is the highest bidder and she is about to win the auction.

  2. At the end of the auction, Alice preemptively calls the cancelAllBidsAndWithdrawCollateral function to withdraw all her collateral from the _withdrawCollateral function. Alice then calls the closeAuction function to withdraw the auction token. These calls are executed within a single transaction, allowing Alice to win the auction successfully and free of charge.

Impact

The user with the highest bid can win the auction for free by using the cancelAllBidsAndWithdrawCollateral function

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

Optimize the _cancelAllBids function

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

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