AI Arena - juancito's results

Lines of code

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

Vulnerability details

Impact

This breaks core invariants:

• Staked Fighters can be transferred. This allows a losing fighter to be transferred, making the receiver lose points and tokens.
• The MAX_FIGHTERS_ALLOWED limit can be broken

Note: I'm separating this issue from the safeBatchTransferFrom issue on RankedBattle as the functions, contracts, and impacts are different and completely unrelated.

Vulnerability Details

FighterFarm has a function _ableToTransfer that prevents transfering tokens if the fighter is staked, or if the max limit was reached.

This check is implemented on the transferFrom() function, and on the safeTransferFrom(address, address, uint256) function.

The problem is that the ERC721 also contains another version of safeTransferFrom(), with a different interface, as it contains a data attribute. This function is not overriden, which makes it possible to transfer tokens at any time, avoiding the security checks.

This is an example on how an attack would look like:

1. Stake the tokenId with some $NRN to increase the amountStaked[tokenId], so that it calculates points later.
2. Lose a battle
3. Transfer the NFT as described in the bug using safeTransferFrom(address, address, uint256, bytes), before the server executes updateBattleRecord()
4. The server will run updateBattleRecord(), but the owner of the losing token will now be the victim.
5. It may even spend voltage from the victim
6. But most importantly, it will subtract the points from the "lost" battle
7. It will subtract points from the lost battle to the victim which can lead to a losing game, and thus also losing tokens, and the opportunity to win NFTs

Proof of Concept

• Add the test to test/FighterFarm.t.sol
• Run forge test --mt "testTransferringFighterWhileStakedSucceeds"

function testTransferringFighterWhileStakedSucceeds() public {
    // Mint and stake fighter
    _mintFromMergingPool(_ownerAddress);
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);

    // Transferring with `safeTransferFrom(address, address, uint256)` fails
    vm.expectRevert();
    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);

    // Transferring with `safeTransferFrom(address, address, uint256, bytes)` succeeds
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
}

Recommended Mitigation Steps

Override the safeTransferFrom() function with data:

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

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Non-transferable Game Items can be transferred. This breaks the main invariant of soulbound tokens.

It incurs the following impacts:

• It breaks the expected flow of the game, by allowing for example, to transfer batteries in times battery transfers are disabled, or allowing to transfer special soulbound weapons. This gives an unfair advantage to players exploiting this.
• It enables a black market for soulbound tokens which should not exist

Reference from the public Discord channel:

Q: Why do you prevent certain GameItems from transfer, by using transferable?

A: If we wanted to issue a soulbound weapon we want to leave the option available and we also might not want the ability to transfer batteries in a certain period of time.

Note: I'm separating this issue from the safeTransferFrom issue on FighterFarm as the functions, contracts, and impacts are different and completely unrelated.

Vulnerability Details

There are certain Game Items that should not be transferrable.

This can be done on creation for items that should never be transferable, or it can be changed for a certain period to temporary disallow transfers.

This check is enforced on the overriden function safeTransferFrom():

    require(allGameItemAttributes[tokenId].transferable);

GameItems.sol#L301

The problem is that it is still possible to transfer tokens via the inherited ERC1155 safeBatchTransferFrom() function.

Proof of Concept

• Add the test to test/GameItems.t.sol
• Run forge test --mt "testTransferNonTransferableTokens"

function testTransferNonTransferableTokens() public {
    // Create a non-transferable game item
    bool transferable = false;
    _gameItemsContract.createGameItem(
        "Gloves", "https://ipfs.io/ipfs/gloves", false, transferable, 10_000, 1 * 10 ** 18, 10
    );
    
    // Fund user with tokens (using `_ownerAddress` like the rest of the codebase)
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(1, 1);

    // Using `safeTransferFrom()` the token works as expected (not transferred)
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 1, 1, "");

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

    // But using `safeBatchTransferFrom()` the "non-transferable" token can be transferred!
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 0);
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 1);
}

Recommended Mitigation Steps

Override the GameItems::safeBatchTransferFrom() function, adding the require statement:

+    function safeBatchTransferFrom(
+        address _from,
+        address _to,
+        uint256[] calldata _ids,
+        uint256[] calldata _values,
+        bytes calldata _data
+    )
+        external 
+        override(ERC1155)
+    {
+        require(allGameItemAttributes[tokenId].transferable);
+        super.safeBatchTransferFrom(_from, _to, _ids, _values, _data);
+    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

There are two types of fighters: Champions and Dendroids.

Icons are a rare subtype of champions. On the other hand Dendroids are a are a more exclusive class of NFTs, which are rarer than champions.

But it is possible to mint a Dendroid that is also an Icon. This breaks the previously mentioned invariant that an icon is a subtype of champion.

NFTs are valued by their rarity and special traits. In this case, it is possible to mint one with attributes that should not co-exist, which can lead to an unfair pricing in markets, and potential glitches on the game given the unexpected combination. A Medium severity was assessed given the broken invariant and the given reasons.

Vulnerability Details

Users can mint new fighters via redeemMintPass().

They can provide both the fighterType, and the iconsType.

Those values aren't validated in the function, nor on any other internal call (as proved on the POC). That means it is possible to create a Champion (fighterType == 0) that is also an Icon (iconsType > 0).

Reference:

iconsType Type of icons fighter (0 means it's not an icon).

Proof of Concept

• Add the test to test/FighterFarm.t.sol
• Run forge test --mt "testRedeemMintPassIcons"

function testRedeemMintPassDendroidIcon() public {
    // Claim mint pass and approve it to _fighterFarmContract
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c");
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    _mintPassContract.approve(address(_fighterFarmContract), 1);

    // Build parameters 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] = 1;                   // @audit Dendroid
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 1;                   // @audit Icon

    // The fighter was successfully minted as a Dendroid and Icon
    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

    // Verify that the fighter is both a Dendroid and an Icon
    (,,,,,,, uint8 iconsType, bool dendroidBool) = _fighterFarmContract.fighters(0);
    assertEq(dendroidBool, true);
    assertEq(iconsType, 1);
}

Recommended Mitigation Steps

Validate that the minted fighter can't be a Dendroid and an Icon at the same time:

+    if (fighterType == 1 && iconsType > 0) {
+        revert();
+    }

• fighterType == 1 => Dendroid
• iconsType > 0 => Icon

Assessed type

Invalid Validation

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

L-1 - Users redeeming a mint pass can mint Icon fighters with any iconsType, including inexisting ones

Lines of code

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

Vulnerability details

Impact

This creates many inconsistencies on the rerolled NFT, with the following impacts:

• Users can reroll with the other fighterType to have 2x rerolls
• Fighters may roll an element that is not be available for their fighter type
• Champions rerolling as Dendroids will use the attribute probabilities from the Dendroids generation
• Champions rerolling as Dendroids will always have a rarityRank == 0, making sure they will always get the first attribute from the probabilities array

Vulnerability Details

Users can reroll with the other fighterType to have 2x rerolls

The reroll limit is checked using maxRerollsAllowed[fighterType]. But if the fighterType is changed, it will use the limit from the other type, which should not be available.

In other words, it is possible to reroll fighters twice as expected.

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
👉      require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

FighterFarm.sol#L372

Fighters may roll an element that is not be available for their fighter type

Rerolling calls _createFighterBase(). There the element is calculated. The problem is that it will be using the generation from the new fighterType.

numElements has separate arrays for Dendroids and Champions, so it may reroll the element with one from the other list that should not coexist with the current fighter type:

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) private view returns (uint256, uint256, uint256) {
👉      uint256 element = dna % numElements[generation[fighterType]];

Champions rerolling as Dendroids will use the attribute probabilities from the Dendroids generation

Rerolling calls createPhysicalAttributes(). But it will pass the generation from the rerolled fighter type:

    fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
        newDna,
👉      generation[fighterType],
        fighters[tokenId].iconsType,
        fighters[tokenId].dendroidBool
    );

FighterFarm.sol#L385

The generation is used via the dnaToIndex() to calculate the attributes probabilities. So, if the generations differ from each fighter type, it will take the probabilities from another generation.

Champions rerolling as Dendroids will always have a rarityRank == 0, making sure they will always get the first attribute from the probabilities array

Rerolling from a Champion to a Dendroid (fighterType 0 => 1) leads to the newDna being equal to uint256(fighterType) == 1:

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

FighterFarm.sol#L472

That new dna is used to create the phisical attributes, and ultimately used to calculate the rarityRank.

Being dna == 1, amd attributeToDnaDivisor[attributes[i]] > 1, the rarityRank will always be zero:

    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);

AiArenaHelper.sol#L107-L108

This will guarantee that the first element from the probability array will be picked.

Proof of Concept

This test proves that a fighter can be rerolled with a different fighterType.

• Add the test to test/FighterFarm.t.sol
• Run forge test --mt "testRerollDifferentFighterType"

function testRerollDifferentFighterType() public {
    _neuronContract.addSpender(address(_fighterFarmContract));

    // `_mintFromMergingPool()` mints a fighter with `fighterType == 0`
    _mintFromMergingPool(_ownerAddress);
    _fundUserWith4kNeuronByTreasury(_ownerAddress);

    // Verify that the fighter type is zero (Champion, not Dendroid)
    (,,,,,,,,bool dendroidBool) = _fighterFarmContract.fighters(0);
    assertEq(dendroidBool, false);

    // It allows to reroll as a "Dendroid"
    uint8 newFighterType = 1;
    _fighterFarmContract.reRoll(0, newFighterType);
}

Recommended Mitigation Steps

Assign the corresponding fighterType with the contract information. Don't let the user send this data.

-   function reRoll(uint8 tokenId, uint8 fighterType) public {
+   function reRoll(uint8 tokenId) public {
+       uint8 fighterType == fighters[tokenId].dendroidBool ? 1 : 0;
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

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

Assessed type

Invalid Validation

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

L-2 - Precision error in curStakeAtRisk

Lines of code

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

Vulnerability details

Impact

After incrementing the generation for a fighter type, all upcoming mints will only have the element corresponding to index 0.

Assesed as Medium since it breaks a chore mechanic, which can't be fixed or mitigated, since the contract will be bricked.

Vulnerability Details

When minting a new fighter, its corresponding element is calculated as:

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

FighterFarm.sol#L470

The game will work fine for any fighterType on initialization, as numElements[0] = 3; is defined on the constructor.

The problem will arise when the generation is incremented with generation[fighterType] += 1;, which is an expected action at some point in time.

After that, numElements[generation[fighterType]] == numElements[1] == 0, as it is its default value. This will translate the element calculation to:

uint256 element = dna % 0; // @audit-info It will always be 0

The problem is that there isn't any function to update numElements.

So, the element for new minted fighters will always be zero, breaking a chore mechanic used to calculate strengths and weaknesses depending on pairing.

Recommended Mitigation Steps

Create a function to set the numElements for a specific generation.

Assessed type

DoS

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

L-9 - Roles can't be revoked

Lines of code

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

Vulnerability details

Impact

Any weight, and element can be set for a freshly fighter minted via the MergingPool.

Those attributes are crutial for the results of off-chain battles. Out of range values may lead to crashes on the server due to out of index errors, or they may even give an unfair advantage to the fighter (as battle attributes are a function of the weight).

Reference to Docs

Vulnerability Details

The problem is that minting a fighter from FighterFarm::mintFromMergingPool() allows to pass any set of customAttributes[]. Those attributes aren't validated either on the calling function MergingPool::claimRewards().

    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.sol#L329

For any value != 100, the custom attributes will set the corresponding element and weight of the minted fighter.

Note that there is no further validation. The following POC will prove that no validation actually exists for those attributes.

Proof of Concept

• Add the test to test/MergingPool.t.sol
• Run forge test --mt "testClaimRewardsOutOfRangeValues"

function testClaimRewardsOutOfRangeValues() public {
    // Mint two tokens to have two different winners
    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(_DELEGATED_ADDRESS);

    // Pick the two winners
    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;
    _mergingPoolContract.pickWinner(_winners);

    // Set model
    string[] memory _modelURIs = new string[](1);
    string[] memory _modelTypes = new string[](1);
    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _modelTypes[0] = "original";

    // Set INVALID values for element and weight
    uint256[2][] memory _customAttributes = new uint256[2][](1);
    _customAttributes[0][0] = uint256(666666666666666666);  // element
    _customAttributes[0][1] = uint256(9999999999999999999); // weight

    // The minting is done successfully
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    // The new fighter has invalid weight and element attributes
    (,,uint256 weight, uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(2);
    assertEq(weight, 9999999999999999999);
    assertEq(element, 666666666666666666);
}

Recommended Mitigation Steps

Add a validation to check that the element, and weight values are on the expected ranges:

+    require(weight >= 65 && weight <= 95);
+    require(element < numElements[generation[fighterType]]);

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Users can predict the fighter attributes before rerolling them and gain an unfair advantage over their rivals, or reroll an NFT to have the rarest attributes and be worth more.

Vulnerability Details

The DNA for fighters when rerolling is calculated as:

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

FighterFarm.sol#L379

Note that it is dependant on the msg.sender. So, an adversary can precalculate on their computer the outcome of the dna with thousands of addresses from contracts they can create with different salt values. Once they find an address value that generates the best possible dna, they will deploy that contract, transfer the NFT to it and call reRoll().

Fighter NFTs can be transferred, and reRoll() only checks for the current owner, making the attack possible, and letting an adversary gain an unfair advantage over other players.

Note that this is not possible on the other normal minting methods.

• Minting from mintFromMergingPool() makes sure the msg.sender is always the merging pool.
• Minting from claimFighters() requires the msg.sender to be included in a signature.

Recommended Mitigation Steps

Remove the msg.sender from the entropy for the dna. This way nobody can manipulate it.

It will still have the entropy from the tokenId + numRerolls, which will give different outcomes for different tokens.

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

In case additional entropy is desired, the fighters.length can be added like in mintFromMergingPool(), and claimFighters():

-    uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
+    uint256 dna = uint256(keccak256(abi.encode(tokenId, numRerolls[tokenId]), fighters.length));

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

AiArenHelper::attributeProbabilities[] defines the probabilities of getting a rare attribute when minting a Fighter NFT.

There is a bug which leads to these unexpected scenarios:

• -1% of probability for the last element of the array
• +1% of probability for the first element of the array

To give a real-scenario example from the current deployment script, there will be a drop from 4% -> 3% in the probability of the last item for several attributes, making them 25% rarer.

It may even get worse for future generations when the probabilities are updated. For example, if the last attribute is intended to be an ultra rare item with 1% chance, that would actually lead to 0% probability of getting it (due to the bug).

NFTs are usually valued according to the rarity of their attributes. Having an imbalanced distribution will affect the perceived value and thus, the pricing of the different items in the collection. Because of that a Medium severity was assessed.

Vulnerability Details

dnaToIndex() is the function that determines the specific attribute that a user will get when minting a Fighter NFT.

The root of the issue is an off-by-one error in the >= comparison when calculating the corresponding attribute index.

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute)  public view returns (uint256 attributeProbabilityIndex) {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
👉          if (cumProb >= rarityRank) {
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex;
    }

AiArenaHelper.sol#L180

To understand the problem it is important to know beforehand:

• rarityRank works as a random number with a value ranged 0-99. This is because of how it is calculated as x % 100.
• As rarityRank can have 100 possible values (0-99), attrProbabilities should sum up to at most 100 as in the values from the deployment script, representing the probability distribution.

Example 1

To make it simple, let's assume the following value: attrProbabilities = [99, 1]. That means the first attribute has a 99% chance, and the last one only 1%.

On the first iteration cumProb += attrProbabilities[0] will equal 99. This means cumProb >= rarityRank will always be true for any rarityRank in the expected range 0-99. So, in this case the first attribute will always be picked, and the last one will never.

Example 2

Another way of checking it, is considering that the first element always gets a +1% probability. For example with attrProbabilities = [0, 99, 1] the first attribute should never be picked, but it can easily be seen that for rarityRank == 0, the first attribute is still picked.

Note: These cases were picked for easiness to understand, but the issue happens for any other values, including the ones in the deployment script.

Proof of Concept

• Add the test to test/AiArenaHelper.t.sol
• Run forge test --mt "testDnaToIndexRarity"

function testDnaToIndexRarity() public {
    _probabilities[0] = [99, 1];
    _helperContract.addAttributeProbabilities(0, _probabilities);

    // It returns the same item for all possible `rarityRank` values [0-99]
    // The last attribute is never picked
    for (uint8 rarityRank = 0; rarityRank < 100; rarityRank++) {
        assertEq(_helperContract.dnaToIndex(0, rarityRank, "head"), 1);
    }
}

Recommended Mitigation Steps

-    if (cumProb >= rarityRank) {
+    if (cumProb > rarityRank) {
        attributeProbabilityIndex = i + 1;
        break;
    }

Assessed type

Other

QA Report

Low Severity Issues

L-1 - Users redeeming a mint pass can mint Icon fighters with any iconsType, including inexisting ones

Impact

It is possible to mint Icons via the minting pass with any iconsType, including ones that the project was not expecting.

Vulnerability Details

iconsType is used to determine special traits for the fighter:

    if (
        i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
        i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
        i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
    ) {
        finalAttributeProbabilityIndexes[i] = 50;

AiArenaHelper.sol#L100-L105

They are not expected to be any number, but within a given range. There is no validation to prevent that as will be proved on the following POC.

Proof of Concept

• Add the test to test/FighterFarm.t.sol
• Run forge test --mt "testRedeemMintPassIconMaxValue"

function testRedeemMintPassIconMaxValue() public {
    // Claim mint pass and approve it to _fighterFarmContract
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c");
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    _mintPassContract.approve(address(_fighterFarmContract), 1);

    // Build parameters 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] = 255;                   // @audit Icon with max value

    // The fighter was successfully minted with _iconsTypes = 255
    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

    // Verify that the fighter is the Icon 255
    (,,,,,,, uint8 iconsType,) = _fighterFarmContract.fighters(0);
    assertEq(iconsType, 255);
}

Recommendation

• Keep a track of the max possible value for Icons (by setting it via an admin function for example)
• Validate that the minted fighter does not exceed that value

L-2 - Precision error in curStakeAtRisk

curStakeAtRisk will be 0 when amountStaked[tokenId] + stakeAtRisk < 1000:

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

RankedBattle.sol#L439

This will make the player lose 0 points when they lose:

    bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
    if (success) {
        _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
        amountStaked[tokenId] -= curStakeAtRisk;

RankedBattle.sol#L493-L496

But they will still earn points when they win:

    points = stakingFactor[tokenId] * eloFactor;

RankedBattle.sol#L445

Recommendation

Require that the fighter has some stake at risk:

    curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
+   require(curStakeAtRisk > 0);

L-3 - Avoid using _setupRole() directly

Contracts are using the internal _setupRole(), which is discouraged by OpenZeppelin.

Instances: addMinter(), addStaker(), addSpender().

Recommendation

Use _grantRole() instead of _setupRole()

L-4 - Set a range for getFighterPoints()

getFighterPoints() only specifies a maxId. If the amount of tokens is very large, it may revert with an Out of Gas error

Recommendation

Instead of using maxId, set an initial id, and an end id, so that it can be queried in a range

L-5 - Validate fighterType

There are instances where fighterType can be passed to the FighterFarm contract and will fail with an Out of Bounds error when doing generation[fighterType] as generation is a two elements length array, meaning that in those cases fighterType can only have values 0, or 1.

Recommendation

Consider validating the fighterType value to return a proper error message. It also may prevent any possible issue if code is updated.

Instances:

• reRoll()
• _createNewFighter()
• createFighterBase()
• incrementGeneration()

L-6 - DOS of updateModel()

FighterFarm::updateModel() increases the global totalNumTrained by one each time anyone calls the function.

totalNumTrained is declared as a uint32.

A uint32 has a max value of 4_294_967_295 is a considerable number, but an adversary could spend sufficient resources to call the function repeatedly until the max value is reached (especially considering that it will be deployed on Arbitrum, with lower fees than mainnet).

This will lead to a DOS for all other users, as it is a shared global variable, and it would revert due to an overflow.

Recommendation

Declare totalNumTrained with a bigger type (like uint256). There shouldn't be any drawback in doing so.

L-7 - Missing check for iconsTypes.length

Mainly for consistency, as it would fail with Out of Bounds otherwise.

    require(
        mintpassIdsToBurn.length == mintPassDnas.length && 
        mintPassDnas.length == fighterTypes.length && 
        fighterTypes.length == modelHashes.length &&
        modelHashes.length == modelTypes.length
    );

FighterFarm.sol#L243-L248

Recommendation

    require(
        mintpassIdsToBurn.length == mintPassDnas.length && 
        mintPassDnas.length == fighterTypes.length && 
        fighterTypes.length == modelHashes.length &&
+       iconsTypes.length == modelHashes.length &&
        modelHashes.length == modelTypes.length
    );

L-8 - redeemMintPass() allows to precalculate the dna of fighters to be minted

Impact

This can be an unfair advantage for users with minting passes, as they can mint fighters with the best, or most valuable attributes.

Note: Assessed as a Low since it seems to be a business decision, but the impact could be Medium considering the unfair advantage over other players

Vulnerability Details

The dna is calculated as a hash of a mintPassDnas value that is passed by parameter to the function by the user:

uint256(keccak256(abi.encode(mintPassDnas[i]))), 

FighterFarm.sol#L254

This means the user can precalculate the outcome and mint the rarest ones.

Recommendation

Consider limiting the ability of mint pass holders to mint a fighter with any DNA they want, as it is the same as creating a fighter by passing the expected attributes.

One way to achieve this could be to define the mintPassDna on the minting pass contract upon minting of the pass.

L-9 - Roles can't be revoked

Some roles in Neuron.sol can be granted but can't be revoked, as there is not function to do so, there is no assigned admin, nor the DEFAULT_ADMIN_ROLE is configured. Instances: addMinter(), addStaker(), addSpender().

This can be particularly dangerous if some account is compromised, like one with the MINTER_ROLE, which will allow minting tokens to any other user.

Other instances are the GameItems, which assigns a not revokable burning address, and FighterFarm which assigns a staker address (also not revokable).

Recommendation

• Assign admin roles, the DEFAULT_ADMIN_ROLE, or create specific functions to revoke roles for Neuron.sol
• Add a boolean to configure GameItems::setAllowedBurningAddresses() and FighterFarm::addStaker()