AI Arena - 0xvj's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

FighterFarm contract inherited Openzepplin's ERC721 contract and overided transferFrom and safeTransferFrom functions to add a validation to restrict transfers of Fighter NFTs when they are staked. But there is another variant of safeTransferFrom function with additional data parameter which the developers forgot to overide using which malicious user can still transfer their Fighter NFTs even if they are staked.

Impact

A malicious Fighter can avoid losing points even though he lost a battle

Proof of Concept

There are three variants of transferFrom functions in Openzepplin's ERC721 contract: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L150-L183[)

    function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes calldata data
    ) external;

The first two functions were overrided to add an additional check to restrict transfers when the NFT is staked.

    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }

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

But the last safeTransferFrom function isn't overided so a malicious user can still transfer their Fighter NFT using that function even if it is staked.

Attack steps

1. Alice has some 100 points.
2. Then she lost a battle and immediately transfers her NFT to a different address.
3. In the new address accumulatedPointsPerAddress will be zero.
4. When the game server tries to update the battle record of Alice and tries to subtract let's say 10 points from the new address the updateBattleRecord function will revert due to underflow.

Because the points to subtract are calculated from accumulatedPointsPerFighter[tokenId][roundId] and subtracted from `accumulatedPointsPerAddress[fighterOwner][roundId].

            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor;
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    // @audit points to subtract are calculated from tokenId
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
                // @audit points are subtracted from fighterOwner too
                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
                totalAccumulatedPoints[roundId] -= points;
                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }

So when a user looses a battle , call to updateBattleRecord will always revert due to underflow error in the below statement, Which a malicious user can take advantage to avoid losing points.

accumulatedPointsPerAddress[fighterOwner][roundId] -= points;

Foundry Poc:

Recommended Mitigation Steps

Instead of overriding every single variant of transferFrom functions individually add the validation to the _beforeTokenTransfer hook.

    function _beforeTokenTransfer(address from, address to, uint256 tokenId)
        internal
        override(ERC721, ERC721Enumerable)
    {
+       require(_ableToTransfer(tokenId, to));
        super._beforeTokenTransfer(from, to, tokenId);
    }

Assessed type

Token-Transfer

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

GameItems contract inherited Openzepplin's ERC1155 contract and overrided safeTransferFrom function to add a validation to restrict transfers of GameItem tokens which are non-transferable. But the developers forgot to override safeBatchTransferFrom function. Malicious users can still transfer their non-transferable game Items to other addresses using safeBatchTransferFrom function.

Impact

Users can transfer non-transferable GameItems like batteries leading to bypass of game limits like battles per day.

Proof of Concept

There are two variants of safeTransferFrom functions in Openzepplin's ERC1155 contract:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L117-L146

    function safeTransferFrom(
        address from,
        address to,
        uint256 id,
        uint256 amount,
        bytes calldata data
    ) external;

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] calldata ids,
        uint256[] calldata amounts,
        bytes calldata data
    ) external;

The safeTransferFrom was overrided to add an additional check to restrict transfers of non-transferable tokens.

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

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

But the safeBatchTransferFrom function isn't overrided so a malicious user can still transfer their non-transferable GameItem tokens using this function.

This bypasses the dailyAllowance (how many gameItems a user can buy per day) mechanism as a user can buy gameItem from any address and transfers to the address that owns the Fighter NFT.

Recommended Mitigation Steps

Instead of overriding those safeTransferFrom functions individually add the validation to the _beforeTokenTransfer hook.

+    function _beforeTokenTransfer(
+        address operator,
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) internal override(ERC1155) {

+        for(uint i = 0; i < ids.length; i++) {
+            require(allGameItemAttributes[ids[i]].transferable);
+        }
+    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Due to lack of validation in redeemMintPass function a malicious user can mint a Dendroid Fighter with a Champion Fighter mint pass.

Proof of Concept

Mint passes were already minted for users using the AAMintPass contract, and users claimed their mint passes using the claimMintPass function.

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AAMintPass.sol#L109-L133

    function claimMintPass(
        uint8[2] calldata numToMint,
        bytes calldata signature,
        string[] calldata _tokenURIs
    ) 
        external 
    {
        require(!mintingPaused);
        bytes32 msgHash = bytes32(keccak256(abi.encode(
            msg.sender,
            // @audit-info Number of Champion Fighters passes to be minted
            numToMint[0], 
            // // @audit-info Number of Dendroid Fighters passes to be minted
            numToMint[1],
            passesClaimed[msg.sender][0],
            passesClaimed[msg.sender][1],
            _tokenURIs
        )));
        require(Verification.verify(msgHash, signature, delegatedAddress));
        uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
        require(_tokenURIs.length == totalToMint);
        passesClaimed[msg.sender][0] += numToMint[0];
        passesClaimed[msg.sender][1] += numToMint[1];
        for (uint16 i = 0; i < totalToMint; i++) {
            createMintPass(msg.sender, _tokenURIs[i]);
        }
    }

There are two types of Fighters called Dendroids and Champions. The AI Arena team issued signatures to users to claim the Fighter mint passes through claimMintPass and encoded the number of Champion NFTs and the number of Dendroid NFTs in the signature.

Unfortunately, while minting the Mint pass, they didn't store any data about the Fighter type in the pass.

So, a malicious user can use a Champion Mint pass to mint a Dendroid Fighter NFT using the redeemMintPass function in the FighterFarm contract. Because the function simply trusts the user-provided fighterType, mints that type of Fighter NFT and burns the user's Mint pass.

(https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L233-L262)[https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L233-L262]

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        require(
            mintpassIdsToBurn.length == mintPassDnas.length && 
            mintPassDnas.length == fighterTypes.length && 
            fighterTypes.length == modelHashes.length &&
            modelHashes.length == modelTypes.length
        );
        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
            require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

Foundry PoC:

Modify the testRedeemMintPass in test/FighterFarm.sol to below test:

    function testRedeemMintPass() public {
        // @audit 1 Champion Mint pass and 0 Dendroid Mint passes were being clamied by user
        uint8[2] memory numToMint = [1, 0];
        bytes memory signature = abi.encodePacked(
            hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
        );
        string[] memory _tokenURIs = new string[](1);
        _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        // first i have to mint an nft from the mintpass contract
        assertEq(_mintPassContract.mintingPaused(), false);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
        assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

        // once owning one i can then redeem it for a fighter
        uint256[] memory _mintpassIdsToBurn = new uint256[](1);
        string[] memory _mintPassDNAs = new string[](1);
        uint8[] memory _fighterTypes = new uint8[](1);
        uint8[] memory _iconsTypes = new uint8[](1);
        string[] memory _neuralNetHashes = new string[](1);
        string[] memory _modelTypes = new string[](1);
        
        _mintpassIdsToBurn[0] = 1;
        _mintPassDNAs[0] = "dna";
        // @audit Instead of minting Champion NFT we are minting Dendroid NFT by changing fighterType to 1
        // _fighterTypes[0] = 0;
        _fighterTypes[0] = 1;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

        // approve the fighterfarm contract to burn the mintpass
        _mintPassContract.approve(address(_fighterFarmContract), 1);

        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
        );

        // check balance to see if we successfully redeemed the mintpass for a fighter
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        // check balance to see if the mintpass was burned
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);
    }

Impact

A malicious user can mint more valuable Dendroid Fighter NFT with a Champion Fighter Mint pass

Recommended Mitigation Steps

Introduce the below storage mapping in FighterFarm contract:

    // Mapping that returns how many passes an address has redeemed already
    mapping(address => mapping(uint8 => uint8)) public passesRedeemed;

Then add the below validations and incrementation statement in redeemMintPass function's for loop block:

require(passesClaimed[msg.sender][0] <= _mintPassInstance.passesClaimed(msg.sender,0);
require(passesClaimed[msg.sender][1] <= _mintPassInstance.passesClaimed(msg.sender,1);
redeemedPasses[msg.sender][fighterType[i]] += 1;

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {       
        require(
            mintpassIdsToBurn.length == mintPassDnas.length && 
            mintPassDnas.length == fighterTypes.length && 
            fighterTypes.length == modelHashes.length &&
            modelHashes.length == modelTypes.length
        );
        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
            require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
+            require(passesClaimed[msg.sender][0] <= _mintPassInstance.passesClaimed(msg.sender,0);
+            require(passesClaimed[msg.sender][1] <= _mintPassInstance.passesClaimed(msg.sender,1);
            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
+            redeemedPasses[msg.sender][fighterType[i]] += 1;
        }
    }

The downside of this mitigation is that mint passes can only be redeemed by the users to whom the mint passes were originally issued. If a user transfers their mint pass to another user, the new owner of the mint pass cannot redeem it.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

The DNA of a fighter NFT should be determined pseudo-randomly. But redeemMintPass function in FighterFarm contract is taking the DNA value from the user. This allows the mint pass holders to deterministically mint popular NFTs with rare attributes.

Impact

User can deterministically mint popular NFTs with rare attributes.

Proof of Concept

The DNA of the fighter is being calculated pseudo-randomly in claimFighters and mintFromMergingPool functions as below:

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

            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(msg.sender, fighters.length))), // @audit
                modelHashes[i], 
                modelTypes[i],
                i < numToMint[0] ? 0 : 1,
                0,
                [uint256(100), uint256(100)]
            );

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

        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), //@audit
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );

But in redeemMintPass function DNA value is directly taken from the user instead of calculating pseudo-randomly.

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

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas, // @audit
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
                ...
        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
            require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(mintPassDnas[i]))),  // @audit
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

Due to this mint pass holders can wait until the battles began and do brute-force to calculate a DNA off-chain which yields a popular Fighter NFTs with rare attributes which perform well in battles.

Recommended Mitigation Steps

Instead of getting DNA value from the user calculate it pseudo-randomly on-chain:

            _createNewFighter(
                msg.sender, 
-               uint256(keccak256(abi.encode(mintPassDnas[i]))), 
+               uint256(keccak256(abi.encode(msg.sender, fighters.length))),
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );

Assessed type

Other

Lines of code

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

Vulnerability details

There is no validation for the fighterType parameter in reRoll function in FighterFarm contract. If a user provides invalid fighterType to reRoll function in some cases the fighters element value will be set to an invalid value.

Impact

User's Fighter NFT will get invalid attribute values.

Proof of Concept

Let's say the user's actual fighterType is Champion(0) but the user provided the fighterType as Dendroid(1) to reRoll function.

Lets say the current generation of Champions is 2 and current generation of Dendroids is 4.

And number of elements in generation 2 is numElements[2] = 2 And number of elements in generation 4 is numElements[2] = 5

As the user provided incorrect fighterType, Dendroids current generation will be considered for deciding the element of fighter instead of considering the Champions generation.

The valid element values for champions current generation is {0,1}

But as the Dendrioids generation is considered our Champion fighter will get any element value between 0 and 5 as {0,1,2,3,4} are valid element values for Dendroids.

function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        // @audit element value will be determined based on Dendroid's generation
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Due to this the user's fighter NFT will get an invalid element value. As the element decides the power of fighter in game server. User's fighter NFT may get unfair advantage or may considered invalid in the game server.

Recommended Mitigation Steps

    function reRoll(uint8 tokenId) public {
        uint8 fighterType = fighters[tokenId].dendroidBool ? 1: 0;
        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] = "";
        }
    } 

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

While incrementing the generation of a fighterType using the incrementGeneration function, the numElements mapping is not being updated for that new generation. This leads to a division by zero error in the _createFighterBase function, which breaks the minting of new Fighter NFTs permanently.

Impact

Incrementing the generation will permanently break the minting of new Fighter NFTs

Proof of Concept

While minting new fighter NFTs _createNewFighter function is being called which calls _createFighterBase function almost every time except when customAttributes[0] == 100

(https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L462-L474)[https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L462-L474]

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

In the _createFighterBase function, the calculation of the element for the new fighter NFT involves modulo dividing the dna by the number of elements in the current generation of the given fighterType. However, as the numElements mapping is not updated when incrementing the generation, the value for the new generation will always be zero. This leads to a modulo by zero error at the below line when calculating the element of the Fighter NFT.

uint256 element = dna % numElements[generation[fighterType]];

Due to this _createFighterBase function will always revert which breaks(DoS) the Fighter NFT minting process.

Foundry Poc:

Add the below test to /test/FighterFarm.t.sol, also add import "forge-std/StdError.sol"; in the contract.

    function testIncrementGenerationIssue() public {
        
        // Incrementing generation of champion Fighters
        _fighterFarmContract.incrementGeneration(0);
        
        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";
        
        // Right sender of signature actually should be able to claim fighter
        // Expect the transaction to revert with a division or modulo by zero error
        vm.expectRevert();
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
        
    }

Recommended Mitigation Steps

Set the numElement value for the new generation when incrementing generation in incrementGeneration function or include a separate setter function for numElement mapping.

function incrementGeneration(uint8 fighterType, uint8 elements) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
+        numElements[generation[fighterType]] = elements;
        // @audit why is it being increased?
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

Assessed type

DoS

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

[L-03] DEFAULT_ADMIN_ROLE not assigned In Neuron ERC20 contract DEFAULT_ADMIN_ROLE is not assigned to any user. Due to this the role once assigned cannot be revoked.

So assign DEFAULT_ADMIN_ROLE role to _ownerAddress.

Lines of code

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

Vulnerability details

While claiming Fighter NFT rewards earned by users they can select the element and weight of their fighter NFT using customAtrributes parameter which is an array of two integers.But their isn't any validation for the user provided element and weight attributes to make sure that they are in valid range.

Impact

1. Abnormal element and weight attribute values may lead to a fighter having abnormal powers, potentially disrupting game balance or fairness.
2. An innocent user's Fighter NFT will be wasted if they provide invalid attribute values and the game server rejects that Fighter NFT.

Proof of Concept

The claimRewards function in the MergingPool contract allows users to mint Fighter NFTs won in the merging pool.

It takes the customAttributes (weight and element) from the user and invokes the mintFromMergingPool function of the FighterFarm contract, passing the customAttributes values , which then calls _createNewFighter internal function

(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/FighterFarm.sol#L313-L331]

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

However, neither of these functions validates the values of the customAttributes to ensure they are within a valid range.

(https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L499-L506)[https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L499-L506]

        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }

But value of element should always be less than numElements[generation] And range of valid fighter weight is [65,95]

Foundry PoC:

 function testInvalidCustomAttributes() public {
        address user = vm.addr(1337);
        _mintFromMergingPool(user);
        _mergingPoolContract.updateWinnersPerPeriod(1);
        
        uint256[] memory _winners = new uint256[](1);
        _winners[0] = 0;

        _mergingPoolContract.pickWinner(_winners);
       
        string[] memory _modelURIs = new string[](1);
        string[] memory _modelTypes = new string[](1);
        uint256[2][] memory _customAttributes = new uint256[2][](1);

        // numElements of generation zero is only 3
        assertEq(_fighterFarmContract.numElements(0), 3);

        // Setting element values to 150 where the actual possible values for element are [1,2,3]
        _customAttributes[0][0] = uint256(150);
        // setting weight to 1000 where the valid range is [65,95]
        _customAttributes[0][1] = uint256(1000);

        vm.prank(user);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        uint256 element; uint256 weight;
        (,,weight,element,,,) = _fighterFarmContract.getAllFighterInfo(1);

        // assert that the element of the claimed Fighter NFT is 150
        assertEq(element,150);
        // assert that the element of the claimed Fighter NFT is 1000
        assertEq(weight,1000);

    }

Recommended Mitigation Steps

Add validations for the custom attributes in _createNewFighter function

if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
+           require(customAttributes[0] < numElements[generation[fighterType]]);
+           require(customAttributes[1] <= 95);
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

The claimNRN function in RankedBattle contract and claimRewards function in MergingPool contract only allow users to claim their rewards in batch from last claimed round to current round. They don't allow to claim rewards for a particular rounds. So these functions are succeptible to DOS if last claimed round is too far.

Impact

Users won't be able to claim their NRN and Fighter NFT rewards.

Proof of Concept

If a particular user joins the AI Arena long time after the game launches, and tries to claim rewards using the claimNRN and claimRewards functions, these functions will attempt to claim rewards from the first round until the current round. This may lead to a DoS scenario if the transaction exceeds the block gas limit and users rewards will be locked in the protocol forever.

    function claimNRN() external {
        require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution) / totalAccumulatedPoints[currentRound];
            numRoundsClaimed[msg.sender] += 1;
        }


        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
        }
    }

DoS scenario for claimNRN function :

1. Lets say Alice joins the game 10 years after the game lauched.
2. By that time 520 rounds have already completed (rounds lasts 1 week according to docs).
3. Alice started playing from 521 rounds and earned some NRN through staking some NRN.
4. When Alice calls claimNRN function as numRoundsClaimed[alice] is zero reward claiming process will start from round 0 all the way up to round 521.
5. So the transaction may exceed block gas limit which leads to DoS.

The DoS scenario for the claimRewards function of the MergingPool is the same as for the claimNRN function.

Recommended Mitigation Steps

Allow the specification of round number and rounds length for claimRewards and claimNRN functions, so that claiming can be broken up into smaller batches if required.

Assessed type

DoS