Auction winner can claim their NFT and cancel their bid

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L134

Vulnerability details

Impact

In the AuctionDemo contract the winner of the auction can claim their NFT and cancel their bid. This means that they can get the NFT for free.

This is done in the following steps, at the end of the auction(block.timestamp == minter.getAuctionEndTime(_tokenid)):

1. Attacker places a bid
2. Attacker calls claimAuction() to claim their NFT
3. Attacker calls cancelBid() or cancelAllBids() to cancel their bid

This attack does require the amount of ETH in the contract to be greater than or equal to the winning bid(before the attacker places the bid). This should easiely be possible with other auctions happening.

It also has the effect of there not being enough ETH left for the other auctions meaning that some of the bidders will not recieve their refund and the owner may not recieve the winning bid.

Proof of Concept

In this POC we assume there are other auctions happening and that the sum of their bids is greater than 1 ETH
This ensures there is enough balance for the attacker to cancel their bid

// advance to the end of the auction(block.timestamp == minter.getAuctionEndTime(_tokenid))
vm.warp(block.timestamp + 1000);

// attacker places a bid
vm.startPrank(attacker);
auction.participateToAuction{value: 1 ether}(tokenIndex);

// attacker claims their nft
auction.claimAuction(tokenIndex);

// attacker cancels the bid
auction.cancelBid(tokenIndex, 0);

vm.stopPrank();

Tools Used

Foundry

Recommended Mitigation Steps

Update claimAuction() so that it can only be called after the end of the auction and not at the end.

This ensures that claimAuction() and cancelBid()/cancelAllBids() are not able to be called in the same transaction

-    require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

+    require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1788

[c4-judge]

alex-ppg marked the issue as satisfactory