Predictable randomness allows claiming of fighters with arbitrary element and weight

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L214 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L254 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L379 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L324

Vulnerability details

The protocol's ERC721 fighters consist of a variety of attributes. This includes base attributes (element, weight, and dna), as well as physical attributes for each of the fighters 6 body parts (head, eyes, mouth, etc.)

Users should not be able to pre-compute inputs that allow them to claim fighters with the attributes they desire.

The current process of generating dna which directly impacts fighter attributes is based on a set formulas, which are easily viewable in FighterFarm.sol, the most common one for calculating dna is seen below.

    uint256(keccak256(abi.encode(msg.sender, fighters.length)))

It should be noted the FighterFarm::reRoll uses a slight variation of the common dna formula but this doesn't increase the difficulty of manipulation.

Since the common formula for dna has only 2 inputs, one of which (msg.sender) is under control of the user, it becomes trivially easy to solve this formula for the length of the fighters at which the user should call claimFighter to mint a fighter with their desired attributes. A function doing such pre-computation for a specified address is included below.

    // DNA is currently computed by combining predictable values 
    // This function demonstrates calculation of dna to give max weight in claimFighters
    function testComputeClaimDNA() public view {
        // should be the address we intend to use to call `claimFighters` 
        // simply change it if the optimal fighter is too far in the future for the given address.
        address adjustableSender = address(0x90193C961A926261B756D1E5bb255e67ff9498A1);

        for (uint8 i = 1; i < 100; i++) {
            bytes memory testDNA = abi.encode(adjustableSender, i);

            uint256 precompClaimDNA = uint256(keccak256(testDNA));

            if(precompClaimDNA % 31 == 30){
                console.log("Mint fighter when fighters.length is:", i);
            }
        }
    }

Logs result:

  Mint fighter when fighters.length is:  2
  Mint fighter when fighters.length is:  16
  Mint fighter when fighters.length is:  52

The reason a successful dna string is considered to be one in which dna % 31 == 30 is because this is the way to ensure max weight when claimFighter reaches the FighterFarm::_createFighterBase step. This example focused on achieving max weight but solving for various weights or any specific element is also possible.

    /// @notice Creates the base attributes for the fighter.
    /// @param dna The dna of the fighter.
    /// @param fighterType The type of the fighter.
    /// @return Attributes of the new fighter: element, weight, and dna.
    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Impact

This vulnerability impacts all public / external functions which can result in the minting of a fighter these being:

• FighterFarm::claimFighters - mint pass holder can calculate fighters.length value at which they should claim for desired attributes

• FighterFarm::redeemMintPass - mint pass holder can calculate values that results in their desired attributes without having to wait for a specified fighters.length to mint the fighter. Some examples that yield max weight when passed as dna argument are 23, 47, 60, 70, 83 ...etc.

• FighterFarm::reRoll - fighter owners can calculate what the result of calling reRoll for their fighter will be before spending the NRN to actually perform the action. Which leads to loss of protocol revenue as some users will not reRoll if they know they will receive a fighter with undesirable attributes.

• FighterFarm::mintFromMergingPool - not directly impacted because it only allows the officially deployed MergingPool to call it

Proof of Concept

    function testClaimMaxWeightFighter() public {
        // have the system mint 2 fighters since this the length we have pre-computed will result
        // in maxWeight for the msg.sender address to be used for demonstration
        _mintFromMergingPool(_ownerAddress); //mint tokenId 0
        _mintFromMergingPool(_ownerAddress); // mint tokenId 1

        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](1);
        claimModelTypes[0] = "original";

        vm.prank(address(0x90193C961A926261B756D1E5bb255e67ff9498A1));
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);

        // Get weight of newly minted fighter (tokenId 2)
        (,,uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(2);

        // verify max weight achieved
        assertEq(weight, 95);
    }

Test setup

• clone https://github.com/code-423n4/2024-02-ai-arena
• Add the test above to protocol's existing test suite (specifically in the FighterFarm.t.sol file)
• Execute: forge test --mt testClaimMaxWeightFighter

Tools Used

• Manual review
• Foundry

Recommended Mitigation Steps

Due to the importance of randomness for the integrity of a fair game the current "randomness" should be replaced with a secure and verifiable source of randomness from Chainlink VRF which is currently available on Arbitrum.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #53

[c4-judge]

HickupHH3 marked the issue as satisfactory

[c4-judge]

HickupHH3 changed the severity to 2 (Med Risk)

[c4-judge]

HickupHH3 marked the issue as duplicate of #376