When a winner calls `MergingPool::claimRewards` there are no limitations on their custom attributes

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L139

Vulnerability details

Description: Winners from the MergingPool contract are allowed to specify their own modelURIs, modelTypes and customAttributes when calling claimRewards. However there is no validation that the numbers given to customAttributes are valid. The custom attributes correspond to the fighters element and weight. The constructor to FighterFarm sets the number of elements for generation zero as 3 here:numElements[0] = 3. However this allows the user to set their element to any number in the uint256 range. As for weight the weights generated normally by the code in FighterFarm::_createFighterBase range from 65 to 95 as shown here: uint256 weight = dna % 31 + 65.

Impact: As these attributes are to effect the gameplay of the project which is not onchain and therefore outside the scope of this contest it's unclear the extent of the problems this issue could cause. However it's likely safe to assume that this would result either: A - Cause a large distruption to the intended gameplay. B - Have to be retconned off chain before gameplay takes place, leading the onchain record of fighter attributes to be an incorrect record of their actual attributes in game.

Proof of Concept: Add the following test to MergingPool.t.sol to confirm this:

    function testWinnerCanPickRareStats() public {
        // Set up two owners to meet required number of winners
        address alice = makeAddr("alice");
        address otherWinner = makeAddr("other winner");
        _mintFromMergingPool(alice);
        _mintFromMergingPool(otherWinner);

        // Select two winners from merging pool
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        _mergingPoolContract.pickWinner(_winners);

        // Add generic model info for alice
        string[] memory _modelURI = new string[](1);
        _modelURI[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

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

        // Have alice set her fighter element & fighter weight to maximum uint256
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = type(uint256).max;
        _customAttributes[0][1] = type(uint256).max;

        // Mint Reward as alice, confirm she now owns token id 2
        vm.prank(alice);
        _mergingPoolContract.claimRewards(_modelURI, _modelType, _customAttributes);
        assertEq(_fighterFarmContract.ownerOf(2), alice);

        // Check that her fighters element and weight is in fact max uint256
        (, ,uint256 weight, uint256 element, , , ) = _fighterFarmContract.getAllFighterInfo(2);

        assertEq(weight, type(uint256).max);
        assertEq(element, type(uint256).max);
    }

Recommended Mitigation: It's recommended that FighterFarm::mintFromMergingPool function adds some verification that the given customAttributes fit within the legal limits for the protocol.

Example:

    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
+       require(customAttributes[0] < numElements[generation[0], "Invalid Fighter Element");
+       require(customAttributes[1] < 100, "Invalid Fighter Weight");
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #226

[c4-judge]

HickupHH3 marked the issue as satisfactory