tokenHash could be fail to update for minted tokenId

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

There is way in which the generator will not produce a hash value, besides the lack of funds on the VRF. MinterContract

MinterContract::mint() call genCore contract mint function, which internally calls _mintProcessing function.

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

As it can be seen, before minting the token, it perform calculateTokenHash call on the randomizer contract.

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

The function validates caller, followed by requesting chainlink requestRandomWords. And after minimum block confirmation(currently set to 3) coordinator call fullfillRandomWords, which sets tokenHash for the minted tokenId above.

The issue here is, the presence of addRandomizer() function on gencore contract. This function allows owner to update randomizer contract when it required, means if owner unaware of unset tokenHash(will set by fullfillRandomWords call) for the tokens minted in past block then any future fullfillRandomWords call for those tokenId will revert. Because only newRandomizer contract can call the setTokenHash,

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

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual review

Recommended Mitigation Steps

Remove the addRandomizer function

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[alex-ppg]

The Warden specifies that it is possible for a "race condition" to manifest whereby a randomizer request is pending fulfillment when the randomizer of the collection the request is for has been updated.

This is indeed a correct observation, however, the Sponsor has specified that the randomizer can simply "re-fulfil" the "empty" token hashes. As such, I consider this to be a QA issue that arises from the misconfiguration of the collection + can be rectified by the person responsible for the misconfiguration.

The Sponsor should consider a "graceful" upgrade system whereby the randomizer can be first paused and then upgraded to prevent this behavior from ever arising.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c