There is a chance that tokenHash is not set if using RandomizerVRF

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L236
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189-L200
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L227-L232
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L71-L75
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/RandomizerVRF.sol#L52-L68
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L299-L303
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L170-L174

Vulnerability details

Impact

There is a chance that tokenHash is not set if using RandomizerVRF

Proof of Concept

According to the chainlink docs If your fulfillRandomWords() implementation reverts, the VRF service will not attempt to call it a second time. Make sure your contract logic does not revert. Consider simply storing the randomness and taking more complex follow-on actions in separate contract calls made by you, your users, or an Automation Node.

If the fulfillRandomWords fails there is no way of setting the tokenHash again and will remain 0. This can fail for one of two reasons. Let's take a look

• User mints a token from the MinterContract with a function mint
• This will invoke the internal _mintProcessing which calls the calculateTokenHash on the set randomizer.
  The first option of failure is here. It doesn't check that the randomizer is even set. If it is not set yet and is address(0) any calls to it will succeed because it is an EOA.

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

• If the randomizer is set the execution flow continues and calls calculateTokenHash

function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) public {
    require(msg.sender == gencore);
    tokenIdToCollection[_mintIndex] = _collectionID;
    requestRandomWords(_mintIndex);
}

function requestRandomWords(uint256 tokenid) public {
    require(msg.sender == gencore);
    uint256 requestId =
        COORDINATOR.requestRandomWords(keyHash, s_subscriptionId, requestConfirmations, callbackGasLimit, numWords);
    tokenToRequest[tokenid] = requestId;
    requestToToken[requestId] = tokenid;
}

• Now when we send a requestRandomWords request to chainlink we have to wait for requestConfirmationswhich in this case is 3, for the chainlink to call the fulfillRandomWords. Once it gets called it will set the tokenHash

function fulfillRandomWords(uint256 _requestId, uint256[] memory _randomWords) internal override {
    gencoreContract.setTokenHash(
        tokenIdToCollection[requestToToken[_requestId]],
        requestToToken[_requestId],
        bytes32(abi.encodePacked(_randomWords, requestToToken[_requestId]))
    );
    emit RequestFulfilled(_requestId, _randomWords);
}

• So far so good, but let's take a look at the setTokenHash function inside the NextGenCore

function setTokenHash(uint256 _collectionID, uint256 _mintIndex, bytes32 _hash) external {
    require(msg.sender == collectionAdditionalData[_collectionID].randomizerContract);
    require(tokenToHash[_mintIndex] == 0x0000000000000000000000000000000000000000000000000000000000000000);
    tokenToHash[_mintIndex] = _hash;
}

• It requires that the call is made from the randomizerContract that is set in the collectionAdditionalData.
• This can revert if the randomizerContract for the collection is accidentally changed between the calls requestRandomWords and fulfillRandomWords as it will receive the call from the old randomizerContract and the require statement will fail causing the tokenToHash mapping for token with index _mintIndex to never be set. Maybe someone can intentionally frontrun the change of the randomizerContract to receive a token without a tokenHash set making it more valuable.

Tools Used

VS Code

Recommended Mitigation Steps

The simplest solution would be to implement the unprocessedRequest counter in the Randomizer contract and require that the value is 0 when changing a randomizer. Also, make sure the randomizer is set when requesting random values.

NextGenCore.sol

function _mintProcessing(
    uint256 _mintIndex,
    address _recipient,
    string memory _tokenData,
    uint256 _collectionID,
    uint256 _saltfun_o 
) internal {
    tokenData[_mintIndex] = _tokenData;
+   require(collectionAdditionalData[_collectionID].randomizer != address(0), "randomizer is zero");

    collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
    tokenIdsToCollectionIds[_mintIndex] = _collectionID;

    _safeMint(_recipient, _mintIndex);
}

function addRandomizer(uint256 _collectionID, address _randomizerContract)
    public
    FunctionAdminRequired(this.addRandomizer.selector)
{
+   require(collectionAdditionalData[_collectionID].randomizer.unprocessedRequest() == 0, "Still pending requests");
    require(IRandomizer(_randomizerContract).isRandomizerContract() == true, "Contract is not Randomizer");
    collectionAdditionalData[_collectionID].randomizerContract = _randomizerContract;
    collectionAdditionalData[_collectionID].randomizer = IRandomizer(_randomizerContract);
}

RandomizerVRF.sol

// Storage variable
uint256 public unprocessedRequest;

function requestRandomWords(uint256 tokenid) public {
    require(msg.sender == gencore);
    uint256 requestId =
        COORDINATOR.requestRandomWords(keyHash, s_subscriptionId, requestConfirmations, callbackGasLimit, numWords);
    tokenToRequest[tokenid] = requestId;
    requestToToken[requestId] = tokenid;
    
+   unprocessedRequest++;
}

function fulfillRandomWords(uint256 _requestId, uint256[] memory _randomWords) internal override {
    gencoreContract.setTokenHash(
        tokenIdToCollection[requestToToken[_requestId]],
        requestToToken[_requestId],
        bytes32(abi.encodePacked(_randomWords, requestToToken[_requestId]))
    );
    
+   unprocessedRequest--;
    emit RequestFulfilled(_requestId, _randomWords);
}

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #1464

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid