NextGen - alexfilippov314's results

Lines of code

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

Vulnerability details

The auctionDemo contract allows to call the cancelBid or the cancelAllBids even after the execution of the claimAuction if both calls are executed in a block with block.timestamp == minter.getAuctionEndTime(_tokenid). To illustrate it let's assume the following scenario:

• Let's assume (for simplicity) that there is only one bidder, he made 2 bids (1 ether and 2 ether).
• He sends two transactions which he hopes will be included in the same block (the order matters) with block.timestamp == minter.getAuctionEndTime(_tokenid):
  • claimAuction - the bidder can call this method since he is the winner and block.timestamp >= minter.getAuctionEndTime(_tokenid)
  • cancelAllBids - the bidder can call this method since block.timestamp <= minter.getAuctionEndTime(_tokenid) and the claimAution doesn't change the status of the bids.
• If he succeeds then he will get:
  • the NFT as the winner of the auction
  • +1 ether as a refund of the first bid in the claimAution
  • +3 ether in the cancelAllBids since all of his bids can be canceled

So basically the bidder gets the NFT for free and additionally steals 1 ether (in this scenario; it is possible to steal everything with some preconditions) from the auction contract.

The attack above is simplified and therefore is not optimal. To increase the probability of success the bidder can deploy some proxy contract that can:

• make bids
• call the claimAuction
• call the cancelAllBids in the receive if all attack conditions are met:
  • block.timestamp == minter.getAuctionEndTime(_tokenid)
  • it is the refund of the last non-winning bid

In this case, the bidder can send only one transaction, and therefore the probability of success is higher.

Since the timestamp of the next block on Ethereum is quite predictable the probability of this attack is relatively high. If the attack doesn't succeed the bidder will still get the NFT and receive all non-winning bids back so the user doesn't risk losing anything.

It is also worth mentioning that an attacker is not obligated to be a winner to perform this attack. If a bidder is not a winner he can still use the aforementioned proxy contract that will help him to call the cancelAllBids if all conditions are met. But since a bidder can't control the claimAuction call time the probability of success is much lower in this case.

Impact

A malicious user is able to steal ether from the auctionDemo contract with a significant probability.

Proof of Concept

-

Tools Used

Manual Review

Recommended Mitigation Steps

Consider replacing the condition block.timestamp >= minter.getAuctionEndTime(_tokenid) with the strict one block.timestamp > minter.getAuctionEndTime(_tokenid) in the auctionDemo.claimAuction.

Assessed type

Timing

Lines of code

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

Vulnerability details

The NextGenCore contract allows a collection admin to change the collectionArtistAddress even after a collection was signed by an artist. To illustrate it let's consider the following scenario:

1. Let's assume (for simplicity) that there is a malicious collection admin who wants to create a fake collection that looks as if it was signed by some famous artist.
2. One of the admins creates a collection by calling the createCollection and registers the collection admin.
3. The collection admin calls the setCollectionData with his own _collectionArtistAddress and the _collectionTotalSupply equal to zero.
4. He sets a fake artist's signature by calling the artistSignature using his own address.
5. Admins call the addRandomizer and all the necessary setup functions in the NextGenMinterContract including setting royalty payout addresses. It is important to note that it is a regular flow and it is not too optimistic to assume that admins might miss that the collectionTotalSupply is zero.
6. Then the collection admin calls the setCollectionData again with the _collectionArtistAddress equal to some famous artist's address and the _collectionTotalSupply equal to a desired value. He is able to set a new artist's address since the collectionTotalSupply is zero.
7. Finally, we have a collection that looks as if it is signed by a famous artist when it is actually not the case. As a result, this collection admin will be able to get proceeds from selling NFTs using a famous artist's name.

Impact

A collection admin can set up a collection that looks as if it is signed by some famous artist when it is actually not. It is a clear violation of one of the main invariants:

• Only artists can sign their collections.

Proof of Concept

context("Impersonate artist", () => {
  it("#createCollection", async function () {
    await contracts.hhCore.createCollection(
      "Test Collection",
      "Artist",
      "For testing",
      "www.test.com",
      "CCO",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["desc"],
    )
  })

  it("#registerCollectionAdmin", async function () {
    await contracts.hhAdmin.registerCollectionAdmin(
      5,
      signers.addr1.address,
      true,
    )
  })

  it("#setCollectionData", async function () {
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      5, // _collectionID
      signers.addr2.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      0, // _collectionTotalSupply
      0, // _setFinalSupplyTimeAfterMint
    )
  })

  it("#artistSignature", async function() {
    await contracts.hhCore.connect(signers.addr2).artistSignature(
      5, // _collectionID
      "Artist Signature", // _artistSignature
    )
  })

  it ("collectionArtistAddress == signers.addr2.address", async function() {
    expect(await contracts.hhCore.retrieveArtistAddress(5)).to.equal(signers.addr2.address);
  })

  it("#setCollectionData again", async function () {
    await contracts.hhCore.connect(signers.addr1).setCollectionData(
      5, // _collectionID
      signers.addr3.address, // _collectionArtistAddress
      2, // _maxCollectionPurchases
      10000, // _collectionTotalSupply
      0, // _setFinalSupplyTimeAfterMint
    )
  })

  it ("collectionArtistAddress == signers.addr3.address", async function() {
    expect(await contracts.hhCore.retrieveArtistAddress(5)).to.equal(signers.addr3.address);
  })
})

Tools Used

Manual Review

Recommended Mitigation Steps

Consider replacing condition collectionAdditionalData[_collectionID].collectionTotalSupply == 0 with wereDataAdded[_collectionID] == false in the NextGenCore.setCollectionData.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

A bidder can call the participateToAuction after the execution of the claimAuction if both transactions are in the block with block.timestamp == minter.getAuctionEndTime(_tokenid). To illustrate it let's consider the following scenario:

1. There is an auction that is close to ending
2. The winner or an admin sends the claimAuction transaction
3. An unlucky bidder sends the participateToAuction transaction
4. Both transactions are included in the same block with block.timestamp == minter.getAuctionEndTime(_tokenid) and the participateToAuction transaction is included after the claimAuction transaction
5. In this case:
   1. the claimAuction will be executed successfully since block.timestamp >= minter.getAuctionEndTime(_tokenid). The winner will get the NFT, all other bids will be refunded.
   2. the participateToAuction will also be executed successfully since block.timestamp <= minter.getAuctionEndTime(_tokenid) and there is no check that the auction was already claimed
6. As a result, the unlucky bidder will lose his bid since:
   1. He skipped a refund already
   2. He can't cancel the bid since the auction ended already
   3. There is no way to get the bid back even with the help of admins or the owner

The probability of such a situation is quite low but it shouldn't be possible at all.

Impact

A participant can accidentally lose his bid in the auction. It is a violation of one of the main invariants:

• The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded.

Proof of Concept

-

Tools Used

Manual Review

Recommended Mitigation Steps

Consider replacing the condition block.timestamp >= minter.getAuctionEndTime(_tokenid) with the strict one block.timestamp > minter.getAuctionEndTime(_tokenid) in the auctionDemo.claimAuction.

Assessed type

Timing

[[1]] Function NextGenMinterContract.mint contains a useless check:

// This condition always holds
require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
// If this condition holds
require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

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

[[2]] Function NextGenMinterContract.burnToMint contains a useless variable:

...
uint256 collectionTokenMintIndex;
collectionTokenMintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);
...
// It has exactly the same value as the collectionTokenMintIndex
uint256 mintIndex = gencore.viewTokensIndexMin(_mintCollectionID) + gencore.viewCirSupply(_mintCollectionID);

There are similar cases in the NextGenMinterContract.mintAndAuction, NextGenMinterContract.burnOrSwapExternalToMint. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L267 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L281 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L362

[[3]] The statement collectionArtistPrimaryAddresses[_collectionID].status = false in the NextGenMinterContract.proposePrimaryAddressesAndPercentages and the NextGenMinterContract.proposeSecondaryAddressesAndPercentages is useless since it is guaranteed to be false.

require (collectionArtistSecondaryAddresses[_collectionID].status == false, "Already approved");
...
collectionArtistSecondaryAddresses[_collectionID].status = false;

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

[[4]] Function fulfillRandomWords in the NextGenRandomizerRNG and NextGenRandomizerVRF contains the construction:

bytes32(abi.encodePacked(numbers,requestToToken[id]))

This construction is equivalent to:

bytes32(numbers[0])

So it is not clear why this construction is used. I guess it was assumed to be:

bytes32(kecak256(abi.encodePacked(numbers,requestToToken[id])))

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerRNG.sol#L49 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L66

[[5]] The word "Watermelon" is never returned in the randomPool.randomWord. The max value of the randomNum is 99. In this case, getWord will return wordsList[99 - 1] that is "Velvet Apple".

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

[[6]] Function NextGenRandomizerNXT.calculateTokenHash looks a bit overcomplicated. In terms of randomness it should be equivalent to:

bytes32 hash = keccak256(abi.encodePacked(_mintIndex, block.prevrandao, blockhash(block.number - 1), block.timestamp);

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

[[7]] There are multiple functions in the NextGenCore (airDropTokens, mint, burnToMint) with similar logic:

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

If the collectionCirculationSupply is equal to collectionTotalSupply - 1 then the collectionCirculationSupply will be increased but nothing will be minted. It is impossible now since this check is present in the minter contract. Still, it looks a bit fragile so it seems that it is better to replace if statement with require statement:

require(msg.sender == minterContract, "Caller is not the Minter Contract");
collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
require(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;
}

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

[[8]] There are a lot of places in the NextGenCore contract where the value 10000000000 is used. Consider introducing a new constant instead.