AI Arena - shaka's results

In AI Arena you train an AI character to battle in a platform fighting game. Imagine a cross between PokΓ©mon and Super Smash Bros, but the characters are AIs, and you can train them to learn almost any skill in preparation for battle.

General Information

Platform: Code4rena

Start Date: 09/02/2024

Pot Size: $60,500 USDC

Total HM: 17

Participants: 283

Period: 12 days

Judge:

Id: 328

League: ETH

AI Arena

Findings Distribution

Researcher Performance

Rank: 143/283

Findings: 9

Award: $15.02

🌟 Selected for report: 0

πŸš€ Solo Findings: 0

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L355-L365

Vulnerability details

Fighter NFTs should only be transferable if they are not staked and the recipient of the transfer has not reached the maximum number of fighters allowed.

The FighterFarm contract ensures this is the case by overriding the transferFrom(address from, address to, uint256 tokenId) and safeTransferFrom(address from, address to, uint256 tokenId) functions from the ERC721 contract, adding the necessary checks.

However, the inherited ERC721 contract also exposes a safeTransferFrom function with the additional parameter data, and this function is not overridden in the FighterFarm contract. This makes it possible to bypass the _ableToTransfer check by calling safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data).

This can cause many unwanted side effects. For one, the maximum number of fighters allowed can be exceeded. Another issue is that a fighter owner could transfer the fighter to another address to avoid losing points by making the updateBattleRecord function revert due to an underflow error (see PoC).

Impact

Fighter owners can bypass the _ableToTransfer check and transfer fighters to other addresses, potentially causing unwanted side effects like exceeding the maximum number of fighters allowed or avoiding losing points.

Proof of Concept

Add the following code to the contract RankedBattle.t.sol and run forge test --mt testTransferFighterStaked:

function testTransferFighterStaked() public {
    address alice = makeAddr("alice");
    address alice2 = makeAddr("alice2");

    _fundUserWith4kNeuronByTreasury(alice);
    _mintFromMergingPool(alice);
    uint8 tokenId = 0;
    
    // Alice stakes NRN for fighter
    vm.prank(alice);
    _rankedBattleContract.stakeNRN(3_000e18, tokenId);

    // Fighter wins battle and points are earned
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true);
    
    // Alice transfers fighter to another address
    vm.prank(alice);
    _fighterFarmContract.safeTransferFrom(alice, alice2, 0, "0x");
    assertEq(_fighterFarmContract.ownerOf(0), alice2);

    // Fighter loses battle and points are subtracted from fighter owner but,
    // since new owner does not have any points, it reverts
    vm.prank(address(_GAME_SERVER_ADDRESS));
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
}

Tools Used

Manual inspection.

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

safeTransferFrom(address from, address to, uint256 tokenId) calls safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data), so it is only required to override this one.

Assessed type

ERC721

#0 - c4-pre-sort

2024-02-23T05:56:45Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-23T05:56:57Z

raymondfam marked the issue as duplicate of #739

#2 - c4-judge

2024-03-11T02:56:16Z

HickupHH3 marked the issue as satisfactory

Lines of code

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

Vulnerability details

Game items can be non transferable and this is handled in the safeTransferFrom override function. However, the inherited ERC1155 contract also exposes the safeBatchTransferFrom function, and this function is not overridden in the GameItems contract. This means that non-transferable tokens can be transferred using the safeBatchTransferFrom function.

Impact

Users can transfer non-transferable GameItems tokens. The final impact for the protocol will depend on the expected use of the non-transferable tokens created.

According to the documentation and the deployment script, at launch, the protocol is meant to use the "battery" item, that will be used to initiate battles. The units of this item are refilled every 24 hours and users can buy additional units, but they are not transferable. This issue opens the possibility of users selling their unspent "battery" tokens, increasing the circulating supply, which will reduce the value of the token and the number of sales of the token by the protocol. It will also discourage some users from using the "battery" token to initiate battles, as they can sell them instead. This will in turn concentrate the use of the token in fewer users, reducing the number of battles and the overall activity of the protocol.

Proof of Concept

Add the following code to the contract GameItems.t.sol and run forge test --mt testTransferNonTransferableToken -vv.

function testTransferNonTransferableToken() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 1);
    _gameItemsContract.adjustTransferability(0, false);

    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;
    uint256[] memory amounts = new uint256[](1);
    amounts[0] = 1;
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");

    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

Tools Used

Manual inspection.

-    /// @notice Safely transfers an NFT from one address to another.
-    /// @dev Added a check to see if the game item is transferable.
-    function safeTransferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId,
-        uint256 amount,
-        bytes memory data
-    ) 
-        public 
-        override(ERC1155)
-    {
-        require(allGameItemAttributes[tokenId].transferable);
-        super.safeTransferFrom(from, to, tokenId, amount, data);
-    }
+    /// @dev Check to see if the game item is transferable.
+    function _beforeTokenTransfer(
+        address operator,
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) internal override(ERC1155) {
+        for (uint256 i = 0; i < ids.length; i++) {
+            require(allGameItemAttributes[ids[i]].transferable, "Non transferable token");
+        }
+    }

Assessed type

Token-Transfer

#0 - c4-pre-sort

2024-02-22T04:41:42Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T04:41:49Z

raymondfam marked the issue as duplicate of #18

#2 - c4-pre-sort

2024-02-26T00:29:46Z

raymondfam marked the issue as duplicate of #575

#3 - c4-judge

2024-03-05T04:58:29Z

HickupHH3 marked the issue as satisfactory

Awards

1.2667 USDC - $1.27

Labels

bug
3 (High Risk)
satisfactory
sufficient quality report
upgraded by judge
:robot:_86_group
duplicate-366

External Links

Lines of code

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

Vulnerability details

In the documentation it is stated that "dendroids are a more exclusive class of NFTs (...) These are very rare and more difficult to obtain". And such has also been confirmed by the sponsor's comments: "Dendroids are more rare than champions".

The protocol offers two ways of minting dendroids. The first one is through the FighterFarm.claimFighters function. This function requires sending the signature of the delegated server to check that the user has the right to mint the dendroids.

The second way is through a mint pass. For this, the user has first to claim the mint pass and then redeem. While the claim function checks that the user has the right to mint the dendroids by checking the signature received matches the mint data, the redeem function does not check again if the fighter type sent by the user matches the fighter submitted in the claim function.

This means that a user can claim a mint pass for a champion and then redeem it for a dendroid, which destroys the exclusivity factor associated with dendroids.

Impact

Users can use mint passes for champions to mint dendroids.

Proof of Concept

Add the following code to the contract FighterFarm.t.sol and run forge test --mt testRedeemMintPassChangingFighterData.

function testRedeemMintPassChangingFighterData() public {
    // Delegated server signs mintpass for 1 champion to the user
    uint8[2] memory numToMint = [1, 0]; // πŸ‘ˆ index 0 for champion
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // Mint NFT from mintpass contract
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);

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

    // Redeem mint pass to mint 1 dendroid
    _mintpassIdsToBurn[0] = 1;
    _mintPassDNAs[0] = "dna";
    _fighterTypes[0] = 1; // πŸ‘ˆ 1 = dendroid
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 1;

    _mintPassContract.approve(address(_fighterFarmContract), 1);
    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

    (,,,,,,,, bool dendroidBool) = _fighterFarmContract.fighters(0);
    assertEq(dendroidBool, true);
}

Tools Used

Manual inspection.

Store the fighter type associated with the mint pass in the AAMintPass contract and validate that the fighter type passed by the user in FighterFarm.redeemMintPass matches the value stored in the AAMintPass contract.

Assessed type

Invalid Validation

#0 - c4-pre-sort

2024-02-22T08:20:32Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T08:20:39Z

raymondfam marked the issue as duplicate of #33

#2 - c4-pre-sort

2024-02-26T00:54:10Z

raymondfam marked the issue as duplicate of #1626

#3 - c4-judge

2024-03-05T10:56:27Z

HickupHH3 changed the severity to 3 (High Risk)

#4 - c4-judge

2024-03-06T03:36:28Z

HickupHH3 marked the issue as satisfactory

Lines of code

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

Vulnerability details

FighterFarm.reRoll allows the owner of a fighter to regenerate its element, weight, and physical attributes. The number of rerolls per fighter is limited to a certain number of times depending on the fighter type.

The problem is that the fighter type is passed as an argument and the function does not check that this value matches the actual fighter type for the given fighter. This has the following implications:

  • The fighter can be rerolled more times than it should be able to. For example, if champions are allowed to reroll 4 times and dendroids are allowed to reroll 3 times, the owner of a dendroid can just reroll a fourth time by passing the fighterType parameter as 0 (champion).
  • Given that the generation of the elements, weight, and physical attributes is dependent on the fighter type, the fighter owner can simulate the outcome of the reroll with the two fighter types and choose the one that gives the best result.

Impact

Fighter owners can reroll their fighters more times than they should be able to and can simulate the outcome of the reroll with the two fighter types and choose the one that gives the best result.

Tools Used

Manual inspection.

File: FighterFarm.sol

-   function reRoll(uint8 tokenId, uint8 fighterType) public {
+   function reRoll(uint8 tokenId) public {
        require(msg.sender == ownerOf(tokenId));
+       uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0;
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

Assessed type

Invalid Validation

#0 - c4-pre-sort

2024-02-22T02:46:12Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T02:46:18Z

raymondfam marked the issue as duplicate of #306

#2 - c4-judge

2024-03-05T04:37:32Z

HickupHH3 marked the issue as satisfactory

#3 - c4-judge

2024-03-19T09:05:07Z

HickupHH3 changed the severity to 3 (High Risk)

Lines of code

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

Vulnerability details

When a fighter has NRN staked and loses a battle, there are two possible scenarios:

  1. If the user has accumulated points from previous battles, the points are decreased.

  2. If the user has no points, some of the staked NRN is set at risk.

Given that the requirement for the first scenario is just having accumulated points higher than 0, in case the user has very few points but a lot of NRN staked, the user can engage in battles with barely any downside risk, but a lot of upside potential.

A possible strategy would be to stake a very low amount of NRN and engage in battles with fighters that can be easily defeated and, after having accumulated at least one point, increase the stake to a very high amount and then engage in battles knowing that if the battle is lost, the user will only lose a few points, but if the battle is won, the user will win a lot of them.

Impact

Fighter owners can engage in battles with almost no downside risk, but a lot of upside potential.

Tools Used

Manual inspection.

Adjust the _addResultPoints function to add stake at risk if the fighter does not have enough points to cover the curStakeAtRisk amount.

Assessed type

Other

#0 - c4-pre-sort

2024-02-22T17:25:18Z

raymondfam marked the issue as insufficient quality report

#1 - c4-pre-sort

2024-02-22T17:25:25Z

raymondfam marked the issue as duplicate of #38

#2 - c4-judge

2024-03-07T02:58:22Z

HickupHH3 changed the severity to 3 (High Risk)

#3 - c4-judge

2024-03-07T03:39:36Z

HickupHH3 marked the issue as satisfactory

Lines of code

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

Vulnerability details

FighterFarm contract allows minting fighters through three functions: claimFighters, redeemMintPass and mintFromMergingPool.

Both claimFighters and redeemMintPass pass the value 100 to the customAttributes parameter of the internal function _createFighter.

This makes the code execute the _createFighterBase function.

In the _createFighterBase function, the elements variable is calculated as the module of the dna and the number of elements for the current generation of the fighterType.

The problem is that the numElements is only set in constructor for generation 0, but cannot be set for other generations. So after the generation is increased for a fighter type via the incrementGeneration function, numElements[generation[fighterType]] will be 0 and the execution will revert due to a modulo by zero error.

This means that fighters of generation 1 or higher will only be mintable from the merging pool. Note that fighters minted from the merging pool can only be of type champion, so dendroids will not be mintable at all.

Another thing that is worth mentioning is that the numElements is associated with a generation, but not with a fighter type. So the protocol lacks the flexibility to have different values for different fighter types in the same generation.

Impact

For fighter generations higher than 0 champions will only be mintable from the merging pool and dendroids will not be mintable at all.

Proof of Concept

Add the following code to the contract FighterFarm.t.sol and run forge test --mt testFighterCreationBroken.

function testFighterCreationBroken() public {
    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";

    // Champion generation increases
    _fighterFarmContract.incrementGeneration(0);

    // Claim fighter
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x12));
    _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
}

Tools Used

Manual inspection.

Add an admin function to set the values of numElements.

Also, consider having different values depending on the fighter type.

Assessed type

Error

#0 - c4-pre-sort

2024-02-22T19:10:53Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-22T19:11:02Z

raymondfam marked the issue as duplicate of #45

#2 - c4-judge

2024-03-08T03:20:05Z

HickupHH3 marked the issue as satisfactory

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470-L472 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L254

Vulnerability details

The dna variable is used to generate certain properties of the fighter, such as element, weight and physical attributes.

While the minter cannot use a specific dna for the generation of the fighter properties, he has some control over its value, especially in the redeemMintPass function, where the dna is generated from the keccak256 of the value passed by the user.

This implies that the minter can simulate the generation of a fighter with different mintPassDnas values until he finds a value that generates a fighter with the desired properties.

Impact

The minter has some control over the properties of the fighter, which could be used to generate fighters with the desired properties, some of which determine the outcome of battles.

Tools Used

Manual inspection.

Use Chainlink VRF as a source of randomness.

Assessed type

Other

#0 - c4-pre-sort

2024-02-24T02:03:11Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-24T02:03:23Z

raymondfam marked the issue as duplicate of #53

#2 - c4-judge

2024-03-06T03:49:26Z

HickupHH3 changed the severity to 3 (High Risk)

#3 - c4-judge

2024-03-06T03:54:00Z

HickupHH3 marked the issue as satisfactory

#4 - c4-judge

2024-03-15T02:10:54Z

HickupHH3 changed the severity to 2 (Med Risk)

#5 - c4-judge

2024-03-22T04:23:07Z

HickupHH3 marked the issue as duplicate of #376

Lines of code

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

Vulnerability details

RankedBattle.updateBattleRecord is called by the game server address to update the battle record of a fighter.

First the function gets the current owner of the fighter.

If the fighter has amount staked or stake at risk, it calls the internal function _addResultPoints.

This function handles different scenarios, two of which result in the stake at risk being updated in the StakeAtRisk contract.

  1. In the case of the battle being lost and the fighter does not have any points the stake at risk is increased.

  2. In the case of the battle is won and the fighter has stake at risk, part or all of the stake at risk is reclaimed.

Now let's take a look at how the StakeAtRisk contract handles these changes in the stake at risk.

updateAtRiskRecords updates the mappings stakeAtRisk, totalStakeAtRisk, and amountLost increasing their values by the amount of NRN received as parameter. For its part, reclaimNRN decreases these same mappings by the amount of NRN received as parameter.

The important thing to note is that the value in the amountLost mapping is updated for the fighter owner. Given that the owner of the fighter can change between battles, this can lead to accounting errors that can in turn cause the transaction to revert (see PoC below).

Impact

DoS for RankedBattle.updateBattleRecord, which implies the fighter owner not being able to recover the stake at risk, the battle record and the total number of battles not being updated, and, in case the fighter owner was the initiator of the battle, the voltage not being subtracted from him.

Proof of Concept

Add the following code to the contract RankedBattle.t.sol and run forge test --mt testAmountLostUnderflows.

function testAmountLostUnderflows() public {
    address alice = makeAddr("alice");
    address bob = makeAddr("bob");

    _mintFromMergingPool(alice);
    uint8 tokenId = 0;
    _fundUserWith4kNeuronByTreasury(alice);
    
    // Alice stakes NRN for fighter
    vm.prank(alice);
    _rankedBattleContract.stakeNRN(3_000e18, tokenId);

    // Fighter loses battle (stake at risk is registered)
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
    
    // Alice sells fighter to Bob
    vm.startPrank(alice);
    _rankedBattleContract.unstakeNRN(3_000e18, tokenId);
    _fighterFarmContract.transferFrom(alice, bob, tokenId);
    vm.stopPrank();

    // Fighter wins battle (stake at risk is reclaimed)
    vm.prank(address(_GAME_SERVER_ADDRESS));
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true);
}

Tools Used

Manual inspection.

Given that StakeAtRisk.amountLost is not used in any other part of the protocol and is probably just an informational variable, the most straightforward fix would be to remove it from the contract.

If it is considered necessary to keep this variable, it should be updated whenever an NFT is transferred, adjusting the values for the previous and new owner.

Assessed type

Under/Overflow

#0 - c4-pre-sort

2024-02-23T20:04:26Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2024-02-23T20:04:37Z

raymondfam marked the issue as duplicate of #1641

#2 - c4-judge

2024-03-12T03:38:07Z

HickupHH3 marked the issue as satisfactory

#3 - c4-judge

2024-03-12T04:01:25Z

HickupHH3 changed the severity to 3 (High Risk)

#4 - c4-judge

2024-03-12T04:03:31Z

HickupHH3 changed the severity to 2 (Med Risk)

#5 - c4-judge

2024-03-13T09:52:47Z

HickupHH3 marked the issue as partial-50

Low Risk Issues

IDTitle
[L-01]MergingPool:getFighterPoints reverts when called with maxId greater than 1
[L-02]The value of RankedBattle.totalBattles will be doubled
[L-03]Signature verification in FighterFarm.claimFighters will always pass if _delegatedAddress is set to address zero
[L-04]Signature in FighterFarm.claimFighters can be replayed in other chains

[L-01] MergingPool:getFighterPoints reverts when called with maxId greater than 1

The size of the array returned by getFighterPoints is set to 1, so this function can only be used to get the points of fighter with id 0. This makes it not possible to use this function for on-chain integrations with the protocol.

Additionally, name and description of the parameter maxId is misleading, as it is not the maximum id of the fighters, but the number of fighters to return points for. E.g. maxId = 1 returns up to fighter with id 0.

Consider applying the following changes:

File: MergingPool.sol

    function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) {
-       uint256[] memory points = new uint256[](1);
+       uint256[] memory points = new uint256[](maxId + 1);
-       for (uint256 i = 0; i < maxId; i++) {
+       for (uint256 i = 0; i <= maxId; i++) {
            points[i] = fighterPoints[i];
        }
        return points;
    }

[L-02] The value of RankedBattle.totalBattles will be doubled

Each time the game server address calls updateBattleRecord, the value of totalBattles is incremented by 1. As this method should be called twice for battle (once for each fighter), the value of totalBattles will be doubled.

Consider sending the data for both fighters in a single call to updateBattleRecord.

[L-03] Signature verification in FighterFarm.claimFighters will always pass if _delegatedAddress is set to address zero

In the case the FighterFarm is deployed setting the _delegatedAddress to address zero, the signature verification in the claimFighters function will always pass. Given that this misconfiguration could not be evident in the early stages of the protocol, but lead to a high-impact issue (free minting of fighters), it is recommended to add a check in the constructor to prevent the value of _delegatedAddress from being set to address zero.

[L-04] Signature in FighterFarm.claimFighters can be replayed in other chains

If the protocol decides to deploy the FighterFarm contract in other chains and use the sale delegated address to sign the messages for claiming fighters, the same signature can be replayed in different chains to claim fighters in all of them.

Consider adding a chain id to the message to prevent replay attacks.

Non-Critical Issues

IDTitleInstances
[N-01]Inconsistent use of attributes array size1
[N-02]Max NRN supply cannot be reached1
[N-03]Unnecessary code2
[N-04]Incorrect comments8

[N-01] Inconsistent use of attributes array size

In the AiArenaHelper contract the size of the attributes array is used in different places to check for valid input data and to iterate over the size of the array. In some places it is used as attributes.length and in other places the value is hardcoded as 6.

Consider using a constant to store the size of the array for consistency and gas optimization.

[N-02] Max NRN supply cannot be reached

The mint function in the Neuron contract has a check to prevent minting more than the max supply, but the condition is incorrect. The condition should be totalSupply() + amount <= MAX_SUPPLY instead of totalSupply() + amount < MAX_SUPPLY.

[N-03] Unnecessary code

File: AiArenaHelper.sol

41    constructor(uint8[][] memory probabilities) {
42        _ownerAddress = msg.sender;
43
44        // Initialize the probabilities for each attribute
45        addAttributeProbabilities(0, probabilities); πŸ‘ˆ 
46
47        uint256 attributesLength = attributes.length;
48        for (uint8 i = 0; i < attributesLength; i++) {
49            attributeProbabilities[0][attributes[i]] = probabilities[i];
50            attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i]; πŸ‘ˆ Set twice
51        }
52    } 
    (...)
68    function addAttributeDivisor(uint8[] memory attributeDivisors) external {
69        require(msg.sender == _ownerAddress);
70        require(attributeDivisors.length == attributes.length);
71
72        uint256 attributesLength = attributes.length;
73        for (uint8 i = 0; i < attributesLength; i++) {
74            attributeToDnaDivisor[attributes[i]] = attributeDivisors[i]; πŸ‘ˆ 
75        }
76    }    
File: FighterFarm.sol

443    /// @notice Hook that is called before a token transfer.
444    /// @param from The address transferring the token.
445    /// @param to The address receiving the token.
446    /// @param tokenId The ID of the NFT being transferred.
447    function _beforeTokenTransfer(address from, address to, uint256 tokenId)
448        internal
449        override(ERC721, ERC721Enumerable)
450    {
451        super._beforeTokenTransfer(from, to, tokenId); πŸ‘ˆ Just calls the default implementation
452    }

[N-04] Incorrect comments and naming

File: MergingPool.sol

136    /// @param modelURIs The array of model URIs corresponding to each round and winner address. πŸ‘ˆ
137    /// @param modelTypes The array of model types corresponding to each round and winner address.
138    /// @param customAttributes Array with [element, weight] of the newly created fighter.
139    function claimRewards(
140        string[] calldata modelURIs, πŸ‘ˆ
    (...)
154                   _fighterFarmInstance.mintFromMergingPool(
155                        msg.sender,
156                        modelURIs[claimIndex], πŸ‘ˆ This value is described as `modelHash The hash of the ML model 
                                                     associated with the fighter` in the FighterFarm contract
File: MergingPool.sol

50    /// @notice Mapping of address to fighter points. πŸ‘ˆ It should say "Mapping of **fighter ID** to points."
51    mapping(uint256 => uint256) public fighterPoints;
File: FighterFarm.sol

50    /// The address that has owner privileges (initially the contract deployer). πŸ‘ˆ Not contract deployer, but value set in constructor
51    address _ownerAddress;
File: GameItems.sol

60    /// The address that has owner privileges (initially the contract deployer). πŸ‘ˆ Not contract deployer, but value set in constructor
61    address _ownerAddress;
File: MergingPool.sol

34    /// The address that has owner privileges (initially the contract deployer). πŸ‘ˆ Not contract deployer, but value set in constructor
35    address _ownerAddress;
File: Neuron.sol

48    /// The address that has owner privileges (initially the contract deployer). πŸ‘ˆ Not contract deployer, but value set in constructor
49    address _ownerAddress;
File: RankedBattle.sol

71    /// The address that has owner privileges (initially the contract deployer). πŸ‘ˆ Not contract deployer, but value set in constructor
72    address _ownerAddress;
File: VoltageManager.sol

22    /// The address that has owner privileges (initially the contract deployer). πŸ‘ˆ Not contract deployer, but value set in constructor
23    address _ownerAddress;

#0 - raymondfam

2024-02-26T07:02:00Z

L1 to #48

#1 - c4-pre-sort

2024-02-26T07:02:03Z

raymondfam marked the issue as sufficient quality report

#2 - c4-judge

2024-03-15T06:42:57Z

HickupHH3 marked the issue as grade-c

#3 - HickupHH3

2024-03-21T03:17:32Z

#1908: R L-01: L L-02: L L-03: L L-04: R

#4 - c4-judge

2024-03-21T03:18:42Z

HickupHH3 marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax Β© 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter