Bias in random word generation in XRandoms contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/XRandoms.sol#L40-L43
https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/XRandoms.sol#L15-L33

Vulnerability details

Impact

The randomWord function in the XRandoms contract exhibits a bias in generating random words. Specifically, it never returns "Watermelon" and doubles the probability of generating "Acai," undermining the function's randomness and fairness.

Proof of Concept

The randomWord function generates a number (randomNum) between 0 and 99, which is then used by the getWord function to return a corresponding word. However, due to a logic error in getWord, the range of returned words is only from index 0 to 98:

    function randomWord() public view returns (string memory) {
        uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 100;
        return getWord(randomNum);
    }

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/XRandoms.sol#L40-L43

The getWord function incorrectly handles this range, causing it to return "Acai" for both 0 and 1, and never return "Watermelon" (which should correspond to 99):

    function getWord(uint256 id) private pure returns (string memory) {
        
        // array storing the words list
        string[100] memory wordsList = ["Acai", "Ackee", "Apple", "Apricot", "Avocado", "Babaco", "Banana", "Bilberry", "Blackberry", "Blackcurrant", "Blood Orange", 
        "Blueberry", "Boysenberry", "Breadfruit", "Brush Cherry", "Canary Melon", "Cantaloupe", "Carambola", "Casaba Melon", "Cherimoya", "Cherry", "Clementine", 
        "Cloudberry", "Coconut", "Cranberry", "Crenshaw Melon", "Cucumber", "Currant", "Curry Berry", "Custard Apple", "Damson Plum", "Date", "Dragonfruit", "Durian", 
        "Eggplant", "Elderberry", "Feijoa", "Finger Lime", "Fig", "Gooseberry", "Grapes", "Grapefruit", "Guava", "Honeydew Melon", "Huckleberry", "Italian Prune Plum", 
        "Jackfruit", "Java Plum", "Jujube", "Kaffir Lime", "Kiwi", "Kumquat", "Lemon", "Lime", "Loganberry", "Longan", "Loquat", "Lychee", "Mammee", "Mandarin", "Mango", 
        "Mangosteen", "Mulberry", "Nance", "Nectarine", "Noni", "Olive", "Orange", "Papaya", "Passion fruit", "Pawpaw", "Peach", "Pear", "Persimmon", "Pineapple", 
        "Plantain", "Plum", "Pomegranate", "Pomelo", "Prickly Pear", "Pulasan", "Quine", "Rambutan", "Raspberries", "Rhubarb", "Rose Apple", "Sapodilla", "Satsuma", 
        "Soursop", "Star Apple", "Star Fruit", "Strawberry", "Sugar Apple", "Tamarillo", "Tamarind", "Tangelo", "Tangerine", "Ugli", "Velvet Apple", "Watermelon"];

        // returns a word based on index
        if (id==0) {
            return wordsList[id];
        } else {
            return wordsList[id - 1];
        }
        }

https://github.com/code-423n4/2023-10-nextgen/blob/58090c9fbc036c06bbaa9600ec326034f2181a17/hardhat/smart-contracts/XRandoms.sol#L15-L33

The function becomes not random since Watermelon has probability of 0, Acai has probability of 2, and other words have probability of 1.

• Save the code in test/nextGen.test.sol
• Setup foundry and Run: forge test -vvvvv --match-contract NextGenCoreTest --match-test testGetWord

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "../smart-contracts/NextGenCore.sol";
import "../smart-contracts/NextGenAdmins.sol";
import "../smart-contracts/NFTdelegation.sol";
import "../smart-contracts/XRandoms.sol";
import "../smart-contracts/RandomizerNXT.sol";
import "../smart-contracts/MinterContract.sol";
import "../smart-contracts/AuctionDemo.sol";
import {console} from "forge-std/console.sol";

contract NextGenCoreTest is Test {
    NextGenCore hhCore;
    DelegationManagementContract hhDelegation;
    randomPool hhRandoms;
    NextGenAdmins hhAdmin;
    NextGenRandomizerNXT hhRandomizer;
    NextGenMinterContract hhMinter;
    auctionDemo hhAuctionDemo;

    address owner;
    address addr1;
    address addr2;
    address addr3;

    function setUp() public {
        owner = address(this);
        addr1 = vm.addr(1);
        addr2 = vm.addr(2);
        addr3 = vm.addr(3);

        // Deploy contracts
        hhDelegation = new DelegationManagementContract();
        hhRandoms = new randomPool();
        hhAdmin = new NextGenAdmins();
        hhCore = new NextGenCore("Next Gen Core", "NEXTGEN", address(hhAdmin));
        hhRandomizer = new NextGenRandomizerNXT(
            address(hhRandoms),
            address(hhAdmin),
            address(hhCore)
        );
        hhMinter = new NextGenMinterContract(
            address(hhCore),
            address(hhDelegation),
            address(hhAdmin)
        );
    }

    function testGetWord() public {
        string memory word0 = hhRandoms.returnIndex(0);
        string memory word1 = hhRandoms.returnIndex(1);
        string memory word99 = hhRandoms.returnIndex(99);
        console.log(word0); // return Acai
        console.log(word1); // return Acai
        console.log(word99); // return Velvet Apple, not Watermelon
    }
}

Tools Used

Manual

Recommended Mitigation Steps

To resolve this bias, the getWord function should be modified to directly return the word corresponding to the generated randomNum, ensuring each word has an equal probability of being selected:

    function getWord(uint256 id) private pure returns (string memory) {
        
        // array storing the words list
        string[100] memory wordsList = ["Acai", "Ackee", "Apple", "Apricot", "Avocado", "Babaco", "Banana", "Bilberry", "Blackberry", "Blackcurrant", "Blood Orange", 
        "Blueberry", "Boysenberry", "Breadfruit", "Brush Cherry", "Canary Melon", "Cantaloupe", "Carambola", "Casaba Melon", "Cherimoya", "Cherry", "Clementine", 
        "Cloudberry", "Coconut", "Cranberry", "Crenshaw Melon", "Cucumber", "Currant", "Curry Berry", "Custard Apple", "Damson Plum", "Date", "Dragonfruit", "Durian", 
        "Eggplant", "Elderberry", "Feijoa", "Finger Lime", "Fig", "Gooseberry", "Grapes", "Grapefruit", "Guava", "Honeydew Melon", "Huckleberry", "Italian Prune Plum", 
        "Jackfruit", "Java Plum", "Jujube", "Kaffir Lime", "Kiwi", "Kumquat", "Lemon", "Lime", "Loganberry", "Longan", "Loquat", "Lychee", "Mammee", "Mandarin", "Mango", 
        "Mangosteen", "Mulberry", "Nance", "Nectarine", "Noni", "Olive", "Orange", "Papaya", "Passion fruit", "Pawpaw", "Peach", "Pear", "Persimmon", "Pineapple", 
        "Plantain", "Plum", "Pomegranate", "Pomelo", "Prickly Pear", "Pulasan", "Quine", "Rambutan", "Raspberries", "Rhubarb", "Rose Apple", "Sapodilla", "Satsuma", 
        "Soursop", "Star Apple", "Star Fruit", "Strawberry", "Sugar Apple", "Tamarillo", "Tamarind", "Tangelo", "Tangerine", "Ugli", "Velvet Apple", "Watermelon"];

        // returns a word based on index
-        if (id==0) {
-            return wordsList[id];
-        } else {
-            return wordsList[id - 1];
-        }
-        }
+       return wordsList[id];

Assessed type

Error

[c4-pre-sort]

141345 marked the issue as duplicate of #508

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-c

[thangtranth]

hi @alex-ppg ,

This issue points to the same vulnerability as #1008. It has full description and POC. However, it was marked as unsatisfactory but the #1008 is marked as grade-b.

Can you please review the grading?

[alex-ppg]

Hey @thangtranth, thanks for flagging this! This submission was marked as C in grade, not an unsatisfactory duplicate of #1008. In general, a QA downgraded issue will be awarded a single B grade in its duplicate set and solely if the submission is of merit. As such, the B grade has already been awarded to #1008 between its numerous duplicates.