AI Arena - ktg's results

Lines of code

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

Vulnerability details

Impact

• _ableToTransfer will be bypassed
• Staked fighter can be transferred
• Address can have more nfts than MAX_FIGHTERS_ALLOWED

Proof of Concept

To restrict the transfer of fighters in certain cases, contract FighterFarm override function transferFrom and safeTransferFrom and added a check using function _ableToTransfer:

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, "");
    }
    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }
    
function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

Function _ableToTransfer will and forbid the transfer if balance of to address exceeds MAX_FIGHTERS_ALLOWED or the fighter is staked.

However, FighterFarm inherit openzeppelin ERC721 contract and you can see they have 2 functions with same name safeTransferFrom, one with data param and one without:

function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public virtual override {
        safeTransferFrom(from, to, tokenId, "");
    }

    /**
     * @dev See {IERC721-safeTransferFrom}.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

The contract FighterFarm only overrides and adds check to safeTransferFrom; therefore, if someone call safeTransferFrom with data, function _ableToTransfer will be bypassed.

This issue will have big impact because the fact that users cannot transfer fighter when they are staked is an important logic of AI Arena. There are many ways to exploit this, for example:

• A user can create a very weak fighter, stake it and transfer to someone else. Since it's staked, it will be selected for Battle Arena and the receiver of the weak nft will lose matches

Below is a POC, save it to contract RankedBattleTest under file test/RankedBattle.t.sol and run it using command: forge test --match-path test/RankedBattle.t.sol --match-test testSafeTransferFromWithData -vvvv

function testSafeTransferFromWithData() public {
        address alice = vm.addr(35);
        _mintFromMergingPool(alice);
        _fundUserWith4kNeuronByTreasury(alice);
        vm.startPrank(alice);
        // Nft 0 is already staked
        _rankedBattleContract.stakeNRN(10 ** 18, 0);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        assertEq(_fighterFarmContract.ownerOf(0), alice);

        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(alice, _ownerAddress, 0);

        // Success when transferred with safeTransferFrom with data
        _fighterFarmContract.safeTransferFrom(alice, _ownerAddress, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        vm.stopPrank();
        
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Since function safeTransferFrom without data will call function safeTransferFrom with data, you only have to override function safeTransferFrom with data and add the check:

function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        bytes memory data
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, data);
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L291-L303

Vulnerability details

Impact

• Non transferable GameItems could be transferred

Proof of Concept

Contract GameItems represents a collection of game items used in AI Arena. The contract inherits ERC1155 contract, and the transfer of "game items" corresponds to the transfer of ERC1155 tokens.

Each game item has a field called transferable which decides if it could be transferred or not. The contract prohibits the transfer of non-transferable items by overriding ERC1155 function safeTransferFrom and requires transferable = True:

function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

However, ERC1155 function safeBatchTransferFrom is not overridden and added the same check, allowing game items with transferable = False to be transferred.

Below is a POC, save this test case to contract GameItemsTest under file test/GameItems.t.sol and run it using command: forge test --match-path test/GameItems.t.sol --match-test testSafeBatchTransfer -vvvv

function testSafeBatchTransfer() public {
        // Create a gameItem with transferable = false and price =0 for testing
        _gameItemsContract.createGameItem(
            "Gloves", "https://ipfs.io/ipfs/gloves", false, false, 10_000, 0, 10
        );
        (string memory name,,,,,) = _gameItemsContract.allGameItemAttributes(1);
        assertEq(name, "Gloves");
        assertEq(_gameItemsContract.remainingSupply(1), 10_000);
        assertEq(_gameItemsContract.uniqueTokensOutstanding(), 2);
        _gameItemsContract.mint(1, 10);
        // If transfer using safeTransferFrom then fail
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 1, 10, "");

        // Transfer successfully with safeBatchTransferFrom
        uint256[] memory ids  = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        ids[0] = 1;
        amounts[0] = 10;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");


    }

Tools Used

Manual review.

Recommended Mitigation Steps

I recommend overriding function safeBatchTransferFrom and add the same check as safeTransferFrom.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

• An nft could be rerolled more than allowed in maxRerollsAllowed

Proof of Concept

Fighter nft can be rerolled, and they can not be rerolled more than what they can. maxRerollsAllowed is a variable defines the number of times an nft can be rerolled, it's an array with 2 elements, the first is the number of times nft of type 0 can be rerolled, and second is for type 1. And each time fighter generation upgraded, maxRerollsAllowed for that fighter type increases by 1:

uint8[2] public maxRerollsAllowed = [3, 3];

function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

The problem is that the function reRoll does not derive the fighterType from the nft data but get it from user input:

function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
...

Thefore, uses can pass in a different fighterType and can reRoll their nfts more than allowed. For example:

• At the start maxRerollsAllowed = [3,3]
• Alice is created a fighter with tokenId = 0 and fighterType = 0
• Alice then reroll her fighter 3 times and then she cannot reRoll anymore
• The owner then call incrementGeneration with fighterType = 1, now maxRerollsAllowed = [3,4], which means the maximum number of rerolls for fighterType = 0 is still 3. However, Alice can just call reroll with fighterType = 1 and successfully reroll again.

Below is a POC, save this test case to contract FighterFarmTest under file test/FighterFarm.t.sol and run it using command: forge test --match-path test/FighterFarm.t.sol --match-test testRerollMoreThanAllowed -vvvv

Note that in the POC I cannot reroll again also because of another vulnerability I've reported involving generation upgrade. But if you run the test, you will see the test case reverts with Division or modulo by 0, which indicates the line require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]) is passed.

function testRerollMoreThanAllowed() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model

        uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
        uint8 tokenId = 0;
        uint8 fighterType = 0;

        _neuronContract.addSpender(address(_fighterFarmContract));
        _fighterFarmContract.reRoll(tokenId, fighterType);
        _fighterFarmContract.reRoll(tokenId, fighterType);
        _fighterFarmContract.reRoll(tokenId, fighterType);
        vm.expectRevert();
        _fighterFarmContract.reRoll(tokenId, fighterType);
        assertEq(_fighterFarmContract.numRerolls(0), 3);

        // Set new generation for fighterType = 1
        _fighterFarmContract.incrementGeneration(1);
        assertEq(_fighterFarmContract.maxRerollsAllowed(1), 4);
        assertEq(_fighterFarmContract.maxRerollsAllowed(0), 3);

        //
        vm.expectRevert();
        _fighterFarmContract.reRoll(tokenId, 1);

    }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend deriving fighterType from the user's nft data in function reRoll instead of letting users pass it as input.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L439 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L519-L534

Vulnerability details

Impact

• NFT with dust staked amount will avoid being placed at risk

Proof of Concept

Function _addResultPoints of RankedBattle is called by game server to update nft points. In case an nft loses and having no points, then the staked amount on that nft is placed at risk. If an nft have some staked amount placed at risk (any amount), then the nft will not gain points the next round if it wins, instead stakeAtRisk.reclaimNRN is called to reduce the staked amount at risk. Only when staked amount at risk is reduced to zero that the nft can accumulate points again.

However, the variable curStakeAtRisk is currently calculated as: curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4 with bpsLostPerLoss = 10. If amountStaked is small enough, for example amountStaked[tokenId] = 1, then curStakeAtRisk = 0.

If the nft loses and having no points, then its staked at risk record is updated with updateAtRiskRecords:

bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }

However, since curStakeAtRisk is zero, the nft is not labeled as having stake at risk and can just earn points the next win normally.

Furthermore, staking dust amount does not decrease the points an nft can receive, as the points = stakingFactor * eloFactor, in that eloFactor is assigned by game server and corresponds to nft ranking, and stakingFactor is calculated as:

function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    } 

You can see if amountStaked[tokenId] is very small, then stakingFactor will still = 1. Therefore, there's not difference if an nft is staked 1 or 10**18 tokens.

Below is a POC, save this test case to contract RankedBattleTest under file test/RankedBattle.t.sol and run it using command: forge test --match-path test/RankedBattle.t.sol --match-test testDustStakeAmount -vvvv

function testDustStakeAmount() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);

        // Stake 1
        _rankedBattleContract.stakeNRN(1, 0);
        assertEq(_rankedBattleContract.amountStaked(0),1);

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        // If the nft wins, we have 1500 points (mergingPortion = 0)
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);
        assertEq(_rankedBattleContract.accumulatedPointsPerAddress(player, 0), 1500);

        // If the player loses, his points will be zero
        _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
        assertEq(_rankedBattleContract.accumulatedPointsPerAddress(player, 0), 0);

        // If the player loses again, his staked amount is not placed at risk
         _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
        assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 0);

        // Since the player has no amount at risk, he can gain points if he wins
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);
        assertEq(_rankedBattleContract.accumulatedPointsPerAddress(player, 0), 1500);

        vm.stopPrank();


    }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend requiring the owner to stake an amount greater than a predefined threshold to prevent dust amount.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

• Fighters with tokenId >= 256 cannot be rerolled due to overflow

Proof of Concept

Function reRoll of contract FighterFarm is currently implemented as:

function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }  

As you can see, the input tokenId is of type uint8, this will prevent all nfts with tokenId > 255 to be rerolled. Given that there will be thousands of nfts created for players, most of them cannot be rerolled.

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend changing tokenId input from uint8 to uint256

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

• FighterFarm will be unable to mint new fighters.
• FighterFarm contracts can only work for 1 generation

Proof of Concept

Function _createFighterBase of contract FighterFarm is responsible for calculating fighter's weight, element 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);
    }

Fighter's element is computed as dna % numElements[generation[fighterType]]. In this formula:

• generation is of type uint8[2], it's initialized as [0, 0]. Generation can be increased using function incrementGeneration:

function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

• numElements is of type mapping(uint8 => uint8)

The problem is that numElements are initialized only once in constructor, and then there's no way to add new element to numElements (you can search the code to confirm this); therefore, after generation upgrade, numElements[generation[fighterType]] will return default value = 0, and the function will revert with Division or modulo by 0. Let's take an example, in this example, fighterType = 0:

• At the start, generation = [0,0] and numElements[0] = 3
• generation[fighterType] = generation[0] = 0 => numElements[generation[fighterType]] = numElements[generation[0]] = numElements[0] = 3
• After generation upgrade, generation[0] = 1 => numElements[generation[0]] = numElements[1] = 0
• Function _createFighterBase will revert with Division or modulo by 0

Function _createFighterBase is used in _createNewFighter and reRoll function, making the users unable to reroll their fighters and create new fighters.

Below is a POC, save this test case to contract FighterFarmTest in file test/FighterFarm.t.sol and run it using command: forge test --match-path test/FighterFarm.t.sol --match-test testCreateNewFighterFailAfterIncrementGeneration -vvvv

function testCreateNewFighterFailAfterIncrementGeneration() public {
        //
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // Increment generation for fighter type 0
        vm.prank(_ownerAddress);
        _fighterFarmContract.incrementGeneration(0);

        // Will revert with Division or modulo by 0
        vm.startPrank(address(_mergingPoolContract));
        vm.expectRevert();
         _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
        vm.stopPrank();


    }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend these fixes:

• Check if numElements[generation[fighterType]] = 0, then take a default value for calculation instead
• Create a new function for adding new values to numElements

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

• Fighter created by mintFromMergingPool can have arbitrary weight and element like 0 or 2**256 - 1
• Invalid weight and element could greatly affect AI Arena battles.

Proof of Concept

When someone claim their nft rewards from MergingPool, they can input customeAttributes and create fighters with arbitrary values since currently there is no check on customeAttributes and it could varies from 0 to 2**256 -1 (type(uint256).max):

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
     ....
        
             _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                   
          
    ....
    }

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
        );
    }

This allow creating fighters with element and weight range from 0 to 2**256 -1 and can have negative impact on AI Arena matches according to the doc here https://docs.aiarena.io/gaming-competition/nft-makeup, for example:

• Weight is described in the doc as used to calculate how far the fighter moves when being knocked back.. If an nft has extremely large weight like 2**256-1, then it could never be knocked back
• Element can only be one of Fire, Electricity or Water, an nft with element outside of this list could be created.

Below is a POC, save the test case to contract MergingPoolTest under file test/MergingPool.t.sol and run it using command: forge test --match-path test/MergingPool.t.sol --match-test testClaimRewardsArbitraryElementAndWeight -vvvv

function testClaimRewardsArbitraryElementAndWeight() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == _ownerAddress, true);
        // winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);
        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(0);
        _customAttributes[0][1] = uint256(type(uint256).max);
        _customAttributes[1][0] = uint256(type(uint256).max);
        _customAttributes[1][1] = uint256(0);
        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        // Fighter 2 has 2**256 weight and element = 0
        (, ,uint256 weight , uint256 element , , , ) = _fighterFarmContract.getAllFighterInfo(2);
        assertEq(weight, 2**256 - 1);
        assertEq(element, 0);

        // Fighter 3 has 0 weight and 2**256 element
        (, , weight ,  element , , , ) = _fighterFarmContract.getAllFighterInfo(3);
        assertEq(weight, 0);
        assertEq(element, 2**256 - 1);

    }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend checking customAttributes in function mintFromMergingPool and only restrict weight and element in predefined ranges. For example, weight must be in range [65, 95], element must be in range [0,2].

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

• Users who won more than 10 fighters in merging pool will lose their rewards

Proof of Concept

Each round the MergingPool admin will pick nft winners for that round by calling pickWinner. Winners can then claim their nfts by calling claimRewards:

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

The problem with this function is that it will claim all nfts from (numRoundsClaimed to latest roundId). Currently, the number of nfts per user cannot exceed MAX_FIGHTERS_ALLOWED (currently set to 10); therefore, if a user won for example 10 nfts and call claimRewards, they are forced to claim it all at once and claimRewards will revert, making the user to lose all his/her nft rewards.

Below is a POC, save these test cases under contract MergingPoolTest in file test/MergingPool.t.sol and run it using command: forge test --match-path test/MergingPool.t.sol --match-test testClaimRewardsRevertWhenMaxFighterIsReached -vvvv

There are 2 test cases, test case testClaimRewardsRevertWhenMaxFighterIsReachedSuccess demonstrates that a user can successfully claim 9 nfts; test case testClaimRewardsRevertWhenMaxFighterIsReachedFail demonstrates that a user will lose their nfts if they won 10 nfts and claim them at once:

function testClaimRewardsRevertWhenMaxFighterIsReachedSuccess() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // picks winner for 9 rounds
         _mergingPoolContract.pickWinner(_winners); // round 0
         _mergingPoolContract.pickWinner(_winners); // round 1
         _mergingPoolContract.pickWinner(_winners); // round 2
         _mergingPoolContract.pickWinner(_winners); // round 3
         _mergingPoolContract.pickWinner(_winners); // round 4
         _mergingPoolContract.pickWinner(_winners); // round 5
         _mergingPoolContract.pickWinner(_winners); // round 6
         _mergingPoolContract.pickWinner(_winners); // round 7
         _mergingPoolContract.pickWinner(_winners); // round 8





        // winner claims rewards for previous roundIds
        string[] memory _modelURIs = new string[](9);
        string[] memory _modelTypes = new string[](9);
        for (uint i = 0 ; i< 9; i++){
            _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
            _modelTypes[0] = "original";
        }

        uint256[2][] memory _customAttributes = new uint256[2][](9);
        for(uint i =0; i< 9; i++) {
            _customAttributes[i][0] = uint256(1);
            _customAttributes[i][1] = uint256(50);
        }
        // Claim rewards succeeds, MAX_FIGHTERS_ALLOWED not reached
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);


    }

    function testClaimRewardsRevertWhenMaxFighterIsReachedFail() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // pick winners for 10 rounds
         _mergingPoolContract.pickWinner(_winners); // round 0
         _mergingPoolContract.pickWinner(_winners); // round 1
         _mergingPoolContract.pickWinner(_winners); // round 2
         _mergingPoolContract.pickWinner(_winners); // round 3
         _mergingPoolContract.pickWinner(_winners); // round 4
         _mergingPoolContract.pickWinner(_winners); // round 5
         _mergingPoolContract.pickWinner(_winners); // round 6
         _mergingPoolContract.pickWinner(_winners); // round 7
         _mergingPoolContract.pickWinner(_winners); // round 8
         _mergingPoolContract.pickWinner(_winners); // round 9




        // claim rewards
        string[] memory _modelURIs = new string[](10);
        string[] memory _modelTypes = new string[](10);
        for (uint i = 0 ; i< 10; i++){
            _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
            _modelTypes[0] = "original";
        }

        uint256[2][] memory _customAttributes = new uint256[2][](10);
        for(uint i =0; i< 10; i++) {
            _customAttributes[i][0] = uint256(1);
            _customAttributes[i][1] = uint256(50);
        }
        // Fail since MAX_FIGHTERS_ALLOWED is reached
        vm.expectRevert();
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    }

Tools Used

Manual Review

Recommended Mitigation Steps

I recommend allowing users to claim rewards to a certain round of their choice. This would allow the owners to transfer nfts to other addresses before continue claiming their remaining rewards.

Assessed type

Invalid Validation

Judge has assessed an item in Issue #1287 as 2 risk. The relevant finding follows:

L-01: MergingPool.claimRewards may DOS users