`cancelBid()` and `cancelAllBids()` functions are incorrectly implemented, resulting in partial/complete DoS-ing of bid cancelling functionality.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/a6f2397b68ef2865374c1bf7629349f25e71a44d/smart-contracts/AuctionDemo.sol#L122-L130 https://github.com/code-423n4/2023-10-nextgen/blob/a6f2397b68ef2865374c1bf7629349f25e71a44d/smart-contracts/AuctionDemo.sol#L132-L143

Vulnerability details

Impact

cancelBid() and cancelAllBids() functions are incorrectly implemented, resulting in partial/complete DoS-ing of bid cancelling functionality.

• bidder could lose funds when they change their mind and want to cancel their bid, imagine they were the top bidder and were going to be rewarded the NFT, and they change their mind, try to cancel but can't.
• for whatever reason it's discovered that the NFT up for auction is fake or problematic or the auction is problematic for some reason, and the auction should be cancelled for valid reasons. This cannot happen, because you will only cancel one bidder's bid, or each bidder will need to call this function individually.
• ability to cancel auctions/auction bids is a critical functionality, which if disabled, could have a serious impact.

Proof of Concept

cancelBid() will only work/be useful if the user/msg.sender knows their index value in the auctionInfoStru[] array, which doesn't seem to be returned to them anywhere when they join an auction. So the user would need to guess.

As far as my understanding goes, cancelAllBids() is meant to cancel all the bids for a specific auction, i.e. to cancel all the bids of bidders for a specific auction and NFT combo. But it actually only cancels the bid for a specific bidder, because it uses msg.sender and then tries to loop through the bidders to cancel ALL bids for this auction... see the logic error?

Tools Used

VSC. Manual.

Recommended Mitigation Steps

The below recommended function implementation for cancelBid() is not fully gas optimized, so take that into account:

// cancel a single Bid
-   function cancelBid(uint256 _tokenid, uint256 index) public { 
+   function cancelBid(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); 
+       uint256 auctionInfoStruLength = auctionInfoData[_tokenid].length;
+       for (uint256 i; i < auctionInfoStruLength;) { 
+           if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true) {
+               auctionInfoData[_tokenid][i].status = false;
+               (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
+               /// @audit add a boolean return value check here! Check my other finding/submission
+               emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
+               return;
            }
+           unchecked {i++;}
        }
-       require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
-       auctionInfoData[_tokenid][index].status = false;
-       (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
-       emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Now the above function should do what it is supposed to do.

To cancel all the bids for a specific auction and NFT combo:

// cancel All Bids
    function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+       uint256 auctionInfoStruLength = auctionInfoData[_tokenid].length;
+       for (uint256 i; i < auctionInfoStruLength;) { 
            if (auctionInfoData[_tokenid][i].status == true) {
                auctionInfoData[_tokenid][i].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
+               /// @audit add a boolean return value check here! Check my other finding/submission
                emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
+           }
+           unchecked {i++;}
        }
    }

Now the above function will do what it's meant to do, cancel all the bids for all the active bidders for this specific auction and NFT combo.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[141345]

expected behavior

[alex-ppg]

The Warden specifies that it might not be possible for a bidder to know the index they have bet on to invoke an atomic cancellation and that the function cancelAllBids will only cancel the bidder's bids rather than all bids of the auction.

As the Sponsor specified, this is deemed to be expected behavior as off-chain tools can aid in identifying the right index to cancel, and the function cancelAllBids will cancel all bids of the caller.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid