AuctionDemo.sol - Winner can get NFT for free

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The logic of Auction can be summarized by describing 3 functions:

• participateToAuction() - user bids.
• cancelBid()/cancelAllBids() - user cancels his bid(s).
• claimAuction() - when auction ends, winner receives NFT.

The vulnerability in above implementation is caused by improper definition of Auction duration.

Winner of the auction can call claimAuction() to receive the NFT, and then, immediately call cancelBid()/cancelAllBids() to get the refund of their bid. This will result in getting NFT for free.

Proof of Concept

File: AuctionDemo.sol

 function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

File: AuctionDemo.sol

    function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

File: AuctionDemo.sol

 function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

• cancelBid() and cancelAllBids() can be called only when block.timestamp <= minter.getAuctionEndTime(_tokenid)
• claimAuction() can be called only when block.timestamp >= minter.getAuctionEndTime(_tokenid)

This implies, that both cancelBid(), cancelAllBids(), claimAuction() can be called when block.timestamp == minter.getAuctionEndTime(_tokenid)

1. User wins auction and calls claimAuction() and cancelBid() (or cancelAllBids()) in the same block where block.timestamp == minter.getAuctionEndTime(_tokenid).
2. claimAuction() executes, it transfers NFT to the user. Please notice that claimAuction() does not change auctionInfoData[_tokenid][i].status. It's set to true even after the NFT claim.
3. Now, in the same block cancelBid() (or cancelAllBids()) executes. Since block.timestamp == minter.getAuctionEndTime(_tokenid) and status is still true, bid is being cancelled and refunded:

File: AuctionDemo.sol

function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        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);
    }

Winner called claimAuction() and cancelBid() in the same block, where block.timestamp == minter.getAuctionEndTime(_tokenid). This allows him to get NTF (claimAuction()) and get the refund (cancelBid()) afterwards.

Tools Used

Manual code review

Recommended Mitigation Steps

There are multiple of ways to resolve this issue.

• (1) claimAuction() should be called when block.timestamp > minter.getAuctionEndTime(_tokenid)
  Please notice that when we change >= to >, it won't be possible to call claimAuction() and cancelBid()/cancelAllBids() during the same block.timestamp.

• (2) claimAuction() should change the status after the NFT transfer
  When NFT is transferred, auctionInfoData[_tokenid][i].status should be changed to false.

That way, it won't be possible to call cancelBid() or cancelAllBids() - since those functions require auctionInfoData[_tokenid][i].status to be true.
Please make sure to set this status before performing IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); to avoid any risk of reentrancy.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #1172

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as satisfactory