AI Arena - vnavascues's results

Lines of code

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

Vulnerability details

Impact

Any fighter (an ERC-721) can be transferred even though the intention of the FighterFarm contract says otherwise (the FighterFarm.fighterStaked mapping value is true for the given token ID).

The most significant vulnerabilities involve the potential loss of data integrity concerning the token and its owner (e.g. points stats, stake at risk) across multiple contracts (e.g. RankedBattle, StakeAtRisk, FighterFarm). This increases the risk of a DoS attack when the game server is updating a battle record (via the updatedBattleRecord function) for the token previosuly transferred.

Description

The FighterFarm contract lacks measures in certain transfer-related functions to enforce the non-transferability status of fighters, allowing transfers even when explicitly disallowed by the contract state (i.e. the fighter is staked):

• safeTransferFrom(address,address,uint256,bytes) (no overriden by FighterFarm, inherited from ERC721): does not implement the _ableToTransfer check.
• safeTransferFrom(address,address,uint256) (overriden by FighterFarm): does implement the _ableToTransfer check.
• transferFrom(address,address,uint256) (overriden by FighterFarm): does implement the _ableToTransfer check.

Therefore, fighters can always be transferred by calling the FighterFarm.safeTransferFrom(address,address,uint256,bytes) function.

Proof Of Concept

Add the following tests in RankedBattle.t.sol:

function testSafeTransferFromDoesNotCheckIfFighterIsStaked() public {
    // Arrange
    address staker = vm.addr(3);
    _mintFromMergingPool(staker);
    _fundUserWith4kNeuronByTreasury(staker);
    assertEq(_neuronContract.balanceOf(staker) >= 4_000 * 10 ** 18, true);
    assertEq(_fighterFarmContract.ownerOf(0), staker);
    assertEq(_neuronContract.hasRole(keccak256("STAKER_ROLE"), address(_rankedBattleContract)), true);
    vm.prank(staker);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
    assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
    assertEq(_fighterFarmContract.fighterStaked(0), true);
    address toAddress = msg.sender;

    // Act - `transferFrom` checks whether fighter is staked
    vm.prank(staker);
    vm.expectRevert();
    _fighterFarmContract.transferFrom(staker, toAddress, 0);

    // Act - `safeTransferFrom(address,address,uint256)` checks whether fighter is staked
    vm.prank(staker);
    vm.expectRevert();
    _fighterFarmContract.safeTransferFrom(staker, toAddress, 0);

    // Act - `safeTransferFrom(address,address,uint256,bytes)` does not check whether fighter is staked
    vm.prank(staker);
    _fighterFarmContract.safeTransferFrom(staker, toAddress, 0, "");

    // Assert
    assertEq(_fighterFarmContract.ownerOf(0), toAddress);
}

Tools Used

Forge tests and manually reviewed.

Recommended Mitigation Steps

Override the FighterFarm.safeTransferFrom(address,address,uint256,bytes) function making sure the token is transferable.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Any game item (an ERC-1155) can be transferred even though the intention of its configuration says otherwise (the GameItemAttributes.transferable member is false).

Description

The GameItems contract lacks measures in certain transfer-related functions to enforce the non-transferability status of items, allowing transfers even when explicitly disallowed by the game attribute configuration:

• safeTransferFrom(address,address,uint256,uint256,bytes) (overriden by GameItems): does implement the GameItemAttributes.transferable check.
• safeBatchTransferFrom(address,address,uint256[],uint256[],bytes) (not overriden by GameItems, inherited from ERC1155): doesn't implement the GameItemAttributes.transferable check.

Therefore, game items can always be transferred by calling the GameItems.safeBatchTransferFrom function.

/// @notice Safely transfers an NFT from one address to another.
/// @dev Added a check to see if the game item is transferable.
function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId,
    uint256 amount,
    bytes memory data
)
    public
    override(ERC1155)
{
    // @audit-issue this check is not present in `safeBatchTransferFrom`
    require(allGameItemAttributes[tokenId].transferable);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

Proof Of Concept

Add the following tests in GameItems.t.sol:

function testSafeBatchTransferFromIsAvailable() public {
    // Arrange
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
    _gameItemsContract.adjustTransferability(0, false);
    (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
    assertEq(transferable, false);

    assertEq(_gameItemsContract.balanceOf(address(this), 0), 2);
    assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 0);

    // Act - safeTransferFrom checks `transferable`
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(address(this), msg.sender, 0, 1, "");

    // Act - safeBatchTransferFrom does not check `transferable`
    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = 0;
    uint256[] memory amounts = new uint256[](1);
    amounts[0] = 1;
    _gameItemsContract.safeBatchTransferFrom(address(this), msg.sender, tokenIds, amounts, "");

    // Assert
    assertEq(_gameItemsContract.balanceOf(address(this), 0), 1);
    assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 1);
}

Tools Used

Forge tests and manually reviewed.

Recommended Mitigation Steps

Override the GameItems.safeBatchTransferFrom function making sure each token is transferable.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A FighterFarm.redeemMintPass caller can forge the following fighter NFT properties of their Mint Pass by arbitrarily set the arguments:

• Fighter type (via the fighterTypes parameter).
• Icon type (via the iconsTypes parameter).
• DNA (via the mintPassDnas parameter).
• ML model hash (via the modelHashes parameter).
• ML model type (via the modelTypes parameter).

Therefore, it is not only possible to redeem as many Dendroid fighters as Mint Passes but also create Champion with cherry picked physical attributes probabilities and other combinations of data not expected by the system.

Description

According to the sponsor and its documentation some fighter traits are more rare than others, for instance fighters of type Dendroid (i.e. fighterType is 1) are more rare than fighter of type Champion (i.e. fighterType is 0) and have particular physical attributes. In fact all Dendroid physical attributes are hard coded to 99 whilst some of the Champion ones are calculated from its DNA and others depend on an icon type.

The lack of validation on the FighterFarm.redeemMintPass arguments against the caller Mint Pass undermines the consideration of rarity for specific fighter traits.

Proof Of Concept

This already implemented test in RankedBattle.t.sol is a good example:

function testRedeemMintPass() public {
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // first i have to mint an nft from the mintpass contract
    assertEq(_mintPassContract.mintingPaused(), false);
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
    assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

    // once owning one i can then redeem it for a fighter
    uint256[] memory _mintpassIdsToBurn = new uint256[](1);
    string[] memory _mintPassDNAs = new string[](1);
    uint8[] memory _fighterTypes = new uint8[](1);
    uint8[] memory _iconsTypes = new uint8[](1);
    string[] memory _neuralNetHashes = new string[](1);
    string[] memory _modelTypes = new string[](1);

    _mintpassIdsToBurn[0] = 1;
    // @audit-issue not only each value can be arbitrarily set but also combinations of them not expected by the system
    _mintPassDNAs[0] = "dna"; // @audit-info an arbitrarily value
    _fighterTypes[0] = 0; // @audit-info an arbitrarily value
    _neuralNetHashes[0] = "neuralnethash"; // @audit-info an arbitrarily value
    _modelTypes[0] = "original"; // @audit-info an arbitrarily value
    _iconsTypes[0] = 1; // @audit-info an arbitrarily value

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

    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

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

Tools Used

Manually reviewed.

Recommended Mitigation Steps

A suggested initial step for mitigation is to establish a mapping system associating a claimed Mint Pass with its immutable fighter data, This mapping can be stored in a designated contract such as AAMintPass or FighterFarm. Then, modify the FighterFarm.redeemMintPass function to retrieve this data from the mapping instead of relying on the parameters passed by the caller.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The system permits the creation or updating of a fighter type (e.g. Champion) with the new DNA and attributes of another type (e.g. Dendroid). The impact of the issue on the AI Arena web server is currently unknown; however, it appears to deviate from the intended behavior of the sytem.

Description

This is a summary of the fighters according to the sponsor and its documentation:

Champion features:

• fighterType: expected to be 0.
• weight: from 65 to 95.
• New DNA: is the original DNA.
• Physical attributes: derived from the new DNA.

Dendroid features:

• More rare to than a Champion.
• fighterType: expected to be 1.
• weight: from 65 to 95.
• New DNA: expected to be uint256(1).
• Physical attributes: expected to be 99 for all attributes.

The FighterFarm.reRoll function doesn't validate the fighterType argument against the token attributes, which allows setting Dendroid data (e.g. DNA, new DNA, element, weight, generation) into a Champion and the other way around.

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 // @audit-issue `fighterType` should be checked against `dendroidBool`
        );
        _tokenURIs[tokenId] = "";
    }
}

The FighterFarm.redeemMintPass function allows to set unsupported fighter types (e.g. 2, 3) in fighterTypes, which have unexpected consequences in the FighterFarm._createNewFighter function due to using different fighter type values across the comparisons:

uint256 newDna = fighterType == 0 ? dna : uint256(fighterType); // @audit-issue comparison against `0`

bool dendroidBool = fighterType == 1; // @audit-issue comparison against `1`

FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
    newDna,
    generation[fighterType],
    iconsType,
    dendroidBool
);

The MergingPool.claimRewards function passes arbitrarily customAttributes into the FighterFarm.mintFromMergingPool function. However, the latter has 0 hard coded as fighterType argument of createNewFighter.

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, // @audit-issue `fighterType` is hard coded as Champion
        0,
        customAttributes // @audit-issue `customAttributes` may contain non-Champion values
    );
}

The lack of validation on the FighterFarm.redeemMintPass arguments against the caller Mint Pass undermines the consideration of rarity for specific fighter traits.

Proof Of Concept

NA

Tools Used

Manually reviewed.

Recommended Mitigation Steps

First, make sure that the FighterFarm.reRoll function validates the fighterType argument against the token dendroidBool attribute. Also, make sure that the FighterFarm._createNewFighter either does not accept unsupported fighterType values or be consistent with the values used on comparisons across the logic (e.g. always use 0)

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

No new fighters (both Champion and Dendroid fighter types) can be created after generation 0 due to a "Division or modulo by 0" revert on the FighterFarm._createFighterBase function. This private function is externally called by:

• FighterFarm.reRoll.
• FighterFarm.claimFighters.
• FighterFarm.redeemMintPass.
• MergingPool.claimRewards.

Description

In the FighterFarm contract:

1. The numElements mapping maps a generation to a number of elements:

/// @notice Mapping of number elements by generation.
mapping(uint8 => uint8) public numElements;

2. The constructor sets the elements of the generation 0 in numElements:

/// @notice Sets the owner address, the delegated address.
/// @param ownerAddress Address of contract deployer.
/// @param delegatedAddress Address of delegated signer for messages.
/// @param treasuryAddress_ Community treasury address.
constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
    ERC721("AI Arena Fighter", "FTR")
{
    _ownerAddress = ownerAddress;
    _delegatedAddress = delegatedAddress;
    treasuryAddress = treasuryAddress_;
    numElements[0] = 3; // @audit here
}

3. The contract has no numElements setter neither an assignment expression that sets numElements on future generations.

4. The _createFighterBase function calculates element of the given fighter type by computing the the modulo operation mod(dna, x), being x the numElements of the current generation for the given fighter type:

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

5. This computation will revert due to a "Division or modulo by 0" for any generation greater than 0.

Proof Of Concept

Add the following tests in FighterFarm.t.sol:

function testClaimFightersRevertForChampionWithGenerationGtZero() public {
    // Arrange
    uint8[2] memory numToMint = [1, 0]; // NB: fighter type is Champion
    bytes memory claimSignature = abi.encodePacked(
        hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
    );
    string[] memory claimModelHashes = new string[](1);
    claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    string[] memory claimModelTypes = new string[](1);
    claimModelTypes[0] = "original";

    // NB: increment Champion generation from 0 to 1
    _fighterFarmContract.incrementGeneration(0);
    assertEq(_fighterFarmContract.generation(0), 1);

    // Act
    vm.expectRevert();
    _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);

    // Assert
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 0);
    assertEq(_fighterFarmContract.totalSupply(), 0);
}

function testClaimFightersRevertForDendroidWithGenerationGtZero() public {
    // Arrange
    uint8[2] memory numToMint = [0, 1]; // NB: fighter type is Dendroid
    bytes memory claimSignature = abi.encodePacked(
        hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
    );
    string[] memory claimModelHashes = new string[](1);
    claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    string[] memory claimModelTypes = new string[](1);
    claimModelTypes[0] = "original";

    // NB: increment Dendroid generation from 0 to 1
    _fighterFarmContract.incrementGeneration(1);
    assertEq(_fighterFarmContract.generation(1), 1);

    // Act
    vm.expectRevert();
    _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);

    // Assert
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 0);
    assertEq(_fighterFarmContract.totalSupply(), 0);
}

Tools Used

Forge tests and manually reviewed.

Recommended Mitigation Steps

Add a numElements setter (with the access controlled) and make sure to properly set it for the upcoming generations. Also consider to do not allow increment generation (via the incrementGeneration function) if numElements for the upcoming generation have not been set yet.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The impact of this issue in the AI Arena web server is unknown, however it is expected that all fighters weight (from Fighter.weight) sits between 65 and 95 (both included).

Description

According to the sponsor a fighter weight must be between 65 and 95 (both included). The MergingPool.claimRewards function does not validate the values in customAttributes and neither does it the FighterFarm.mintFromMergingPool function. Therefore, any caller claiming its rewards can create a fighter with an out of range weight.

Proof Of Concept

Add the following test in MergingPool.t.sol is a good example:

function testClaimRewardsSetsWeightOutOfRange() public {
    // Arrange
    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(_DELEGATED_ADDRESS);
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
    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) == _ownerAddress, true);
    // winner matches ownerOf tokenId
    assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);
    string[] memory _modelURIs = new string[](2);
    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    string[] memory _modelTypes = new string[](2);
    _modelTypes[0] = "original";
    _modelTypes[1] = "original";
    uint256[2][] memory _customAttributes = new uint256[2][](2);
    uint256 weightBelowRange = 64;
    uint256 weightAboveRange = 96;
    _customAttributes[0][0] = uint256(1);
    _customAttributes[0][1] = uint256(weightBelowRange); // @audit-info out of range
    _customAttributes[1][0] = uint256(1);
    _customAttributes[1][1] = uint256(weightAboveRange); // @audit-info out of range
    // winners of roundId 1 are picked
    _mergingPoolContract.pickWinner(_winners);
    // winner claims rewards for previous roundIds

    // Act
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    // Assert
    // @audit-info check whether weight is below the (65,95) range
    (
        uint256 weight2,
        ,
        ,
        ,
        ,
        ,
        ,
        ,
    ) = _fighterFarmContract.fighters(2);
    assertEq(weight2, weightBelowRange);

    // @audit-info check whether weight is above the (65,95) range
    (
        uint256 weight3,
        ,
        ,
        ,
        ,
        ,
        ,
        ,
    ) = _fighterFarmContract.fighters(3);
    assertEq(weight3, weightAboveRange);
}

Tools Used

Manually reviewed.

Recommended Mitigation Steps

Properly validate the customAttributes values in the FighterFarm._createNewFighter function. For instance:

uint256 weight = customAttributes[1];
require(weight >= MIN_FIGHTER_WEIGHT);
require(weight <= MAX_FIGHTER_WEIGHT);

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/MergingPool.sol#L149 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L152

Vulnerability details

Impact

Winners may never successfully claim their rewards due to running out of gass when calling the MergingPool.claimRewards function.

Description

The inefficiency of the MergingPool.claimRewards arises from its double-loop structure. It begins by iterating from numRoundsClaimed[msg.sender] to currentRound < roundId and then proceeds to loop through the list of winners (i.e. winnerAddresses[currentRound][j]) until it finds the corresponding address. This approach becomes notably inefficient when an address has accumulated wins across multiple rounds but has not claimed rewards previously. The function's design results in unnecessary iterations, leading to increased gas consumption and diminished performance for users with a history of unclaimed wins.

Just being the first winner address and claiming the previous round ID reward costs ~840k gas units.

Proof Of Concept

NA

Tools Used

Forge tests for gas estimation and manually reviewed.

Recommended Mitigation Steps

A recommended first step in mitigating this issue is to adjust the MergingPool.claimRewards function to facilitate claiming rewards for individual rounds. Additionally, it is advisable to explore the use of an iterable mapping for efficiently checking if an address has won in a specific round, especially considering the anticipated number of addresses in winnerAddresses per round. The adoption of an iterable mapping not only enhances efficiency but also helps prevent the presence of duplicated addresses.

Assessed type

DoS

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

NC-03 FighterFarm calls to _createNewFighter and _createFighterBase provide weak sources of randomness for dna param

Lines of code

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

Vulnerability details

Impact

Transfering unstaked fighters has the potential to compromise data integrity related to the token and its owner (e.g. points stats, stake at risk) across multiple contracts (e.g. RankedBattle, StakeAtRisk, FighterFarm). This increases the risk of a DoS attack when the game server is updating a battle record (via the updatedBattleRecord function) for the token previosuly transferred.

Description

Any fighter (an ERC-721) can be transferred after as long as it doesn't have any NRN amount staked on it. However, in the RankedBattle.updatedBattleRecord there are statements and assignment statements that use fighter owner data without distinguishing the current and previous owner. This creates an elevated risk of DoS scenarios when the game server updates a battle record due to unexpected circumstances, such as:

• More likely:
  • Scenario 1: there is an arithmetic underflow when reclaiming NRN after winning a battle when the token had certain stake at risk and the new owner hasn't lost yet (see the Proof Of Concept section below):

  if (curStakeAtRisk > 0) {
  // @audit-issue unexpected revert due to `fighterOwner` had 0 amount lost
  _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
  amountStaked[tokenId] += curStakeAtRisk;
  }

  • Scenario 2: there is an arithmetic underflow when calculating the accumulated points per fighter and owner after losing a battle and the new owner has no points on the current round:

  accumulatedPointsPerFighter[tokenId][roundId] -= points;
  // @audit-issue unexpected revert due to `fighterOwner` had 0 points
  accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
  totalAccumulatedPoints[roundId] -= points;

• Less likely:
  • Scenario 3: failure to satisfy the initial RankedBattle.updatedBattleRecord checks (i.e. require()). The new owner voltage could be 0 and expected to be replenished ahead of time (both achievable thanks to the VoltageManager.spendVoltage function lack of checks on voltageSpent):

  // @audit-issue the last two expressions could be falsy for the new fighter owner (involves multiple transfers)
  require(
    !initiatorBool ||
    _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp ||
    _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
  );

Proof Of Concept

This proof of concept showcases the Scenario 1:

1. The Owner1 stakes NRN on the fighter.
2. The game server records a battle loss, creating stake-at-risk on the fighter.
3. The Owner1 unstakes all the NRN on the fighter.
4. The Owner1 transfer the fighter to the Owner2 (which has not played before so there is no registry on the contracts).
5. The game server attempts to record a battle win (that should reclaim some of the NRN at risk).

Add the following tests in RankedBattle.t.sol:

function testUpdateBattleRecordDoSDueToLossUnstakeTransferWin() public {
    // Arrange
    address player = vm.addr(3);
    _mintFromMergingPool(player);
    uint8 tokenId = 0;
    _fundUserWith4kNeuronByTreasury(player);
    vm.prank(player);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
    assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
    uint256 roundId = _rankedBattleContract.roundId();

    // Act - Update a loss battle record that leaves 3 NRN as current stake at risk
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

    // Act - Unstake all the NRN on the token
    uint256 amountStakedAfterLoss = _rankedBattleContract.amountStaked(0);
    vm.prank(player);
    _rankedBattleContract.unstakeNRN(amountStakedAfterLoss, tokenId);

    // Act - Transfer fighter from player to new address
    vm.prank(player);
    _fighterFarmContract.transferFrom(player, address(777), tokenId);

    // Act - Update a win battle record
    vm.prank(address(_GAME_SERVER_ADDRESS));
    vm.expectRevert();
    _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
}

Tools Used

Forge tests.

Recommended Mitigation Steps

This issue is not trivial to address. A recommended starting point for mitigation involves assuming that:

• Tokens can be transferred to and from users who both possess and lack a battle history on the system.
• Owners without a battle record have default values stored, potentially resulting in arithmetic operations that underlow.

Assessed type

DoS

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

L-03 GameItems allowedBurningAddresses cannot be unset Instances

GameItems.sol Description

Function setAllowedBurningAddresses can only grant and never revoke this permission to given addresses since there is a hardcoded true.

Recommended Mitigation Steps

Remove the hardcoded boolean and, similarly to adjustAllowedVoltageSpenders, add a second parameter bool allowed to setAllowedBurningAddresses, to indicate whether the given address should or should not be allowed to burn game items.

L-04 FighterFarm hasStakerRole cannot be unset Instances

FighterFarm.sol Description

Function addStaker can only grant and never revoke this permission to given addresses since there is a hardcoded true.

Recommended Mitigation Steps

Remove the hardcoded boolean and, similarly to adjustAllowedVoltageSpenders, add a second parameter bool allowed to addStaker, to indicate whether the given address should or should not be allowed to stake.

QA Report

Low Risk Issues

L-01 Ownership is transferred in a single step

Instances

• AiArenaHelper.sol
• FighterFarm.sol
• GameItems.sol
• MergingPool.sol
• Neuron.sol
• RankedBattle.sol
• VoltageManager.sol

Description

Function transferOwnership transfers the ownership of the smart contract to a new address in a single transaction. This is risky as errors in specifying the new address can result in unintended consequences.

Recommended Mitigation Steps

Use two-step transactions to set the new owner address, e.g. by integrating OpenZeppelin Ownable2Step.

L-02 Admin is not adjusted when transferring ownership

Instances

• GameItems.sol
• MergingPool.sol
• Neuron.sol
• RankedBattle.sol
• VoltageManager.sol

Description

Contracts that manage administrator permissions always set the _ownerAddress as administrator in the constructor. However, transferOwnership neither revokes the permission from the previous owner, nor sets the new owner as administrator.

All these contracts include a function adjustAdminAccess for managing administrator permissions, but decoupling this from when ownership is transferred may lead to errors.

Recommended Mitigation Steps

Update isAdmin in transferOwnership, e.g. by setting the current (soon to be previous) owner address to false and the new owner to true.

L-03 GameItems allowedBurningAddresses cannot be unset

Instances

• GameItems.sol

Description

Function setAllowedBurningAddresses can only grant and never revoke this permission to given addresses since there is a hardcoded true.

Recommended Mitigation Steps

Remove the hardcoded boolean and, similarly to adjustAllowedVoltageSpenders, add a second parameter bool allowed to setAllowedBurningAddresses, to indicate whether the given address should or should not be allowed to burn game items.

L-04 FighterFarm hasStakerRole cannot be unset

Instances

• FighterFarm.sol

Description

Function addStaker can only grant and never revoke this permission to given addresses since there is a hardcoded true.

Recommended Mitigation Steps

Remove the hardcoded boolean and, similarly to adjustAllowedVoltageSpenders, add a second parameter bool allowed to addStaker, to indicate whether the given address should or should not be allowed to stake.

L-05 FighterFarm redeemMintPass can revert due to iconsTypes access out-of-bounds

Instances

• FighterFarm.sol

Description

Function redeemMintPass requires that all input parameters of type array have the same length except for iconsTypes. After that, the function traverses all the arrays in a single for loop.

Since the function missed validating the length of iconsTypes, it could revert unexpectedly when attempting to access an index out-of-bounds.

Recommended Mitigation Steps

Update the require L243 so that it also checks iconsTypes.length.

L-06 FighterFarm updateModel always increments stats, even when receiving same model data

Instances

• FighterFarm.sol

Description

Function updateModel always increments numTrained[tokenId] and totalNumTrained (+1). It skips validating whether modelHash and modelType params are different from data currently in storage. This could be used by players to their advantage.

Recommended Mitigation Steps

Validate modelHash and modelType: compare given params against storage and only increment numTrained[tokenId] and totalNumTrained if there are differences.

L-07 VoltageManager spendVoltage not protected against underflow

Instances

• VoltageManager.sol

Description

Function spendVoltage skips validating whether ownerVoltage[spender] is greater or equal than voltageSpent. If voltageSpent is greater, the subtraction operation L110 will cause a revert due to an underflow error.

Recommended Mitigation Steps

Validate that ownerVoltage[spender] is greater or equal than voltageSpent before performing the subtraction.

L-08 Neuron setupAirdrop can interfere with existing approvals

Instances

• Neuron.sol

Description

The Neuron contract might already have certain approvals set for specific addresses. Initiating an airdrop directly from the Neuron contract could potentially override or interfere with these existing approvals, leading to unexpected behavior.

Recommended Mitigation Steps

Move the airdrop logic onto a separate contract.

L-09 Neuron NRN MAX_SUPPLY can never be minted

Instances

• Neuron.sol

Description

Function mint includes a condition requiring totalSupply() + amount to be lesser than MAX_SUPPLY, which prevents from minting all the NRN supply.

Recommended Mitigation Steps

Use <= operator instead of < so that all NRN supply can be minted.

L-10 RankedBattle unstakeNRN performs state changes for amount 0

Instances

• RankedBattle.sol

Description

Function unstakeNRN skips validating whether the amount of NRN tokens to unstake is not 0. However, still performs a number of state updates, setting e.g.:

• stakingFactor[tokenId]
• _calculatedStakingFactor[tokenId][roundId] = true
• hasUnstaked[tokenId][roundId] = true

May also emit events, like Unstaked.

Recommended Mitigation Steps

Validate that the amount of NRN tokens to unstake is not 0.

L-11 RankedBattle functions not protected against division by zero

Instances

• RankedBattle.sol claimNRN
• RankedBattle.sol getUnclaimedNRN

Description

Both functions can revert. This is because totalAccumulatedPoints for the given round id can be 0, resulting in a division by zero scenario.

Recommended Mitigation Steps

Validate that totalAccumulatedPoints[roundId] is not 0 before performing the division.

L-12 RankedBattle updateBattleRecord performs state changes for any battleResult

Instances

• RankedBattle.sol

Description

According to _updateRecord, battleResult can only adopt 3 values: 0, 1, 2.

Function updateBattleRecord skips validating battleResult, which results in a number of state updates (points, voltage and totalBattles) that may not be correct.

Recommended Mitigation Steps

Validate that battleResult is within range.

Non-Critical Issues

NC-01 AiArenaHelper attributes are different from docs

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

Description

Contract attributes are "head", "eyes", "mouth", "body", "hands", "feet".

Documentation attributes (see The Skin) are "hair", "eyes", "mouth", "body", "hands", "feet".

Notice HEAD vs HAIR.

Recommended Mitigation Steps

Review and update fighter attributes accordingly.

NC-02 AiArenaHelper constructor could call addAttributeDivisor

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

Description (and Mitigation)

Contract constructor could call addAttributeDivisor external function on the same contract, instead of updating attributeToDnaDivisor mapping. In which case, the function should be made public.

NC-03 FighterFarm calls to _createNewFighter and _createFighterBase provide weak sources of randomness for dna param

Instances

• FighterFarm.sol claimFighters
• redeemMintPass
• mintFromMergingPool
• reRoll

Description

Fighters dna is supposed to be random and unique.

To generate fighters dna functions above use Solidity keccak256 to calculate a hash based on different parameters. The issue is that these parameters are all deterministic and predictable, and as a result dna lacks the inherent unpredictability associated with true randomness.

Recommended Mitigation Steps

Improve dna generation and use a better source of randomness, e.g. ChainLink VRF.

NC-04 FighterFarm updateModel can update model data anytime

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

Description

Function updateModel can be called to update the fighters modelHash and modelType anytime. This could be used by players to their advantage.

Recommended Mitigation Steps

Change updateModel so that modelHash and modelType can only be updated under certain conditions, e.g. after a timelock, after a number of wins, etc.

NC-05 GameItems bool in struct GameItemAttributes could be isTransferable

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

Description (and Mitigation)

Use isTransferable, instead of transferable.

NC-06 GameItems _mint hardcoded data

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

Description (and Mitigation)

Review hardcoded string random in _mint call.

NC-07 Functions instantiate...Contract have a misleading name

Instances

• FighterFarm.sol instantiateAIArenaHelperContract
• FighterFarm.sol instantiateMintpassContract
• FighterFarm.sol instantiateNeuronContract
• GameItems.sol instantiateNeuronContract
• RankedBattle.sol instantiateNeuronContract
• RankedBattle.sol instantiateMergingPoolContract

Description (and Mitigation)

Functions above do not create a new contract/instance, they only create a reference. Consider updating function names to e.g. setNeuronContract.

NC-08 RankedBattle setStakeAtRiskAddress performs redundant step

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

Description (and Mitigation)

It is redundant to store a reference to the StakeAtRisk Contract (L195) and the address (L194). Consider removing the address.

NC-09 StakeAtRisk not providing consistent feedback when reverting

Instances

• StakeAtRisk.sol setNewRound
• reclaimNRN
• updateAtRiskRecords

Description (and Mitigation)

Functions above validate msg.sender == _rankedBattleAddress, but provide mixed feedback. Consider reverting with the same message when checking for a particular condition.

NC-10 VoltageManager useVoltageBattery uses hardcoded tokenId

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

Description

Function useVoltageBattery uses a hardcoded 0 when referring to battery tokenId:

require(_gameItemsContractInstance.balanceOf(msg.sender, 0) > 0);
_gameItemsContractInstance.burn(msg.sender, 0, 1);

This will work for as long as the deployment script keeps creating battery as the first game item.

Recommended Mitigation Steps

Update the implementation so that it does not rely on hardcoded information.

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