search

sherlock-audit / 2024-02-radicalxchange-judging

3 stars 1 forks source link

jasonxiale - malicious user can steal his collateral back after he wins the auction #27

Closed sherlock-admin2 closed 8 months ago

sherlock-admin2 commented 8 months ago

jasonxiale

high

malicious user can steal his collateral back after he wins the auction

Summary

In current implementation, a bidder can cancel and withdraw his collateral if he is not the highest bidder. And if the user is the highest bidder, his collateral can't be withdrawn and will be transferred to privious owner when EnglishPeriodicAuctionFacet.closeAuction is called. In EnglishPeriodicAuctionInternal._cancelBid, there is a check to make sure the bidder isn't the highest bidder. But in EnglishPeriodicAuctionInternal._cancelAllBids, the function lacks of such check, without the check, a malicious user can steal his collateral after he wins the auction.

Vulnerability Detail

In EnglishPeriodicAuctionInternal._cancelBid, there is a check to make sure the bidder is not the highest bidder in the aution in EnglishPeriodicAuctionInternal.sol#L393-L396

But in EnglishPeriodicAuctionInternal._cancelAllBids, the function lacks of such check.

416     function _cancelAllBids(uint256 tokenId, address bidder) internal {
417         EnglishPeriodicAuctionStorage.Layout
418             storage l = EnglishPeriodicAuctionStorage.layout();
419
420         uint256 currentAuctionRound = l.currentAuctionRound[tokenId];
421
422         for (uint256 i = 0; i <= currentAuctionRound; i++) {
423             Bid storage bid = l.bids[tokenId][i][bidder]; /// <<<--- function doesn't check if the bidder is the highest bidder here.
424
425             if (bid.collateralAmount > 0) {
426                 // Make collateral available to withdraw
427                 l.availableCollateral[bidder] += bid.collateralAmount;
428
429                 // Reset collateral and bid
430                 bid.collateralAmount = 0;
431                 bid.bidAmount = 0;
432             }
433         }
434     }

attack step:

  1. Alice(the malicious user) calls EnglishPeriodicAuctionFacet.placeBid with enough large amount of collateral to win the auction
  2. When the auction ends, Alice calls EnglishPeriodicAuctionFacet.cancelAllBidsAndWithdrawCollateral immediately after the auction ends, before EnglishPeriodicAuctionFacet.closeAuction is called.

Impact

Without the check, malicious user can steal his collateral back after he wins the auction

Code Snippet

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

Tool used

Manual Review

Recommendation

diff --git a/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol b/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol
index daeef9f..f1b0ade 100644
--- a/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol
+++ b/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol
@@ -420,6 +420,11 @@ abstract contract EnglishPeriodicAuctionInternal is
         uint256 currentAuctionRound = l.currentAuctionRound[tokenId];

         for (uint256 i = 0; i <= currentAuctionRound; i++) {
+
+            if(bidder == l.highestBids[tokenId][round].bidder) {
+                continue; // maybe we can skip here
+            }
+
             Bid storage bid = l.bids[tokenId][i][bidder];

             if (bid.collateralAmount > 0) {

Duplicate of #14