The RandomizerNXT.sol doesn't provide randomness

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerNXT.sol#L55-L59
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L444-L446
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L192-L193
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L228-L229
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerNXT.sol#L55-L59
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/XRandoms.sol#L35-L38
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/XRandoms.sol#L40-L43

Vulnerability details

Impact

The RandomizerNXT.sol contract doesn't provide a true randomness source. In a context where true randomness is required, this contract is not appropriate to deliver a random hash. The reason is because the minting and hash generation happen all within one transaction and if not satisfactory to the odds abuser could be reverted.

The mint() function is here called,

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

        ...

        if (collectionAdditionalData[_collectionID].collectionTotalSupply >= 
        collectionAdditionalData[_collectionID].collectionCirculationSupply) {
@>         _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);

            ...
    }

as we can see the mint function calls the _mintProcessing()

    function _mintProcessing(uint256 _mintIndex, address _recipient, string memory _tokenData, uint256 _collectionID, uint256 _saltfun_o) internal {
      
       ...

 @>       collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
       ...
    }

The randomizer contract for the given collection is thereafter called...

    function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) public {
        require(msg.sender == gencore);
@>       bytes32 hash = keccak256(abi.encodePacked(_mintIndex, blockhash(block.number - 1), randoms.randomNumber(), randoms.randomWord()));
        gencoreContract.setTokenHash(_collectionID, _mintIndex, hash);
    }

The hash can lastly be checked in the core contract by anyone in the retrieveTokenHash().

This is where the 'magic' happens. A random hash is calculated, through the combination of hashed block timestamps, block hashes and a string, calculation made in the XRandom contract randomWord() and randomNumber() functions.
Although it is true that it is hard to predict this number, this hash is calculated within one single transaction, an exploiter could easily revert, after not having a result that is not satisfactory. This means that this randomness source is not random, because it allows the for the executor to be selective.

As for the impact we could rank this issue as med, depending on how much an NFT collection relies on the randomness of this contract. It seems as if the protocol wanted to pursue randomness because it tries to be unpredictable, one could say that it is quite unpredicatble. however the minter has the last say in the hash that the nft will have.

Proof of Concept

Here is an example of a contract that could revert upon minting an NFT that doesn't have a suitable hash:

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.8.0) (utils/Strings.sol)

pragma solidity ^0.8.0;

interface IGenCore {
    function viewCirSupply(uint256 _collectionID) external view returns (uint256);
    function retrieveTokenHash(uint256 _tokenid) external returns(bytes32);
}

interface IMinter {
     function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) external payable;
}
contract RiggedRandomness {
    IGenCore core;
    IMinter minter;
    constructor(address _core) {
        core = IGenCore(_core);
    }

    function randomnessRigger(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o, bytes32 desiredOutcome) public {
        minter.mint(_collectionID, _numberOfTokens, _maxAllowance, _tokenData, _mintTo, merkleProof, _delegator, _saltfun_o);
        uint256 tokenID = core.viewCirSupply(_collectionID);
        bytes32 tokenHash = core.retrieveTokenHash(tokenID);
        //insert logic here :
        
        require(tokenHash == desiredOutcome, "not the desired hash");

    }
}

As we can see this implementation allows the exploiter to discard the nft if the hash is not favorable.

Tools Used

manual review

Recommended Mitigation Steps

For optimal randomness sources I would stick to the randomness that is created through RNG and VRF contracts.
If the protocol wants to impliment alternative randomness, one way would be to allow for the hash to be visible in the retrieveTokenHash() function only after the minting period is over. Here is a possible fix:

    function retrieveTokenHash(uint256 _tokenid, uint256 col) public view returns(bytes32){
        require(block.timestamp >  minter.collectionPhases[col].publicEndTime, "minting not finished";
        return (tokenToHash[_tokenid]);
    }


## Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #163

[c4-judge]

alex-ppg marked the issue as duplicate of #1901

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid