AI Arena - cartlex_'s results

Lines of code

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

Vulnerability details

Impact

• fighter can be transferred despite it currently staked.
• user can have more than 10 fighers despite the restriction in _ableToTransfer function.

Proof of Concept

FighterFarm contract is inherited from ERC721 contract. In the ERC721 token contract there are methods which allow users to transfer tokens - safeTransferFrom and safeTransferFrom with data parameter.

In FighterFarm contract the first method is overriden and additional check is implemented to not allow users to transfer currently staked fighter.

    require(_ableToTransfer(tokenId, to));
    _safeTransfer(from, to, tokenId, "");

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

_ableToTransfer function restrict users from transferring currently staked fighters, moreover it limits possession to more than MAX_FIGHTERS_ALLOWED which is equal to 10 fighters.

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

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

However, it is still possible to transfer a fighter via second safeTransferFrom function.

Below tests show these issues.

1. It's possible to transfer a fighter that is currently staked via safeTransferFrom function with data parameter.
2. It's possible for user to have more than 10 fighters.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {FighterFarm} from "../src/FighterFarm.sol";
import {Neuron} from "../src/Neuron.sol";
import {AAMintPass} from "../src/AAMintPass.sol";
import {MergingPool} from "../src/MergingPool.sol";
import {RankedBattle} from "../src/RankedBattle.sol";
import {VoltageManager} from "../src/VoltageManager.sol";
import {GameItems} from "../src/GameItems.sol";
import {AiArenaHelper} from "../src/AiArenaHelper.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

/// @notice Tests to show issues with transfers of fighters.
contract FighterTransferredTest is Test {

    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    /*//////////////////////////////////////////////////////////////
                             CONTRACT INSTANCES
    //////////////////////////////////////////////////////////////*/

    FighterFarm internal _fighterFarmContract;
    AAMintPass internal _mintPassContract;
    MergingPool internal _mergingPoolContract;
    RankedBattle internal _rankedBattleContract;
    VoltageManager internal _voltageManagerContract;
    GameItems internal _gameItemsContract;
    AiArenaHelper internal _helperContract;
    Neuron internal _neuronContract;

    function getProb() public {
        _probabilities.push([25, 25, 13, 13, 9, 9]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 10]);
        _probabilities.push([25, 25, 13, 13, 9, 23]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 3]);
    }

    /*//////////////////////////////////////////////////////////////
                                SETUP
    //////////////////////////////////////////////////////////////*/

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);
        getProb();

        _fighterFarmContract = new FighterFarm(_ownerAddress, _DELEGATED_ADDRESS, _treasuryAddress);

        _helperContract = new AiArenaHelper(_probabilities);

        _mintPassContract = new AAMintPass(_ownerAddress, _DELEGATED_ADDRESS);
        _mintPassContract.setFighterFarmAddress(address(_fighterFarmContract));
        _mintPassContract.setPaused(false);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);

        _voltageManagerContract = new VoltageManager(_ownerAddress, address(_gameItemsContract));

        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _rankedBattleContract = new RankedBattle(
            _ownerAddress, address(_fighterFarmContract), _DELEGATED_ADDRESS, address(_voltageManagerContract)
        );

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));

        _mergingPoolContract =
            new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContract));

        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
        _fighterFarmContract.instantiateAIArenaHelperContract(address(_helperContract));
        _fighterFarmContract.instantiateMintpassContract(address(_mintPassContract));
        _fighterFarmContract.instantiateNeuronContract(address(_neuronContract));
        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
    }

    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to mint an fighter nft to an address.
    function _mintFromMergingPool(address to) internal {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(to, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    }

    function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
        // Handle the token transfer here
        return this.onERC721Received.selector;
    }

    /*//////////////////////////////////////////////////////////////
                                AUDIT TESTS
    //////////////////////////////////////////////////////////////*/
    
    /// @notice It's possible to transfer currently staked fighter via `safeTransferFrom` with data parameter.
    function test_TransferOfCurrentlyStakedFighter() public {
        // owner address has staker role
        _fighterFarmContract.addStaker(_ownerAddress);
        
        // fighter minted to owner address
        _mintFromMergingPool(_ownerAddress);

        // owner staked its fighter
        _fighterFarmContract.updateFighterStaking(0, true);

        // fighter currently staked
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        // owner of fighter with 0 index is _ownerAddress
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        vm.expectRevert();
        // figther can't be transferred via `safeTransferFrom` without data parameter
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);

        // owner successfully transferred currently staked fighter to another address via `safeTransferFrom` with data parameter
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");

        // another address successfully received a fighter
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

    /// @notice User can have more than 10 fighters if fighter transferred via `safeTransferFrom` with data parameter.
    function test_UserCanHaveMoreThan10Fighters() public {
        address user = makeAddr("user");

        // 10 fighters minted to user address
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        _mintFromMergingPool(user);
        assertEq(_fighterFarmContract.balanceOf(user), 10);

        // it's not possible to have more than 10 fighters for each user due to restriction in `_ableToTransfer` function.
        vm.expectRevert();
        _mintFromMergingPool(user);

        // owner address mints fighter for himself
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);

        // owner successfully transferred fighter to user address via `safeTransferFrom` with data parameter
        _fighterFarmContract.safeTransferFrom(_ownerAddress, user, 10, "");

        // user now has more than 10 fighters
        assertEq(_fighterFarmContract.balanceOf(user), 11);
    }
}

Tools Used

Foundry, manual review.

Recommended Mitigation Steps

To mitigate these issues consider to override safeTransferFrom function with data parameter in FighterFarm contract, as it does below.

Another mitigation will be to revert when call this function.

function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    bytes memory data
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to)); // @audit add require statement to not allow transfers.

    // revert("Transfers not allowed"); // @audit or use `revert`.

    _safeTransfer(from, to, tokenId, data);
}

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

• protocol restriction avoidance. User can transfer token even if bool transferable set to false.
• user can mint items for another account and then transfer them to himself, therefore bypass allowanceRemaining parameter.

Proof of Concept

GameItem contract is inherited from ERC1155 contract, therefore there is a safeBatchTransferFrom method in it.

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

When game Item is creating it's possible to make it non-transferable in createGameItem function or to change transferability in adjustTransferability function as well. When transferable set to false it means that users can't transfer item.

However, there is no require statement in safeBatchTransferFrom function to check if the token is transferable or not, therefore any user can call safeBatchTransferFrom function to transfer game item to another account. This issue allow users to bypass allowanceRemaining check in mint function.

Possible exploit scenario:

• user mint 10 tokens from first account and can't mint more due to allowanceRemaining restriction.
• user mint 10 tokens from second account.
• user transfer 10 tokens from second account to first via safeBatchTransferFrom.

Below is the proof of concept that show the issue.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console, stdError} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {GameItems} from "../src/GameItems.sol";
import {Neuron} from "../src/Neuron.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";

contract GameItemsTest is Test {
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    GameItems internal _gameItemsContract;
    Neuron internal _neuronContract;

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);
        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _neuronContract.addSpender(address(_gameItemsContract));

        _gameItemsContract.instantiateNeuronContract(address(_neuronContract));
        _gameItemsContract.createGameItem("Battery", "https://ipfs.io/ipfs/", true, true, 10_000, 1 * 10 ** 18, 10);
    }

    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to fund an account with 4k $NRN tokens.
    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 4_000 * 10 ** 18);
        assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

    function onERC1155Received(address, address, uint256, uint256, bytes memory) public pure returns (bytes4) {
        return this.onERC1155Received.selector;
    }

    /*//////////////////////////////////////////////////////////////
                                AUDIT TEST
    //////////////////////////////////////////////////////////////*/

    /// @notice Test shows how user can transfer token that is non-transferable
    function testTransferability() public {
        // mints neuron to _ownerAddress
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        // token minted to _ownerAddress
        _gameItemsContract.mint(0, 1);

        // _ownerAddress has 1 token
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

        // _DELEGATED_ADDRESS has 0 tokens
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);

        // change transferability to false, therefore token can't be transferable
        _gameItemsContract.adjustTransferability(0, false);

        vm.expectRevert();
        // user can't transfer token due to `transferable == false`
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // user can transfer token that is not transferrable via `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, "");

        // _DELEGATED_ADDRESS has 1 token
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);

        // _ownerAddress has 0 tokens
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }
}

Tools Used

Foundry, manual review.

Recommended Mitigation Steps

To mitigate these issue consider to override safeBatchTransferFrom function in GameItems contract and check that each token id is transferable as it does in safeTransferFrom or revert to not allow users to call this function.

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override(ERC1155) {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );

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

        // revert("batch transfer not allowed"); // @audit or just revert to not allow call this function.
        
        _safeBatchTransferFrom(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#L110 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L131

Vulnerability details

Impact

• functions that call _createFighterBase will revert in the case of generation value was incremented, therefore fighters can't be created.

Proof of Concept

numElements is used to determine the number of elements by generation. When FighterFarm contract is deployed it sets to 3 for zero generation.

/// @notice Mapping of number elements by generation.
numElements[0] = 3;

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

Then it used in _createFighterBase function to calculate the element.

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

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

It's also possible for admin to change the number of generation via incrementGeneration function.

generation[fighterType] += 1;

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

There is no function to set numElements depending on generation.

Therefore, possible bug scenario:

• admin deployed the contract generation == 0, numElements[0] = 3.
• an admin decide to increment the generation using incrementGeneration, generation == 1, numElements[1] = 0.
• when _createFighterBase function calls it will always revert due to division by zero.

uint256 element = dna % numElements[generation[fighterType]] => dna % numElemets[1] => dna % 0 => zero division.

Proof of concept with 4 tests that show the issue:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console, stdError} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {FighterFarm} from "../src/FighterFarm.sol";
import {Neuron} from "../src/Neuron.sol";
import {AAMintPass} from "../src/AAMintPass.sol";
import {MergingPool} from "../src/MergingPool.sol";
import {RankedBattle} from "../src/RankedBattle.sol";
import {VoltageManager} from "../src/VoltageManager.sol";
import {GameItems} from "../src/GameItems.sol";
import {AiArenaHelper} from "../src/AiArenaHelper.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

contract FighterFarmZeroDivisionTest is Test {

    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    /*//////////////////////////////////////////////////////////////
                             CONTRACT INSTANCES
    //////////////////////////////////////////////////////////////*/

    FighterFarm internal _fighterFarmContract;
    AAMintPass internal _mintPassContract;
    MergingPool internal _mergingPoolContract;
    RankedBattle internal _rankedBattleContract;
    VoltageManager internal _voltageManagerContract;
    GameItems internal _gameItemsContract;
    AiArenaHelper internal _helperContract;
    Neuron internal _neuronContract;

    function getProb() public {
        _probabilities.push([25, 25, 13, 13, 9, 9]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 10]);
        _probabilities.push([25, 25, 13, 13, 9, 23]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 3]);
    }

    /*//////////////////////////////////////////////////////////////
                                SETUP
    //////////////////////////////////////////////////////////////*/

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);
        getProb();

        _fighterFarmContract = new FighterFarm(_ownerAddress, _DELEGATED_ADDRESS, _treasuryAddress);

        _helperContract = new AiArenaHelper(_probabilities);

        _mintPassContract = new AAMintPass(_ownerAddress, _DELEGATED_ADDRESS);
        _mintPassContract.setFighterFarmAddress(address(_fighterFarmContract));
        _mintPassContract.setPaused(false);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);

        _voltageManagerContract = new VoltageManager(_ownerAddress, address(_gameItemsContract));

        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _rankedBattleContract = new RankedBattle(
            _ownerAddress, address(_fighterFarmContract), _DELEGATED_ADDRESS, address(_voltageManagerContract)
        );

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));

        _mergingPoolContract =
            new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContract));

        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
        _fighterFarmContract.instantiateAIArenaHelperContract(address(_helperContract));
        _fighterFarmContract.instantiateMintpassContract(address(_mintPassContract));
        _fighterFarmContract.instantiateNeuronContract(address(_neuronContract));
        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
    }

    /*//////////////////////////////////////////////////////////////
                                AUDIT TESTS
    //////////////////////////////////////////////////////////////*/

    /// @notice Division by zero in `reRoll` function after generation was incremented.
    function test_reRoll_ZeroDivision() public {
        _mintFromMergingPool(_ownerAddress);

        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        
        // after successfully minting a fighter, update the model
        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
            uint8 tokenId = 0;
            uint8 fighterType = 0;

            _neuronContract.addSpender(address(_fighterFarmContract));
            _fighterFarmContract.reRoll(tokenId, fighterType);
            assertEq(_fighterFarmContract.numRerolls(0), 1);
            assertEq(neuronBalanceBeforeReRoll > _neuronContract.balanceOf(_ownerAddress), true);

            // numElements[0] == 3 after contract is deployed
            assertEq(_fighterFarmContract.numElements(0), 3);

            // An admin increments a generation
            _fighterFarmContract.incrementGeneration(0);

            // Generation is equal to 1
            assertEq(_fighterFarmContract.generation(1), 0);

            // numElements[1] == 0
            assertEq(_fighterFarmContract.numElements(1), 0);

            vm.expectRevert(stdError.divisionError);
            // Division by zero after generation was incremented
            _fighterFarmContract.reRoll(tokenId, fighterType);
        }
    }

    /// @notice Division by zero in `redeemMintPass` function after generation was incremented.
    function test_redeemMintPass_ZeroDivision() public {
        // numElements[0] == 3 after contract is deployed
        assertEq(_fighterFarmContract.numElements(0), 3);

        // An admin increments a generation
        _fighterFarmContract.incrementGeneration(0);

        // Generation is equal to 1
        assertEq(_fighterFarmContract.generation(1), 0);

        // numElements[1] == 0
        assertEq(_fighterFarmContract.numElements(1), 0);

        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";
        _fighterTypes[0] = 0;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

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

        vm.expectRevert(stdError.divisionError);
        // Division by zero after generation was incremented
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
        );
    }

    /// @notice Division by zero in `mintFromMergingPool` function after generation was incremented.
    function test_MintFromMergingPool_ZeroDivision() public {
        // numElements[0] == 3 after contract is deployed
        assertEq(_fighterFarmContract.numElements(0), 3);

        // An admin increments a generation
        _fighterFarmContract.incrementGeneration(0);

        // Generation is equal to 1
        assertEq(_fighterFarmContract.generation(1), 0);

        // numElements[1] == 0
        assertEq(_fighterFarmContract.numElements(1), 0);

        vm.expectRevert(stdError.divisionError);
        // Division by zero after generation was incremented
        _mintFromMergingPool(_ownerAddress);
    }

    /// @notice Division by zero in `claimFighters` function after generation was incremented.
    function test_ClaimFighters_ZeroDivision() public {
        // numElements[0] == 3 after contract is deployed
        assertEq(_fighterFarmContract.numElements(0), 3);

        // An admin increments a generation
        _fighterFarmContract.incrementGeneration(0);

        // Generation is equal to 1
        assertEq(_fighterFarmContract.generation(1), 0);

        // numElements[1] == 0
        assertEq(_fighterFarmContract.numElements(1), 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";

        vm.expectRevert(stdError.divisionError);
        // Division by zero after generation was incremented
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
    }

    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to mint an fighter nft to an address.
    function _mintFromMergingPool(address to) internal {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(to, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
    }

    /// @notice Helper function to fund an account with 4k $NRN tokens.
    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 4_000 * 10 ** 18);
        assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

    function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
        // Handle the token transfer here
        return this.onERC721Received.selector;
    }
}

To run the tests use:

forge test --mc FighterFarmZeroDivisionTest -vvvv

Tools Used

Foundry, manual review.

Recommended Mitigation Steps

Consider to implement a function that allows an admin to set value to numElements mapping depending on generation value.

Assessed type

Other

[L-01] mint function is useless if dailyAllowance set to 0.

Description

In GameItems contract when game item is creating it's possible to set dailyAllowance value to 0.

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

Due to this mint function will always revert if quantity != 0.

Moreover, mint function do nothing with quantity == 0

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

Recommendation

1. Consider to add require statement in createGameItem function.

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 
        public 
    {
++      require(dailyAllowance != 0, "InvalidAllowance");
        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
                transferable,
                itemsRemaining,
                itemPrice,
                dailyAllowance
            )
        );
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

2. Add require that quantity != 0 in mint function.

function mint(uint256 tokenId, uint256 quantity) external {
        require(tokenId < _itemCount);
++      require(quantity != 0, "Invalid amount");
    ...
}

[L-02] Direct call to spendVoltage is useless.

Description

In VoltageManager user can directly call spendVoltage function. However it does nothing and only spend user's voltage.

function spendVoltage(address spender, uint8 voltageSpent) public {
        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]); // @audit user can call it directly
        if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
            _replenishVoltage(spender);
        }
        ownerVoltage[spender] -= voltageSpent; 
        emit VoltageRemaining(spender, ownerVoltage[spender]);
    }

Recommendation

Consider to restrict users from calling this function and allow only RankedBattle contract to call it.

function spendVoltage(address spender, uint8 voltageSpent) public {
--      require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
++      require(spender == address(_rankedBattleAddress), "Only ranked battle can call");
        if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
            _replenishVoltage(spender);
        }
        ownerVoltage[spender] -= voltageSpent; 
        emit VoltageRemaining(spender, ownerVoltage[spender]);
    }

[NC-01] Lack of fighterType check in incrementGeneration function.

Description

There are only two types of fighter in the game. However, there is no check that fighterType is equal 0 or 1 in incrementGeneration function.

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

Recommendation

Consider to add check that fighterType is equal 0 or 1.

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

[NC-02] Lack of zero divisor check.

Description

There is no check than input attribute divisor in not equal to zero.

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L73-L74

It could lead to zero division error in createPhysicalAttributes function, when rarityRank is calculating.

Recommendation

Consider to additional check that attribute divisor is not equal to zero.

function addAttributeDivisor(uint8[] memory attributeDivisors) external {
        require(msg.sender == _ownerAddress);
        require(attributeDivisors.length == attributes.length);

        uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
++          require(attributeDivisors[i] != 0, "InvalidDivisor");
            attributeToDnaDivisor[attributes[i]] = attributeDivisors[i];
        }
    }   

[NC-03] Lack of iconsTypes array length check.

Description

In FighterFarm contract inside the comments to redeemMintPass function says:

Each input array must correspond to the same index, i.e., the first element in each 

However, there is no check for iconsTypes array length.

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

Recommendation

Consider to add additional check for iconsTypes array length.

function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes, // @audit no check for this array and no natspec for it
        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 &&
++          modelTypes.length == iconsTypes.length
        );

[NC-04] Unsafe casting to uint16.

Description

In claimFighters function there is an unsafe calculation of totalToMint variable.

uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);

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

Function can revert if the sum numToMint array elements is greater than 255.

Recommendation

Consider to implement the next changes:

--  uint16 totalToMint = uint16(numToMint[0] + numToMint[1]); 
++  uint16 totalToMint = (uint16(numToMint[0]) + uint16(numToMint[1]));

[NC-05] No boundary for setting bpsLostPerLoss value.

Description

In RankedBattle an admin can set bpsLostPerLoss using setBpsLostPerLoss function.

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L226-L229

However, there is no any boundary for the bpsLostPerLoss value, therefore it can be any value and curStakeAtRisk can be more than user expects.

    /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
    curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

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

Recommendation

Consider to implement the next changes:

function setBpsLostPerLoss(uint256 bpsLostPerLoss_) external {
    require(isAdmin[msg.sender]);
++  require(bpsLostPerLoss_ <= 10_000, "InvalidBpsValue");
    bpsLostPerLoss = bpsLostPerLoss_;
}

[NC-06] Comment is misleading.

Description

In the constructor of Neuron contract there is a comment:

/// @notice Grants roles to the ranked battle contract. 

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

However, it doesn't grant the role to the RankedBattle contract during the contruction. As can see from the RankedBattle.t.sol, roles granted via addStaker and addMinter functions.

_neuronContract.addStaker(address(_rankedBattleContract));
_neuronContract.addMinter(address(_rankedBattleContract));

Recommendation

Consider to remove this commentary.

--  /// @notice Grants roles to the ranked battle contract. 

[NC-07] Dna will always consist of MergingPool address when mintFromMergingPool function is used.

Description

It's possible to create a fighter via mintFromMergingPool function. This function internally calls _createNewFighter function and pass dna as a parameter.

To calculate the dna it uses the next formula:

uint256(keccak256(abi.encode(msg.sender, fighters.length))),

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

As can seen it used msg.sender i.e. address of MergingPool contract. However, other functions that call _createNewFighter use user as msg.sender.

Recommendation

Consider to implement the next changes.

--  uint256(keccak256(abi.encode(msg.sender, fighters.length))),
++  uint256(keccak256(abi.encode(to, fighters.length))),