AI Arena - Aamir's results

Lines of code

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

Vulnerability details

The FighterFarm contract permits the transfer of Fighter NFTs even under conditions where the receiver exceeds the maximum allowed limit of fighters or the sender has staked on the fighter. This vulnerability arises due to the improper override of the safeTransferFrom() function.

Impact

The FighterFarm contract restricts the transfer of Fighter NFTs to an address unless all of the following conditions is met:

1. The msg.sender is approved or the owner of the NFT.
2. The balance of the receiver is below the maximum limit of fighters allowed per address.
3. The Fighter NFT is not currently staked.

The transferFrom() and safeTransferFrom() functions are overridden in the FighterFarm contract to include checks for these conditions with the help of the following function:

File: 2024-02-ai-arena/src/FighterFarm.sol

    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]
        );
    }

GitHub: [539-545]

The ERC721 standard includes two variations of the safeTransferFrom() function. One variant features an additional argument named data, which can be transmitted to the recipient if it's a contract. Essentially, the first variation internally relies on the second variation (referenced here). However, instead of overriding the second function, the contract mistakenly overrides the first one, which is an incorrect approach. Consequently, users retain the ability to transfer their tokens to recipients who fail to meet the last two conditions outlined above.

Proof of Concept

Here is a test for PoC:

Include the following test in FighterFarm.t.sol

    function testAnAddressCanHaveMoreThanMaxLimitOfFighters() public {
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        
        // minting 10 fighters to alice and bob
        _mintMultipleNFTsToAUser(alice);
        _mintMultipleNFTsToAUser(bob);

        // getting the balance of alice and bob
        uint256 balanceOfAlice = _fighterFarmContract.balanceOf(alice);
        uint256 balanceOfBob = _fighterFarmContract.balanceOf(bob);
        
        // chekcing the balance of alice and bob
        assertEq(balanceOfAlice, 10);
        assertEq(balanceOfBob, 10);

        // bob tries to transfer his NFT to alice will fails as alice already have max limit of fighters
        vm.prank(bob);
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(bob, alice, 10);

        // but bob can still transfer his NFT to alice because override of second variation of safeTransferFrom is not done
        vm.prank(bob);
        _fighterFarmContract.safeTransferFrom(bob, alice, 10, "0x0DeadBeef"); 

        // checking the balance of alice and bob
        assertEq(_fighterFarmContract.balanceOf(alice), 11);
        assertEq(_fighterFarmContract.balanceOf(bob), 9);    
    }

    function _mintMultipleNFTsToAUser(address user) public{
        (address delegated, uint256 delegatedPvtKey) = makeAddrAndKey("delegated");

        // setting new delegate.
        // NOTE: this function is not present in the contract, it is just for testing purposes
        _fighterFarmContract.setDelegatedAddress(delegated);

        // creating signature for the user to 3 challenger and 2 dendroids

        uint8[2] memory numToMint = [10, 0];
        bytes32 messageToSign = keccak256(abi.encode(
            user,
            numToMint[0],
            numToMint[1],
            _fighterFarmContract.nftsClaimed(user, 0),
            _fighterFarmContract.nftsClaimed(user, 1)
        ));

        bytes32 signedMessage = ECDSA.toEthSignedMessageHash(messageToSign);

        (uint8 v, bytes32 r, bytes32 s)  = vm.sign(delegatedPvtKey, signedMessage); 

        bytes memory signature = abi.encodePacked(r, s, v);

        string[] memory claimModelHashes = new string[](10);
        for(uint i = 0; i < 10; i++){
            claimModelHashes[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        }

        string[] memory claimModelTypes = new string[](10);
        for(uint i = 0; i < 10; i++){
            claimModelTypes[i] = "original";
        }

        // Right sender of signature should be able to claim fighter
        vm.prank(user);
        _fighterFarmContract.claimFighters(numToMint, signature, claimModelHashes, claimModelTypes);

    }

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/ai-arena-audit]
└─$ forge test --mt testAnAddressCanHaveMoreThanMaxLimitOfFighters
[⠔] Compiling...
No files changed, compilation skipped

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testAnAddressCanHaveMoreThanMaxLimitOfFighters() (gas: 9323024)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.68ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review

Recommended Mitigation Steps

It is recommended to override the second variation of the safeTransferFrom function instead of the first variation:

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

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

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L10

Vulnerability details

The GamesItems contract fails to appropriately override and include essential checks within the safeBatchTransferFrom function, enabling the transfer of non-transferrable Game Items.

Impact

While the GamesItems contract allows for the designation of Game Items as either transferrable or non-transferrable through different states and overrides the ERC1155::safeTransferFrom(...) function accordingly, it neglects to override the ERC1155::safeBatchTransferFrom(...) function. This oversight permits users to transfer Game Items that were intended to be non-transferrable.

Proof of Concept

Here is a test for PoC:

NOTE Include the below given test in GameItems.t.sol

    function testNonTransferableItemCanBeTransferredWithBatchTransfer() public {
        // funding owner address with 4k $NRN
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        // owner minting itme
        _gameItemsContract.mint(0, 1);

        // checking that the item is minted correctly
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

        // making the item non-transferable
        _gameItemsContract.adjustTransferability(0, false);

        vm.expectRevert();
        // trying to transfer the non-transferable item. Should revert
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // checking that the item is still in the owner's account
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

        // transferring the item using safeBatchTransferFrom
        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");

        // checking that the item is transferred to the delegated address
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/ai-arena-audit]
└─$ forge test --mt testNonTransferableItemCanBeTransferredWithBatchTransfer
[⠒] Compiling...
[⠃] Compiling 1 files with 0.8.13
[⠒] Solc 0.8.13 finished in 1.77s
Compiler run successful!

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testNonTransferableItemCanBeTransferredWithBatchTransfer() (gas: 190756)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.32ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to override the safeBatchTransferFrom(...) function and include the necessary checks to prevent the transfer of non-transferrable Game Items.

+    function safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) public override(ERC1155) {
+        for(uint256 i; i < ids.length; i++{
+            require(allGameItemAttributes[ids[i]].transferable);
+        }
+        super.safeBatchTransferFrom(from, to, ids, amounts, data);
+    }

Or, consider overriding the _safeBatchTransferFrom(...) function as follows:

+    function _safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) internal override(1155) {
+        require(ids.length == amounts.length, "ERC1155: ids and amounts length mismatch");
+        require(to != address(0), "ERC1155: transfer to the zero address");
+
+        address operator = _msgSender();
+
+        _beforeTokenTransfer(operator, from, to, ids, amounts, data);
+
+        for (uint256 i = 0; i < ids.length; ++i) {
+            require(
+            uint256 id = ids[i];
+            uint256 amount = amounts[i];
+           require(allGameItemAttributes[id].transferable);
+            uint256 fromBalance = _balances[id][from];
+            require(fromBalance >= amount, "ERC1155: insufficient balance for transfer");
+            unchecked {
+                _balances[id][from] = fromBalance - amount;
+            }
+            _balances[id][to] += amount;
+        }
+
+        emit TransferBatch(operator, from, to, ids, amounts);
+
+        _afterTokenTransfer(operator, from, to, ids, amounts, data);
+
+        _doSafeBatchTransferAcceptanceCheck(operator, from, to, ids, amounts, data);
+    }

Assessed type

Other

Lines of code

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

Vulnerability details

The FighterFarm::redeemMintPass(...) function lacks validation for the type of NFTs that can be minted by burning the mint pass, allowing holders to claim any type of fighter they desire.

Impact

Currently, there are two types of fighter NFT available in the game. AR-X Bots / ArenaX bots / champions and dendroids. Dendroids are more exclusive class of NFTs that have some special ability as given in the official documentation. This class is more rare and difficult to obtain.

But FighterFarm::redeemMintPass(...) allows anyone who holds an AAMintPass to claim any type of Fighter NFTs they want. There is no check for that in the function. The function takes in the type of the NFT desired and mint the same to the user.

The sponsors have also verified that this behavior is not intended, as users should not have the capability to select the type of NFTs they wish to mint.

Proof of Concept

Here is test for PoC:

Include the below given test in FighterFarm.t.sol

    function testAnyoneWithAAMintPassCanMintAnyTypeOfFighterTheyWant() public {
        // setup
        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";
        // setting fighter type to 1. This means the fighter will be a rare fighter
        _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);
    }

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/ai-arena-audit]
└─$ forge test --mt testAnyoneWithAAMintPassCanMintAnyTypeOfFighterTheyWant
[⠢] Compiling...
[⠊] Compiling 1 files with 0.8.13
[⠒] Solc 0.8.13 finished in 3.64s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testAnyoneWithAAMintPassCanMintAnyTypeOfFighterTheyWant() (gas: 582877)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 12.09ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to add the signature validation as well in the FighterFarm::redeemMintPass(...) for checking the type of NFTs that user can claim through his mint pass or use some other way to do the verification.

    // the verification can also be done for multiple NFTs at once
    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes,
+        bytes[] calldata signatures,
    ) 
        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]));

+       // Add the necessary data in the msgHash. Here I have added only two
+       bytes32 msgHash = bytes32(keccak256(abi.encode(
+            msg.sender, 
+            fighterTypes[i], 
+            mintpassIdsToBurn[i]
+        )));
+        require(Verification.verify(msgHash, signatures[i], _delegatedAddress));

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

Assessed type

Other

Lines of code

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

Vulnerability details

The FighterFarm::reRoll(...) function exposes a vulnerability by failing to verify the fighter type provided to the contract. This oversight allows for additional rerolls beyond the established limit when the maximum reroll limits for different fighter types are unequal.

Impact

The oversight in FighterFarm::reRoll(...) allows users to reroll their fighter NFTs, even if they don't match the specified fighter type. Initially, each fighter NFT in the FighterFarm contract can be either of type 0 or type 1, each with its own generation and max reroll limit. The contract sets an initial reroll limit of 3 for all fighter NFTs. However, when the FighterFarm::incrementGeneration() function is called for a specific fighter type, both the generation and the maximum reroll limit for that type increases. Despite this, the contract fails to verify whether the provided fighter type matches the NFT being rerolled. As a result, users can exceed the maximum reroll limit for their current fighter type by requesting a reroll with a different fighter type. This issue is particularly concerning when there are unequal reroll limits for different fighter types, as it enables users to exploit the difference to gain an unfair advantage in rerolling their NFTs.

For instance:

1. Alice initially mints a fighter NFT of type 0.
2. Over time, Alice rerolls the NFT up to 3 times (the initial maximum reroll limit).
3. Later, the generation for fighter type 1 is elevated by the owner, thereby increasing the maximum reroll limit for fighter type 1 by 1.
4. Alice can now specify fighterType = 1 in FighterFarm::reRoll(...) for her NFT of type 0 and obtain an additional reroll.

Proof of Concept

Here is a test for PoC:

Notes:

1. Include the below given test in FighterFarm.t.sol
2. Make sure the FighterFarm::incrementGeneration(...) is modified as given below. This line is added because it is missing in the original function (a bug) that will cause DoS when the generation is increased.

Add the following Line in the FighterFarm::incrementGeneration(...) function:

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

Test:

    function testAUserCanReRollNFTMoreThanTheMaxLimitForTheGeneration() public {
        address alice = makeAddr("alice");

        // giving some tokens to alice
        _fundUserWith4kNeuronByTreasury(alice);

        // minting 10 fighters to alice
        _mintMultipleNFTsToAUser(alice);

        // getting the balance of alice
        uint256 balanceOfAlice = _fighterFarmContract.balanceOf(alice);

        // chekcing the balance of alice
        assertEq(balanceOfAlice, 10);

        // checking the fighter type of the first fighter and if owner is alice
        (,,,,,,,,bool dendroid) = _fighterFarmContract.fighters(0);
        assertEq(dendroid, false);
        assertEq(_fighterFarmContract.ownerOf(0), alice);

        // giving the spender role to the fighter farm contract in neuron contract
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Rerolling the fighter NFT 3 times which is the current max limit
        vm.startPrank(alice);
        _fighterFarmContract.reRoll(0, 0);
        _fighterFarmContract.reRoll(0, 0);
        _fighterFarmContract.reRoll(0, 0);

        // trying to reroll the fighter NFT again will fail
        vm.expectRevert();
        _fighterFarmContract.reRoll(0, 0);

        // trying to reroll the fighter with fighter type 1 will fail as well
        vm.expectRevert();
        _fighterFarmContract.reRoll(0, 1);
        vm.stopPrank();

        // Let's say the generation for the fighter type 1 is increased in the future. this will also increase
        // the max reRoll limit for it. But alice cannot reRoll again since only holds the NFT of fighter type 0.
        // But she can reRoll again by passing the fighter type 1

        // increasing the limit for fighter type 1
        _fighterFarmContract.incrementGeneration(1);

        vm.prank(alice);
        _fighterFarmContract.reRoll(0, 1);
    }

    function _mintMultipleNFTsToAUser(address user) public{
        (address delegated, uint256 delegatedPvtKey) = makeAddrAndKey("delegated");

        // setting new delegate.
        // NOTE: this function is not present in the contract, it is just for testing purposes
        _fighterFarmContract.setDelegatedAddress(delegated);

        // creating signature for the user to 3 challenger and 2 dendroids

        uint8[2] memory numToMint = [10, 0];
        bytes32 messageToSign = keccak256(abi.encode(
            user,
            numToMint[0],
            numToMint[1],
            _fighterFarmContract.nftsClaimed(user, 0),
            _fighterFarmContract.nftsClaimed(user, 1)
        ));

        bytes32 signedMessage = ECDSA.toEthSignedMessageHash(messageToSign);

        (uint8 v, bytes32 r, bytes32 s)  = vm.sign(delegatedPvtKey, signedMessage); 

        bytes memory signature = abi.encodePacked(r, s, v);

        string[] memory claimModelHashes = new string[](10);
        for(uint i = 0; i < 10; i++){
            claimModelHashes[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        }

        string[] memory claimModelTypes = new string[](10);
        for(uint i = 0; i < 10; i++){
            claimModelTypes[i] = "original";
        }

        // Right sender of signature should be able to claim fighter
        vm.prank(user);
        _fighterFarmContract.claimFighters(numToMint, signature, claimModelHashes, claimModelTypes);

    }

Output:

┌──(aamirusmani1552㉿Victus)-[/mnt/d/ai-arena-audit]
└─$ forge test --mt testAUserCanReRollNFTMoreThanTheMaxLimitForTheGeneration
[⠘] Compiling...
[⠒] Compiling 12 files with 0.8.13
[⠒] Solc 0.8.13 finished in 9.18s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testAUserCanReRollNFTMoreThanTheMaxLimitForTheGeneration() (gas: 4948244)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.92ms

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

Tools Used

• Manual Review

Recommended Mitigation Steps

It is recommended to add the following changes:

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

+       // It will work as the tokenId minted depends on the length of the fighters array
+       require((fighterType == 0 && fighters[tokenId].dendroidBool == false) || (fighterType == 1 && fighters[tokenId].dendroidBool == true), "Invalid fighter type");   

        _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

Error

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

[L-8] RankedBattle::_getStakingFactor() works in a biased way for small stakers.

Lines of code

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

Vulnerability details

The FighterFarm::incrementGeneration(...) function fails to update the numElements mapping based on the generation of fighter type, leading to denial of service when users attempt to mint.

Impact

In the game, each fighter NFT possesses unique abilities determined by their elemental type. Currently, three elemental types exist, each with its own set of elemental moves, as outlined in the documentations. These moves have varying strengths and weaknesses based on matchups, making them integral to gameplay.

The FighterFarm contract maintains a count of elements by fighter generation:

File: 2024-02-ai-arena/src/FighterFarm.sol

85       mapping(uint8 => uint8) public numElements;

110      numElements[0] = 3;

GitHub: [110, 85]

This implies that each fighter generation can have a different number of elements. However, the contract fails to add corresponding elements to the numElements mapping when increasing the generation of fighters.

File: 2024-02-ai-arena/src/FighterFarm.sol

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

GitHub: [29-34]

Consequently, when a user tries to mint a new NFT for a given generation, the transaction fails due to division by zero, resulting in a denial of service:

File: 2024-02-ai-arena/src/FighterFarm.sol

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

GitHub: [47]

Additionally, there is no separate function in the contract to add elements.

Proof of Concept

Here is a test for PoC:

NOTE

1. Add the below given test in the FighterFarm.t.sol
2. There is no setDelegatedAddress() function in the FighterFarm.sol. It was added for the sake of testing only
3. The signature generated in the test below functions correctly. There is a function given at the end of this section to check the same.

    // @audit-info test-passed
    function testClaimFighterWithCustomSignatureAfterIncreasingGenerationLeadsToRevertTx() public {
        address alice = makeAddr("alice");
        (address delegated, uint256 delegatedPvtKey) = makeAddrAndKey("delegated");

        // increment the generation of type 1 fighter
        _fighterFarmContract.incrementGeneration(1);

        // setting new delegate.
        // NOTE: this function is not present in the contract, it is just for testing purposes
        _fighterFarmContract.setDelegatedAddress(delegated);

        // creating signature for the user to 3 challenger and 2 dendroids
        uint8[2] memory numToMint = [3, 2];
        bytes32 messageToSign = keccak256(abi.encode(
            alice,
            numToMint[0],
            numToMint[1],
            _fighterFarmContract.nftsClaimed(alice, 0),
            _fighterFarmContract.nftsClaimed(alice, 1)
        ));

        bytes32 signedMessage = ECDSA.toEthSignedMessageHash(messageToSign);

        (uint8 v, bytes32 r, bytes32 s)  = vm.sign(delegatedPvtKey, signedMessage);


        bytes memory signature = abi.encodePacked(r, s, v);

        string[] memory claimModelHashes = new string[](5);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[3] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[4] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](5);
        claimModelTypes[0] = "original";
        claimModelTypes[1] = "original";
        claimModelTypes[2] = "original";
        claimModelTypes[3] = "original";
        claimModelTypes[4] = "original";

        // Right sender of signature should be able to claim fighter
        vm.prank(alice);
        vm.expectRevert(stdError.divisionError);
        _fighterFarmContract.claimFighters(numToMint, signature, claimModelHashes, claimModelTypes);
    }

Also Include the following function in the FighterFarm.sol:

    function setDelegatedAddress(address newDelegatedAddress) external {
        require(msg.sender == _ownerAddress);
        _delegatedAddress = newDelegatedAddress;
    }

Output:

└─$ forge test --mt testClaimFighterWithCustomSignatureAfterIncreasingGenerationLeadsToRevertTx
[⠃] Compiling...
[⠃] Compiling 1 files with 0.8.13
[⠊] Solc 0.8.13 finished in 3.93s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testClaimFighterWithCustomSignatureAfterIncreasingGenerationLeadsToRevertTx() (gas: 1543320)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.93ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Test to checking the signatures:

<details> <summary>Code:</summary>

    function testClaimFighterWithCustomSignatureSuccess() public {
        address alice = makeAddr("alice");
        (address delegated, uint256 delegatedPvtKey) = makeAddrAndKey("delegated");

        // setting new delegate.
        // NOTE: this function is not present in the contract, it is just for testing purposes
        _fighterFarmContract.setDelegatedAddress(delegated);


        // creating signature for the user to 3 challenger and 2 dendroids
        uint8[2] memory numToMint = [3, 2];
        bytes32 messageToSign = keccak256(abi.encode(
            alice,
            numToMint[0],
            numToMint[1],
            _fighterFarmContract.nftsClaimed(alice, 0),
            _fighterFarmContract.nftsClaimed(alice, 1)
        ));

        bytes32 signedMessage = ECDSA.toEthSignedMessageHash(messageToSign);

        (uint8 v, bytes32 r, bytes32 s)  = vm.sign(delegatedPvtKey, signedMessage);


        bytes memory signature = abi.encodePacked(r, s, v);

        string[] memory claimModelHashes = new string[](5);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[3] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        claimModelHashes[4] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](5);
        claimModelTypes[0] = "original";
        claimModelTypes[1] = "original";
        claimModelTypes[2] = "original";
        claimModelTypes[3] = "original";
        claimModelTypes[4] = "original";

        // Right sender of signature should be able to claim fighter
        vm.prank(alice);
        _fighterFarmContract.claimFighters(numToMint, signature, claimModelHashes, claimModelTypes);
    }

</details>

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to do the following changes:

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

Assessed type

DoS

Summary

Lows

---

[L-0] iconsTypes length should also be checked in FighterFarm::redeemMintPass(...) <a id="low-0"></a>

Check for iconsTypes length is missed in the FighterFarm::redeemMintPass(...)` contract. This can cause issues later. Add the check fo that as well

File: 2024-02-ai-arena/src/FighterFarm.sol

243        require(
244            mintpassIdsToBurn.length == mintPassDnas.length &&
245            mintPassDnas.length == fighterTypes.length &&
246            fighterTypes.length == modelHashes.length &&
248            modelHashes.length == modelTypes.length
249        );

GitHub: [243-248]

---

[L-1] customAttributes are not checked for valid ranges when sent using FighterFarm::mintFromMergingPool(...) <a id="low-1"></a>

FighterFarm::_createFighterBase(...) allows only setting attributes like weight and element within specific range. For weight, there is a valid range of 65 - 95. And elements will be selected on the basis of no. of elements set for the generation.

File: 2024-02-ai-arena/src/FighterFarm.sol

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

GitHub: (462-474)

But, these ranges will be implemented only when the value inside the customAttributes is set to 100. If any other value is sent then this condition will be used to set the weight, element, and dna:

File: 2024-02-ai-arena/src/FighterFarm.sol

502        else {
503            element = customAttributes[0];
504            weight = customAttributes[1];
505            newDna = dna;
506        }

GitHub: [502-506]

This allows anyone to set these values to anything. This can cause issues if the values are set outside the specified range.

Recommendations:

Add the neccessary checks for these customAttributes in FighterFarm::_createNewFighter(...)

        else {
+           require(customAttributes[0] < numElements[generation[fighterType]]);
+           require(ustomAttributes[1] >= 65 && ustomAttributes[1] <= 95);
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }

---

[L-2] No check for new Game Item is added in GameItems::createGameItem(...) <a id="low-2"></a>

GameItems::createGameItem(...) allows admin to create new game items. But there is no check for the different parameters in the functions. If wrong data is sent by mistake it can cause issue. For example, dailyAllowance should not be more than the itemsRemaining. It is recommended to add the neccessary checks so taht it will not cause any issue later.

---

[L-3] Add checks for duplicate addresses passed as winners in MergingPool::pickWinner() <a id="low-3"></a>

The MergingPool::pickWinner() function will used by the Admin to pick the winner. But there is no check in the function whether the duplicate address is passed as a winner in the winner paramter. Although the funciton is only admin and he is trusted. But he can still make a mistake and can cause losses to the users by claiming some of the rewards.

File: 2024-02-ai-arena/src/MergingPool.sol

    function pickWinner(uint256[] calldata winners) external {
        require(isAdmin[msg.sender]);
        require(winners.length == winnersPerPeriod, "Incorrect number of winners");
        require(!isSelectionComplete[roundId], "Winners are already selected");
        uint256 winnersLength = winners.length;
        address[] memory currentWinnerAddresses = new address[](winnersLength);
        for (uint256 i = 0; i < winnersLength; i++) {
            currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
            totalPoints -= fighterPoints[winners[i]];
            fighterPoints[winners[i]] = 0;
        }
        winnerAddresses[roundId] = currentWinnerAddresses;
        isSelectionComplete[roundId] = true;
        roundId += 1;
    }

GitHub: [118-132]

It is recommended to add neccessary checks for that.

---

[L-4] Add Array parameter length checks in MergingPool::claimRewards(...) <a id="low-4"></a>

There is no checks in the MergingPool::claimRewards(...) for the length of the array parameters passed. The lengths of the arrays should different array parameters should be matched.

File: 2024-02-ai-arena/src/MergingPool.sol

    function claimRewards(
@>        string[] calldata modelURIs,
@>        string[] calldata modelTypes,
@>        uint256[2][] calldata customAttributes
    )

GitHub: [139-143]

---

[L-5] Neuron::mint(...) contract can't mint tokens to the absolute value of MAX_SUPPLY <a id="low-5"></a>

Neuron::mint(...) contract doesn't allow minting of tokens upto absolute MAX_SUPPLY because this require statement:

File: 2024-02-ai-arena/src/Neuron.sol

    function mint(address to, uint256 amount) public virtual {
@>        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

GitHub: [155-159]

As you can see, it uses < but should be <= so that value upto MAX_SUPPLY is included in the amount. It is recommended to add the following check correctly.

    function mint(address to, uint256 amount) public virtual {
-        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
+        require(totalSupply() + amount <= MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

---

[L-6] RankedBattle::globalStakedAmount is not updated after the end of each round showing incorrect info. <a id="low-6"></a>

When user stake NRN in the RankedBattle contract, the following two mappings store the staked amount info. amountStaked and globalStakedAmount. amountStaked stores the amount of NRN staked token by a specific tokenId and globalStakedAmount stores the overall amount staked. When user loses/wins the tokens in a battle, some part of the his staked tokens is lost/regained from/to StakeAtRisk contract. Also amountStaked mapping is updated based on the result. But globalStakedAmount in not updated (check in RankedBattle::updateBattleRecord(...)). This is because the globalStakedAmount is still considered a valid stake as user can reclaim the lost token if he wins a battle. But at the end of each round, any tokens available in StakeAtRisk contract are permanently lost. That means globalStakedAmount should also be updated on the basis of that. But it is not. That mean globalStakedAmount stores incorrect amount.

It is recommended to update the globalStakedAmount after the end of each round to match with staked amount. This can be done by making the following changes:

File: RankedBattle.sol

    function setNewRound() external {
        require(isAdmin[msg.sender]);
        require(totalAccumulatedPoints[roundId] > 0);
+        globalStakedAmount = globalStakedAmount - _stakeAtRiskInstance.totalStakeAtRisk(roundId);
        roundId += 1;
        _stakeAtRiskInstance.setNewRound(roundId);
        rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];
    }

---

[L-7] Holder of the tokenId will not be to claim rewards in RankedBattle::claimNRN(...) if the NFT is transferred to another address <a id="low-7"></a>

If the fighter NFT is transferred to some other address, then the holder of that NFT will not be able to claim the NRN with that new address. Currently the RankedBattle::claimNRN(...) only calculates the rewards on the basis of the address of the msg.sender, not tokenId.

File: RankedBattle.sol

    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);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

GitHub: [294-311]

This would not be a problem if the user still has access of initial address. But if the user lost the access to the wallet, then he will not be able to claim his rewards for any previous rounds.

It is recommened to calculate the rewards based on the accumulatedPointsPerFighter mapping instead. But this would require whole lot of changes in the function. Make sure those are implemented correctly. Doing this can save the funds of user in various situations.

---

[L-8] RankedBattle::_getStakingFactor() works in a biased way for small stakers. <a id="low-8"></a>

The RankedBattle::_getStakingFactor() function works little bit biased for small stakers. The staking factor calculated for someone who has staked amount like 3.99 NRN is same as who has staked 1 wei amount of NRN. A small staker can take the advantage of this by placing on 1 wei amount of NRN as a staked and can get benefits of staking equal to someone who has staked 3.99 NRN. In this way he will not have to worry about losing any big amount and can still avail for the benefits if win.

Here is a test for PoC:

NOTE: Include the test below in RankedBattle.t.sol File.

    function testStakerCanStakeSmallAmountAndGetTheRewardsLikeNormalBiggerStaker() public {
        // setup
        address alice = makeAddr("Alice");
        address bob = makeAddr("bob");

        // giving tokens to alice and bob
        _fundUserWith4kNeuronByTreasury(alice);
        _fundUserWith4kNeuronByTreasury(bob);

        // checking the balance of alice and bob
        assertEq(_neuronContract.balanceOf(alice), 4_000 * 10 ** 18);
        assertEq(_neuronContract.balanceOf(bob), 4_000 * 10 ** 18);

        // minting fighters to alice and bob
        _mintFromMergingPool(alice);
        _mintFromMergingPool(bob);

        // checking if the fighters are minted
        _mintFromMergingPool(alice);
        _mintFromMergingPool(bob);

        // checking if the fighter has been minted
        assertEq(_fighterFarmContract.ownerOf(0), alice);
        assertEq(_fighterFarmContract.ownerOf(1), bob);

        // alice stakes 3.99 NRN in the pool
        // bob stakes only 1 wei amount of NRN in the pool

        vm.prank(alice);
        _rankedBattleContract.stakeNRN(3.99 ether, 0);

        vm.prank(bob);
        _rankedBattleContract.stakeNRN(1 wei, 1);

        // checking if the tokens has been staked
        assertEq(_rankedBattleContract.amountStaked(0), 3.99 ether);
        assertEq(_rankedBattleContract.amountStaked(1), 1 wei);

        // Let's assume alice wins the battle
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);
        vm.stopPrank();

        // getting the points earned by alice
        uint256 pointsEarnedByAlice = _rankedBattleContract.accumulatedPointsPerAddress(alice,_rankedBattleContract.roundId());

        // Now next round is set
        _rankedBattleContract.setNewRound();

        // Now in this round bob wins
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 0, 0, 1500, true);
        vm.stopPrank();

        // getting the points earned by bob
        uint256 pointsEarnedByBob = _rankedBattleContract.accumulatedPointsPerAddress(bob,_rankedBattleContract.roundId());

        // checking if the points earned by bob and alice are equal
        assertEq(pointsEarnedByAlice, pointsEarnedByBob);
    }

After running this test, you will see that both Alice and Bob won 1500 points even when bob just stake 1 wei of NRN while alice staked almost ~4 wei.

Mitigation

Do not allow for staking small amount of tokens. Add a minimum limit for it.

---

[L-9] Only Success case is checked for calls in the contracts that can create issue if the call fails in some cases. <a id="low-9"></a>

Various calls in the contract only make progress when the transfer call is success, for example:

File: RankedBattle.sol

    function _addResultPoints(
        uint8 battleResult,
        uint256 tokenId,
        uint256 eloFactor,
        uint256 mergingPortion,
        address fighterOwner
    )
        private
    {
            ...


            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
@>                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }
        }
    }

GitHub: [416-500]

As we can see, only when the transfer of token is successfull then the StakeAtRisk::updateAtRiskRecords() is called for the staker. But it will not be called if the call is not successfull. And the function will also not revert. This is good thing in some cases, but in some of them it create issue. Like in the above case, if the call fails then the risk Records of the user will not be updated and that would mean user can get away without paying anything from his account when loses. It is always better to add the case when the call fails and the success is false. Something like revert if not success wherever needed.

---

[L-10] MergingPool::claimRewards(...) can cause DoS if the user won several times but didn't claim the NFT reward immediately <a id="low-10"></a>

MergingPool::claimRewards(...) let user's claim their NFT rewards for the each round ID they won. But it enables them to do so in the same transaction. That mean if the number of claims for the user gets bigger, the function can cause DoS as the function is GAS heavy.

It is recommended to let them claim the rewards for each roundId one by one or specify a max limit that can be claimed in one transaction.

---

[L-11] Various function in FigtherFarm allows for putting modelHash by the User, but it can harm the Game if something Malicious is put into it. <a id="low-11"></a>

It is allowed to add the model Hash by the user. But A user can put the hash of something malicious and can harm the game in some way. Add the proper checks to prevent this from happening.

---

Non Criticals

[N-0] No way to remove a staker in FighterFarm.sol <a id="non-critical-0"></a>

There is only way to add a staker in FighterFarm.sol using addStaker(...) that can stake the fighters on behalf of the Users. But No way to remove it. This might cause an issue if some contract or address is no longer required to be a staker in the contract. Or if some address is mistakenly added as a staker.

Recommendation

Add remove staker function as well:

+    function removeStaker(address staker) external {
+        require(msg.sender == _ownerAddress);
+        hasStakerRole[staker] = false;
+    }

---

[N-1] Mistake in Natspac for MergingPool::fighterPoints mapping. <a id="non-critical-1"></a>

MergingPool::fighterPoints is a mapping from tokenId to fighter points for an NFT. But according to natspac, it is a mapping from address to fighter points which is incorrect.

@>    /// @notice Mapping of address to fighter points.
    mapping(uint256 => uint256) public fighterPoints;

GitHub: [50-52]