If ERC721 safeTransferFrom fails in claimAuction, the ether collected by the Auction contract will not be refunded to the bidders

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

There are situations that ERC721's safeTransferFrom will fail and upon reviewing claimAuction function, ether stored inside the Auction contract will be affected.
As you can see below, once the auction time expires, and the highest bidder is selected, the auctioned nft will be transferred from the owner to the highest bidder address, and then the ether collected by the Auction contract is supposed to be forwarded to the Auction contract owner for bid winner's ether and for those loser bidders, their ether will be refunded to them.

// claim Token After Auction

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
    auctionClaim[_tokenid] = true;
    uint256 highestBid = returnHighestBid(_tokenid);
    address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
    address highestBidder = returnHighestBidder(_tokenid);
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

However, if the safeTransferFrom of nft will fail, claimAuction will revert and the collected ether from the bidders including the winner itself, will got stuck inside the auction contract.

Proof of Concept

Before testing the POC, here is the secret gist link for the setup of foundry test https://gist.github.com/bluenights004/1c5e62a27d050bff4d70d337a4ff974c

Here is the coded POC to show the situation wherein the safeTransferFrom fails because the recipient contract is not capable of receiving nft. In this test, the claimAuction will revert as a result and the balances of ether will be stuck inside the contract. No ether distribution is made to the bidders upon revert.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "../../lib/forge-std/src/Test.sol";
import "../../lib/forge-std/src/Vm.sol";
import "../../lib/forge-std/src/console.sol";
import "../../smart-contracts/NFTdelegation.sol";
import "../../smart-contracts/NextGenAdmins.sol";
import "../../smart-contracts/NextGenCore.sol";
import "../mock/MockMinter.sol"; 
import "../mock/MockERC721.sol"; 
import "../mock/WinnerContract.sol"; 
import "../../smart-contracts/AuctionDemo.sol";
import  "../../smart-contracts/Ownable.sol";  


contract AuctionTest is Test {

    DelegationManagementContract hhDelegation;
    NextGenAdmins hhAdmin;
    NextGenCore hhCore;
    MockMinterContract hhMinter;
    MockERC721 mockERC721;
    auctionDemo auctionContract;
    WinnerContract winnerContract;

    address testTokenOwner;
    address highestBidder;
    address auctionContractOwner;
    
   
    address bidder1 = address(3);
    address bidder2 = address(4);

    uint256 testTokenId = 123;

    function setUp() public {   

        //Deploy mock contracts
        hhDelegation = new DelegationManagementContract();
        hhAdmin = new NextGenAdmins();
        hhCore = new NextGenCore("Next Gen Core", "NEXTGEN", address(hhAdmin));
        hhMinter = new MockMinterContract();
        mockERC721 = new MockERC721("token123", "NFT");

        //Deploy the winning contract but this contract can't accept NFT
        winnerContract = new WinnerContract();

        // Deploy auctionDemo contract with mock addresses
        auctionContract = new auctionDemo(address(hhMinter), address(mockERC721), address(hhAdmin));

        // Setup Auction contract owner address
        auctionContractOwner = auctionContract.owner();

       // Setup test accounts
        testTokenOwner = address(1);
        highestBidder = address(2);
      
        // Fund the bidder accounts
        vm.deal(bidder1, 10 ether);
        vm.deal(bidder2, 10 ether);
        vm.deal(address(winnerContract), 10 ether);
        
       // Setup mock behaviors
        mockERC721.mint(testTokenOwner, testTokenId);  // Mint a token to the testTokenOwner
        hhMinter.setAuctionEndTime(testTokenId, block.timestamp + 1 days);
        hhMinter.setAuctionStatus(testTokenId, true);

        // Simulate testTokenOwner (address(1)) calling the approve function
        vm.prank(testTokenOwner);
        mockERC721.approve(address(auctionContract), testTokenId);
    }
// Fallback function to accept Ether. This is necessary because this testing contract serves as owner of Auction contract.
    // Why owner? because it deployed the auction contract.
    // Owner of auction contract will be the recipient of collected eth from winner bid.   
    receive() external payable {}

   
 function testClaimAuctionRevert() public {
        
         // First bidder participates
        vm.prank(bidder1);
        auctionContract.participateToAuction{value: 1 ether}(testTokenId);

        // Second bidder participates
        vm.prank(bidder2);
        auctionContract.participateToAuction{value: 2 ether}(testTokenId);

        // Third bidder (eventual winner) participates
        vm.prank(address(winnerContract));
        auctionContract.participateToAuction{value: 3 ether}(testTokenId);

        // Fast forward time to simulate auction end
        vm.warp(block.timestamp + 2 days);

        // check the eth bal before winner claim
        console.log("Eth Balance of Auction Contract (before claim)        :", address(auctionContract).balance);
        console.log("Eth Balance of Auction Contract Owner (before claim)  :", auctionContractOwner.balance);
        console.log("Eth Balance of Bidder1 (before claim)                 :", bidder1.balance);
        console.log("Eth Balance of Bidder2 (before claim)                 :", bidder2.balance);
        console.log("Eth Balance of mockWinner (before claim)              :", address(winnerContract).balance);
        console.log("Owner of NFT (before claim))                          :", mockERC721.ownerOf(testTokenId));

        // The highest bidder (winnerContract in this case) claims the auction
        vm.prank(address(winnerContract));
        vm.expectRevert();
        auctionContract.claimAuction(testTokenId);// this will revert because winnerContract can't receive NFT

        // check the eth bal after winner claim
        console.log("Eth Balance of Auction Contract (after claim)         :", address(auctionContract).balance);
        console.log("Eth Balance of Auction Contract Owner (after claim)   :", auctionContractOwner.balance);
        console.log("Eth Balance of Bidder1 (after claim)                  :", bidder1.balance);
        console.log("Eth Balance of Bidder2 (after claim)                  :", bidder2.balance);
        console.log("Eth Balance of mockWinner (after claim)               :", address(winnerContract).balance);
        console.log("Owner of NFT (after claim))                           :", mockERC721.ownerOf(testTokenId));

        // Check the NFT ownership has not been transferred to winner contract
       assertTrue(mockERC721.ownerOf(testTokenId) != address(winnerContract), "Owner should not be the winner contract");

       // Check the NFT ownership remains to testTokenOwner
        assertEq(mockERC721.ownerOf(testTokenId), testTokenOwner);
 
        // Check that the auction is marked as not claimed meaning false because of failed claim auction
        assertFalse (auctionContract.auctionClaim(testTokenId));

        // Assertions to check if the non-winning bidders don't received their refunds. Balance remains as before auction claim
        assertEq((bidder1.balance), 9 ether, "Bidder1 should have 9 ether");
        assertEq((bidder2.balance), 8 ether, "Bidder2 should have 8 ether");

    }
}

Result of test:
No changes between the balances of before claim and after claim, as well as nft ownership.

Running 1 test for test/foundry/AuctionTest.t.sol:AuctionTest
[PASS] testClaimAuctionRevert() (gas: 429064)
Logs:
  Eth Balance of Auction Contract (before claim)        : 6000000000000000000
  Eth Balance of Auction Contract Owner (before claim)  : 79228162514264337593543950335
  Eth Balance of Bidder1 (before claim)                 : 9000000000000000000
  Eth Balance of Bidder2 (before claim)                 : 8000000000000000000
  Eth Balance of mockWinner (before claim)              : 7000000000000000000
  Owner of NFT (before claim))                          : 0x0000000000000000000000000000000000000001
  Eth Balance of Auction Contract (after claim)         : 6000000000000000000
  Eth Balance of Auction Contract Owner (after claim)   : 79228162514264337593543950335
  Eth Balance of Bidder1 (after claim)                  : 9000000000000000000
  Eth Balance of Bidder2 (after claim)                  : 8000000000000000000
  Eth Balance of mockWinner (after claim)               : 7000000000000000000
  Owner of NFT (after claim))                           : 0x0000000000000000000000000000000000000001

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 13.70ms

Tools Used

Manual , Foundry

Recommended Mitigation Steps

Create emergency withdraw function so the admin can recover the ether funds and distribute it to the bidders.

Assessed type

ERC721

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #364

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[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 partial-50