Randomizer RNG return value 0 will revert

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/RandomizerRNG.sol#L48-L50
https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/NextGenCore.sol#L299-L303

Vulnerability details

Impact

The fulfillRandomWords call will revert and the NFT random hash cannot be set. The NFT will be corrupted.

Proof of Concept

When generating the random hash using Arrng, after getting the numbers, it packs the numbers with tokenID and cast to bytes32 to get the random hash. This casting truncates the packed data to its first 256 bits.

    bytes32(abi.encodePacked(numbers,requestToToken[id]))

File: smart-contracts/RandomizerRNG.sol
48:     function fulfillRandomWords(uint256 id, uint256[] memory numbers) internal override {
49:         gencoreContract.setTokenHash(tokenIdToCollection[requestToToken[id]], requestToToken[id], bytes32(abi.encodePacked(numbers,requestToToken[id])));
50:     }

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/RandomizerRNG.sol#L48-L50

If the returned random number is 0, it means that the packed bytes will be 0x0000000000000000000000000000000000000000000000000000000000000000 despite of the token id.

Chisel output

    chisel
    Welcome to Chisel! Type `!help` to show available commands.
    ➜ uint256[] memory numbers = new uint256[](1);
    ➜ numbers[0] = 0;
    ➜ uint256 id = 10000000000;
    ➜ bytes32 c = bytes32(abi.encodePacked(numbers, id));
    ➜ c
    Type: bytes32
    └ Data: 0x0000000000000000000000000000000000000000000000000000000000000000
    ➜

However, if the _hash is 0x0000000000000000000000000000000000000000000000000000000000000000 then the gencoreContract.setTokenHash call will revert. The tokenToHash is not set.

File: smart-contracts/NextGenCore.sol
299:     function setTokenHash(uint256 _collectionID, uint256 _mintIndex, bytes32 _hash) external {
300:         require(msg.sender == collectionAdditionalData[_collectionID].randomizerContract);
301:         require(tokenToHash[_mintIndex] == 0x0000000000000000000000000000000000000000000000000000000000000000);
302:         tokenToHash[_mintIndex] = _hash;
303:     }

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/NextGenCore.sol#L299-L303

The same issue can happen to RandomizerVRF despite the probability of returning 0 is very low.

Tools Used

Manual

Recommended Mitigation Steps

To prevent the potential for a zero hash, modify the hash generation in RandomizerRNG to use keccak256 instead of abi.encodePacked

    function fulfillRandomWords(uint256 id, uint256[] memory numbers) internal override {
-        gencoreContract.setTokenHash(tokenIdToCollection[requestToToken[id]], requestToToken[id], bytes32(abi.encodePacked(numbers,requestToToken[id])));
+        gencoreContract.setTokenHash(tokenIdToCollection[requestToToken[id]], requestToToken[id], keccak256(abi.encodePacked(numbers,requestToToken[id])));
    }

Assessed type

en/de-code

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #852

[c4-judge]

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

[c4-judge]

This previously downgraded issue has been upgraded by alex-ppg

[c4-judge]

alex-ppg marked the issue as duplicate of #1688

[c4-judge]

alex-ppg marked the issue as partial-50