Lack of fallback in case of failed call to get RNG can result in never setting randomness for a minted tokenId, which breaks critical logic

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

There are a number of different reasons that the call to get a random value for a minted tokenId might fail (using Arrng or Chainlink VRF). For example, during times of high blockchain usage, the actual callback costs for using the Arrng service might jump to being higher than the payment sent in the RandomizerRNG contract (equal to ethRequired, which is only periodically updated by the NextGen admin). When this happens, the RandomizerRNG function will be refunded the cost here. However, it is not possible to re-request a random number for that minted tokenId, which in turn breaks any logic reliant on that random number. This even includes breaking the logic for returning the URI for that token.

Proof of Concept

The logic for requesting randomness for a minted tokenId happens in NextGenCore:_mintProcessing, which is defined as follows:

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

This is done by calling calculateTokenHash on the randomizer contract which has been configured for that collection. Note that there is no way to re-call this function for a minted tokenId, if there are any issues which occur when requesting the RNG.

One such issue that's possible is with the RandomizerRNG contract, where the request for randomness is done in calculateTokenHash -> requestRandomWords, where the requestRandomWords function is defined as follows:

function requestRandomWords(uint256 tokenid, uint256 _ethRequired) public payable {
    require(msg.sender == gencore);
    uint256 requestId = arrngController.requestRandomWords{value: _ethRequired}(1, (address(this)));
    tokenToRequest[tokenid] = requestId;
    requestToToken[requestId] = tokenid;
}

As can be seen, the Arrng service requires that enough callback gas is sent in the request. Here, that amount is _ethRequired, which is equal to ethRequired, which is a state variable that NextGen admin periodically set/update. However, during times of high blockchain congestion, its highly probable that the actual cost to request randomness exceeds this cached value, which will result in the request failing. However, there is no revert (so mint is still successful) - instead, the payment is just refunded here. In turn, this means that randomness will never be set for that tokenId, which then breaks logic reliant on that value being set (including returning the correct token URI).

Tools Used

Manual review

Recommended Mitigation Steps

There should be a fallback function which is only callable by the NextGen admin which can re-request randomness for a tokenId if there are any issues (if adequate amount of time has passed for a minted tokenId, where randomness wasn't set correctly for that token).

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

if it fails due to lack of funds it will fail also when re executed. If the tokenHash has not been set an admin can deploy a contract that can call the RNG for that specific id, the hash will be generated and stored, but this is not the case here. The contracts will be fully funded to cover the costs.

[alex-ppg]

This submission and multiple of its duplicates relate to various nuisances concerning the oracles in NextGen which are all invalid.

For further details, please consult the following submissions that have a detailed rationale attached: #1307, #2035, #1901, #998

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid