AI Arena - 14si2o_Flint's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L391 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/AiArenaHelper.sol#L83-L121 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L169-L185

Vulnerability details

Impact

The reRoll(uint8 tokenId, uint8 fighterType) allows a player to reroll the physical attributes of a fighter, at a cost, to new random values in hopes of obtaining a high rarity attribute and thereby increasing the value of the NFT.

If Bob with Fighter A (tokenId = 0, fighterType = 0 ) calls reRoll(0,0), the function will work correctly. However, there is no validation that the tokenId and fighterType are correctly related.

So Bob can call reRoll(0,1), which will pass. And this will cause the NFT to obtain the highest rarity possible for all its physical attributes (Diamond).

Since the rarity of an NFT is fundamental to its value, this finding will cause the value off all AiArena NFTs to collapse to near 0.

Proof of Concept

We have Player Bob with Chamption NFT A (fightertype =0)

Bob calls reRoll(A,1)

The require statements do not check that fighter A has fighterType 1, so the function does not revert. The function calls _createFighterBase(dna,1)

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    } 

Since the fighterType is 1, the newDna will be set to 1.
This newDna is then used as input for createPhysicalAttributes.

    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); // returns 1
        return (element, weight, newDna);
    }

The uint256 rarityRank is now calculated with a dna of 1. This will result in either 0 or 1 (if attributeToDnaDivisor[attributes[i]]= 1).
Since attributeToDnaDivisor does not have 1 neither in FighterFarm nor in the DeployerScript, the result will always be 0. The newDna is returned to reRoll which calls createPhysicalAttributes which then calls dnaToIndex with a rarityRank of 0.

    function createPhysicalAttributes(
        uint256 dna, 
        uint8 generation, 
        uint8 iconsType, 
        bool dendroidBool
    ) 
        external 
        view 
        returns (FighterOps.FighterPhysicalAttributes memory) 
    {
        if (dendroidBool) {
            return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99);
        } else {
            uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length);

            uint256 attributesLength = attributes.length;
            for (uint8 i = 0; i < attributesLength; i++) {
                if (
                  i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
                  i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
                  i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
                ) {
                    finalAttributeProbabilityIndexes[i] = 50;
                } else {
                    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
                    // 1 / x % 100 = 0 or 1
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
                    finalAttributeProbabilityIndexes[i] = attributeIndex;
                }
            }
            return FighterOps.FighterPhysicalAttributes(
                finalAttributeProbabilityIndexes[0],
                finalAttributeProbabilityIndexes[1],
                finalAttributeProbabilityIndexes[2],
                finalAttributeProbabilityIndexes[3],
                finalAttributeProbabilityIndexes[4],
                finalAttributeProbabilityIndexes[5]
            );
        }
    }

Since rarityRank = 0, on the first loop with i=0 and cumProb = 0 this will evaluate to:
if(0 >= 0){attributeProbabilityIndex = 0 + 1} return attributeProbabilityIndex

Every physical attribute will therefore be set at the highest rarity rank of 1.

    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;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex;
    }

Foundry Proof of Concept

First increase maxRerollsAllowed in FighterFarm.sol to 20.

uint8[2] public maxRerollsAllowed = [3, 20];

Then increase _fundUserWith4kNeuronByTreasury in FighterFarm.t.sol to 40_000.

    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 40_000 * 10 ** 18);
        assertEq(40_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

Add the below test to FighterFarm.t.sol

    /// @notice Test rerolling champing to max rarity with wrong fighterType input
    function testRerollMaxRarity() public {
        _mintFromMergingPool(_ownerAddress);
        // get 40k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model
        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);
            uint8 exceededLimit = maxRerolls-1;
            uint8 tokenId = 0;
            uint8 fighterType = 1;

            //reroll the fighter 20 times
            _neuronContract.addSpender(address(_fighterFarmContract));
            for (uint8 i = 0; i < exceededLimit; i++) {

                    _fighterFarmContract.reRoll(tokenId, fighterType);
            }
        }
    }

Run the test with forge test --match-test testRerollMaxRarity -vvvv

You will get 20x the output: FighterPhysicalAttributes({ head: 1, eyes: 1, mouth: 1, body: 1, hands: 1, feet: 1 })

    ├─ [89681] FighterFarm::reRoll(0, 1)
    │   ├─ [586] Neuron::balanceOf(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1]) [staticcall]
    │   │   └─ ← 13000000000000000000000 [1.3e22]
    │   ├─ [22833] Neuron::approveSpender(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 1000000000000000000000 [1e21])
    │   │   ├─ emit Approval(owner: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], spender: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], value: 1000000000000000000000 [1e21])
    │   │   └─ ← ()
    │   ├─ [5948] Neuron::transferFrom(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 1000000000000000000000 [1e21])
    │   │   ├─ emit Approval(owner: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], spender: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], value: 0)
    │   │   ├─ emit Transfer(from: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, value: 1000000000000000000000 [1e21])
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   ├─ [31307] AiArenaHelper::createPhysicalAttributes(1, 0, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 1, eyes: 1, mouth: 1, body: 1, hands: 1, feet: 1 })

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Since the tokenId already has the required data to ascertain the fighterType, the solution is simply to remove fighterType as an input parameter.

-    function reRoll(uint8 tokenId, uint8 fighterType) public {
+    function reRoll(uint8 tokenId) public {
        require(msg.sender == ownerOf(tokenId));
+        uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0;
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If a user stakes 999 wei or less, it will be impossible for stakeAtRisk to increase since curStakeAtRisk will always resolve to 0. As such, the player can accumulate points risk-free and obtain a very small reward every round.

This represents an unfair situation for normal players who stake NRN. They suffer a slight financial loss since their slice of the NRN rewards becomes smaller the greater the total amount of distributed points per round.

Proof of Concept

Player Alice stakes 999 wei and battles with her NFT Fighter

The game server calls updateBattleRecord which checks the stake and stakeAtRisk to see if the player is eligible for points.

    uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
    if (amountStaked[tokenId] + stakeAtRisk > 0) {
        _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
    }

amountStaked[tokenId] = 999 > 0 so _addResultsPoints is called.

If Alice wins, she gains points. Her stakeAtrisk == 0 since its her first battle.

    if (stakeAtRisk == 0) {
        points = stakingFactor[tokenId] * eloFactor;
    }

The stakingFactor[tokenId] == 1 will be obtained by _getStakingFactor

    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
          // sqrt((999 + 0 ) /10**18)) = 0
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;

Therefore Alice will receive 1 x eloFactor (f.e. 1500 points).

If Alice loses, curStakeAtRisk will used to determine how much stake she loses. However, since Alice staked 999 wei or less, the calculation will resolve to 0.999 and be rounded down to 0.

        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        // (10 * (999 + 0)) / 10*4 = 0.999 => 0

Since curStakeAtRisk = 0, the below code will effectively do nothing but waste gas since nothing is transferred and no state variable is changed.

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

The financial loss to normal players will be extremely small (1% at most) since their stakingFactors (and by consequence points) will most likely be 10x-100x greater than Alice.

Yet, this clearly undermines the fundamental fair playing field premise of AiArena, where players put their NRN at risk for the goal of winning points & NRN. Additionally, this also forces the protocol to execute pointless 0 calls, which are a complete unneccessary waste of gas.

Tools Used

Manual Review

Recommended Mitigation Steps

If we require players to stake at minimum 1 NRN, then this situation cannot occur. In RankedBattle

    function stakeNRN(uint256 amount, uint256 tokenId) external {
-        require(amount > 0, "Amount cannot be 0");
+        require(amount > 10**18, "Amount must be at least 1 NRN"); 
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        require(_neuronInstance.balanceOf(msg.sender) >= amount, "Stake amount exceeds balance");
        require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");

        _neuronInstance.approveStaker(msg.sender, address(this), amount);
        bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, true); 
            }
            amountStaked[tokenId] += amount;
            globalStakedAmount += amount;
            stakingFactor[tokenId] = _getStakingFactor(
                tokenId, 
                _stakeAtRiskInstance.getStakeAtRisk(tokenId)
            ); 
            _calculatedStakingFactor[tokenId][roundId] = true;
            emit Staked(msg.sender, amount);
        }
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The fundamental premise of AiArena is that players stake NRN and put that NRN at risk during battles in the hopes of winning points & prizes. A balance between prizes distributed and NRN lost by players is essential to the long-term longevity of the protocol.

However, if a player on a losing streak unstakes everything, he can still continue to play to reclaim his lost stake.

If he wins a battle: a small part of the lost stake gets reclaimed and he immediately unstakes that part.
If he loses a battle: nothing happens since there is no stake to lose.

This represents a direct financial loss to the protocol since the _sweepLostStake will be much smaller due to losing players reducing their lost stake without ever risking any loss.

Proof of Concept

Bob stakes 1M NRN on Fighter A.

amountStaked[A] = 1_000_000

Bob plays 20 battles over 2 days and he loses every battle.
amountStaked[A] = 980_000
stakAtRisk[A] = 20_000

He then decides to unstake everything and continues to play.
amountStaked[A] = 0
stakAtRisk[A] = 20_000

Since amountStaked[A] + stakeAtRisk[A] remains >0, _addResultsPoints will be called.

If Bob wins a battle, he reclaims a part of the stakeAtRisk which he unstakes immediately.

In _addResultsPoints:

   curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
   // = 10 * (0 +20_000) / 10000 = 20   

If Bob loses a battle, nothing happens.

In _addResultsPoints:

   curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
   // = 10 * (0 +20_000) / 10000 = 20
   
   /// Do not allow users to lose more NRNs than they have in their staking pool
   if (curStakeAtRisk > amountStaked[tokenId]) {
      curStakeAtRisk = amountStaked[tokenId];
      // 20 > 0  => curStakeAtRisk = 0 and a 0 transfer will executed, which changes nothing 
   }   

If the round still last for 12 more days (bi-weekly), Bob can potentially reclaim 12 * 10 * 20 = 2400 NRN while risking 0 NRN. Or he could buy 2 batteries every time he wins and play up to 12 * 5 * 10 = 600 extra battles in the round, thereby reclaiming up to 12000 extra NRN

Note: It is practically impossible for a normal player to lose all their stake. You lose 0.1% of your stake with every loss, so for Alice (deposit 1000 NRN) to lose everything, she would have to lose 1000 battles in a row. Since a normal round only has 140 battles, this would require buying 86 batteries with a 100% loss rate (nearly impossible with an elo-matchmaking system). So this situation can only occur as a deliberate misuse of the system.

Tools Used

Manual Review

Recommended Mitigation Steps

The migitation is straightforward. If a player wins, require that his staked NRN be at least the same as his stakeAtRisk.

In _addResultsPoints

        if (battleResult == 0) {
            /// If the user won the match
+           require(amountStaked[tokenId] >= stakeAtRisk,"You cannot reclaim NRN if you have no stake");

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The numElements mapping keeps track of the number of elements by generation and is set in the constructor to numElements[0]= 3

When the incrementGeneration(uint8 fightertype) is called to increase the generation of f.e fighertype 0 from generation 0 -> 1, the numElements mapping is not updated. This means that for generation 1, the numElements[generation[fightertype]] will be numElements[1] = 0

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

This causes _createFighterBase(dna, fighterType) to panic due to modulo by zero

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

The functions claimfighters and redeemMintPass always call _createNewFighter which calls createFighterBase. For mintFromMergingPool, if the custom attributes are 100,createFighterBase is also called. Furthermore, rerolls of a fighter directly call createFighterBase.

Thus, increasing the generation breaks nearly all fighter creation and all rerolling, effectively bricking the contract.

Since there is no functionality to set numElements, the protocol is bricked permanently.

Proof of Concept

In FighterFarm.t.sol

For claimFighters

    /// @notice Test whether claiming a fighter still works if you increase the generation.
    function testGenerationIncreaseClaimFightersRevert() public {
        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](1);
        claimModelTypes[0] = "original";
        _fighterFarmContract.incrementGeneration(0);

        vm.expectRevert();// panic: division or modulo by zero (0x12)
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);  
    }

For redeemMintPass

    /// @notice Test to see whether redeeming a mintpass for a fighter still works when the generation is increased
    function testGenerationIncreaseRedeemMintPassRevert() 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
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        // 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] = 0;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

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

        vm.expectRevert();// panic: division or modulo by zero (0x12)

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

For mintFromMergingPool

    /// @notice Test that the merging pool contract can mint an fighter after generation is increased.
    function testGenerationIncreaseMintFromMergingPoolRevert() public {
        _fighterFarmContract.incrementGeneration(0);
        vm.prank(address(_mergingPoolContract));

        vm.expectRevert();// panic: division or modulo by zero (0x12)
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
        // If customattributes are not [100,100], _createFighterBase is not called and creation is successful.
    }

For reRoll

    /// @notice Testing Rerolls after generation increase. 
    function testGenerationIncreaseRerollRevert() public {
        _mintFromMergingPool(_ownerAddress);

        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model
        _fighterFarmContract.incrementGeneration(0);
        _neuronContract.addSpender(address(_fighterFarmContract));
        vm.expectRevert(); // panic: division or modulo by zero (0x12)
        _fighterFarmContract.reRoll(0, 0);        
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

The mitigation is to simply apply the same logic as is used for rankedNrnDistribution.

When a generation is increased, the numElements mapping should be automatically set to the same values of the last generation. Alongside, provide a setNumElements function in order to set it to a new value when needed.

In FighterFarm.sol

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

    function setNumElements(uint8 fighterType, uint8 numElements) external {
        require(isAdmin[msg.sender]);
        numElements[generation[fightertype]] = numElements;
    }

s

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

This finding requires two conditions, which are easily met:

• The sold Fighter NFT has a value > 0 for stakeAtRisk[][] at the time of sale.
• The buyer battles with his new Fighter in the same round that the Fighter was sold.

When the buyer obtain a fighter, the variable that tracks stakeAtrisk is not reset. The buyer cannot not stake until a new round starts (since the previous owner unstaked in order to sell), so he assumes that the battles with his new fighter will only affect the battlerecord. However, if the above conditions are met, the unstaked Fighter will be treated as a "staking" fighter and the player will, to his surprise, receive a part of the stakeAtRisk of the previous owner if he wins.

The player can thus obtain the lost stake of the previous owner, which by all rights should belong to the protocol. This is a direct loss of funds to the protocol, so I believes this warrents a High severity.

Proof of Concept

Player Alice owns Fighter A and after playing and losing 20+ battles in round 1, she has no points and some stakeAtRisk.

amountStaked[A]= 1_000_000
accumulatedPointsPerFighter[A][1]= 0
accumulatedPointsPerAddress[Alice][1] = 0
stakeAtRisk[1][A]= 2000

Frustrated with the game, she unstakes and sells the Fighter A to Bob who immediately starts playing with Fighter A.

However, the state variables related to Fighter A are not being reset.

Which means the variables for Bob are:

amountStaked[A]= 0
accumulatedPointsPerFighter[A][1]= 0
stakeAtRisk[1][A]= 2000

Since he cannot stake anything on Fighter A in the same round (due to the hasUnstaked require in stake()), he uses fighter A to test a new strategy. The server calls server calls updateBattleRecord, which has the below line to decides if the player can earn or lose points.

        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }

Since Bob cannot stake on Fighter A, amountStaked[A] = 0. However, stakeAtRisk is set by calling StakeAtRisk.getStakeAtRisk

    function getStakeAtRisk(uint256 fighterId) external view returns(uint256) {
        return stakeAtRisk[roundId][fighterId];
    }

This returns 2000 since the stakeAtrisk[1][A] is still set to 2000.

The if statement then resolves to true if(0 + 2000 > 0) and _addResultPoints is called.

If Bob loses the battle, nothing will happen since he has no stake nor any points on the fighter.

        } 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]) {
            // amountStaked[1] = 0 so curStakeAtRisk = 0;
                curStakeAtRisk = amountStaked[tokenId];
            }
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
            // accumulatedPointsPerFighter[A][1])== 0 so no points can be deducted 
                /// 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);
                // 0 is transferred to the Neuron Contract. 
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }
        }

If Bob wins however, he will receive a part of the stakeAtRisk of Alice:

        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        // curStakeAtRisk = 10 * ( 0 + 2000) / 10000 = 2;
        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);
            // Bob receives 2 NRN on a fighter which he never staked. 
                amountStaked[tokenId] += curStakeAtRisk;
            }

So Bob can win 2 NRN for every battle he wins with Fighter A for which he never staked anything. And he cannot lose anything.

Even though the amount reclaimed is small, this still represents a direct financial loss to the protocol and players should never able to drain the lost stake of another player.

Tools Used

Manual Review

Recommended Mitigation Steps

A simple mitigation would be to set the stakeAtRisk[roundId][fighterId] to 0 whenever a fighter is transferred.

In StakeAtRisk.soladd the below function.

    function _clearStakeAtRisk(uint256 tokenId) private {
        require (msg.sender == _fighterFarmAddress, "Call must be from FighterFarm");
        stakeAtRisk[roundId][tokenId] = 0;
    }

In FighterFarm.sol

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

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

Assessed type

Other

[L-01] globalStakedAmount is greater than actual staked amount

The globalStakedAmount state variable in RankedBattle.sol keeps track of the overall staked amount in the protocol. It is increased by staking and decreased by unstaking. However, it does not take into account the stake moved to the StakeAtRisk contract, a part of which will be sent to the treasury.

As such, every round the globalStakedAmount becomes a fraction larger than the actual staked amount and this discrepancy will only increase over time.

This is currently only a Low since the variable is not used in any calculations.

[L-02] Incomplete require check in redeemMintPass

The require statement in redeemMintPass checks that all the arrays have the same length, but does not include iconsTypes.

As a result, it is possible to input an iconTypes array with a shorter length, which will case een Out-Of-Bounds error and revert the function.

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

[L-03] Incomplete require check in GameItems.mint

The 4th require statement in GameItems.mint checks that the quantity demanded does not exceed the dailyAllowance or the allowanceRemaining. Yet the first part of the require only checks if the user can replenish his allowance, not that the quantity is equal or lower than the dailyAllowance.

As a consequence, if quantity = 100, dailyAllowance = 10 and dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp == true, then it will pass the require statement.

This will cause an underflow and revert on allowanceRemaining[msg.sender][tokenId] -= quantity;

        require(
-            dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp || 
+            (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp && quantity <= allGameItemAttributes[tokenId].dailyAllowance ) || 
            quantity <= allowanceRemaining[msg.sender][tokenId]
        );

[L-04] GameItems.remainingSupply will return 0 for a game item with infinite supply

When a game item is created, if finiteSupply == true then the itemsRemaining is set to a specific number. However, when finiteSupply == false then the itemsRemaining is set to 0. This is confirmed in the Deployer script for the battery game item.

As such, when a player calls remainingSupply on a game item with infinite supply, he will get 0 as return. Which is obviously an error. I would suggest changing the remainingSupply to avoid confusion.

    function remainingSupply(uint256 tokenId) public view returns (uint256) {
        if(allGameItemAttributes[tokenId].finiteSupply){
            return allGameItemAttributes[tokenId].itemsRemaining;
        }else{
            return type(uint256).max;
        }
    }

[NC-01] Replenish Voltage when Allowance is sufficient

The spendVoltage function in VoltageManager does not check if the current voltage is sufficient for the expenditure before calling _replenishVoltage. This means a player can have a maximum voltage of 100 and still be forced to replenish to a 100 when he spend 10 to do one battle.

The replenish timer is then set every day exactly 24 hours after the first battle, which forces players to keep track of this timer on a daily basis.

[NC-02] Arbitrum Decentralisation and Front-running

If it were possible to frontrun the results of a battle then this would have severe negative consequences for the protocol. Current Arbitrum is centralised and frontrunning is impossible.

However, decentralision is planned by Arbitrum and will become reality. At which point the protocol will have a serious problem.

Analysis of AiArena

1. Description AiArena

AI Arena is a PvP platform fighting game where players can develop and use AI models to let charachters battle against each other. In this way it functions both as game and as an AI education platform.

In the web3 version of the game, which is currently under audit, the characters are tokenised into NFT fighters with each their unique attributes and loaded up with the AI model provided by the players. Players then stake NRN and battle to gain points while risking to lose a tiny fraction of their stake if they lose a battle. After a round prizes are distributed and a semi-random draw is done for the rights to mint a new fighter. The NFT's can also be traded freely, which opens up an interesting secondary market.

2. Approach taken for this contest

Time spent on this audit: 9 days

Day 1:

• Reading the documentation
• First pass through the code with a bottom to top approach
• Adding @audit-info tags anywhere a question came up

Day 2-3:

• Drawing in Excalidraw:
  • High-level Architecture of the protocol
  • Function diagrams of every contract
  • User Flows

Day 4-5:

• Second pass through the code
• Line-by-line review of every contract
• Adding @audit-issue tags whenever I see something wrong
• Start writing Notes.md with preliminary findings.

Day 6-8:

• Foundry Testing of all @audit-issue tags
• Writing up reports & creating a POC when I have confirmed a finding
• Making sure that all tags were properly treated
• Writing QA report

Day 9:

• Writing Analysis report

3. Architecture

The core of the protocol consist of two contracts,FighterFarm.sol and RankedBattle.sol.

FighterFarm is the NFT Factory, it contains the various ways of minting a Fighter NFT and also the semi-random calculations used to obtain the various attributes of the NFTs.

RankedBattle is the accounting module of the system. It contains the functionality for staking, unstaking and the claiming of rewards. It also receives the outcome of NFT battles from the gameserver, after which it calculates the lossess and gains of the players and tries to ensure that all state variables are correctly updated.

4. Main contracts and functionality

For each contract I have provided a description, an overview of the most important functions available to normal users and a diagram to visualise internal and external calls.

RankedBattle.sol: This is the accounting contract, which handles the staking, battle records and reward calculation/distribution based upon the battle outcomes.

• stakeNRN: This function allows a user to stake NRN on a specific fighter NFT.
• unstakeNRN: This allows the reverse, the unstaking of NRN from a specific fighter NFT.
• claimNRN: A user can claim NRN from past rounds.
• getUnclaimedNRN: This returns the unclaimed NRN tokens for a specified address.

FighterFarm.sol: This contract functions as the NFT Factory and is the central point of management for the creation, ownership and redemption of Fighter NFTs.

• claimFighters: This enables users to claim a pre-determined number of fighters.
• redeemMintPass: A user can exchange n number of mint passess for fighter NFTs.
• updateModel: The owner of an NFT Fighter can change the machine learning model used by the fighter through this function.
• (safe)transferFrom : These 2 functions allow a user to transfer the ownership off an NFT fighter.
• reroll: A user can re-roll an existing fighter NFT to have new random physical attributes.

AiArenaHelper.sol: This contract handles the creation and management of the attributes of the NFT fighters.

• createPhysicalAttributes: This functions returns randomised physical attributes which can be used in the minting of the fighter NFT.
• getAttributeProbalities: This simply returns the attribute probabilities for a given generation and attribute.
• dnaToIndex: The function converts dna and rarity rank into a attribute probability index.

FighterOps.sol: This library is used for creating and managing Fighter NFTs in the AI Arena game

• getFighterAttributes: This returns the fighter attributes from the Fighter struct.
• viewFighterInfo: This returns all relevant fighter information.

GameItems.sol: This contract provides a range of game items that can be used by players in the game.

• mint: A user can spend NRN to mint and receive a game item.
• getAllowanceRemaining: This function informs a user the remaining number of items that can still be minted today.
• remainingSupply: This returns the remaining quota for a specific game item.
• safeTransferFrom: This transfers a game item to a new owner.

MergingPool.sol: This contract allows users to earn a new fighter NFT assuming they have won rounds.

• claimRewards: The function checks the rounds and mints a new NFT for each round that the user won.

• getUnclaimRewards: This returns the amount of unclaimed rewards for the addres inputted.

• getFighterPoints: This rerurns an array with the points for all fighters upto a maxId.

  

Neuron.sol: The ERC20 Neuron token contract which is used for the in game economy.

• claim: A user can call this function to claim a specific amount of tokens from the treasury.

• burn: This burns the specified amount of tokens from the caller's address.

• burnFrom: This burns the specified amount of tokens from the account address.

  

StakeAtRisk.sol: This contract manages the staked NRN that are at risk during a round of competition. It is mainly used by RankedBattle.

• getStakeAtRisk: This returns the stake at risk for a specific fighter NFT in the current round.

  

VoltageManager.sol: This contract manages the voltage system for AI Arena.

• useVoltageBattery: This allows a user to use a voltage battery to replenish voltage.
• spendVoltage: This functions spends the voltage.

5. User flows

The user flows are organised in chronological order.

1. Obtaining a Fighter NFT

The first action of anyone who wants to participate in AiArena is to obtain a Fighter NFT. At launch, the redeemMintPass() function will be the sole way to mint a fighter NFT. After some time, the protocol will pick winners who can claim new fighters from the mergingPool and/or distribute signatures that can be redeemed for a new Fighter. Additionally, it is possible to buy a NFT Fighter on the secondary market the moment the protocol goes live.

2. Staking & Unstaking

If a player wishes to do battle, he needs to stake some NRN. If he no longer wishes to risk his NRN, he can unstake.
It is important to note that in its current iteration the staking and unstaking is prone to malicious misuse.

• There is no minimum stake required, so it is possible to deposit dust so small it cannot be lost, but which still allows a player to acquire points and prizes.
• In the other sense, if a player has lost some of his NRN due to bad play, he can unstake but still continue to play risk-free in order to regain some of his lost NRN. These unhappy/malicious paths have not been sufficiently explored.

3. Battle

The battle mechanism can be called the pivotal part of the entire system since the success of the entire protocol rests upon its ability to correctly distribute gains and losses to players. The functions receive the battle outcome from the off-chain gameserver and the entire decision tree is run for each of the participants of the battle. The system works correctly when a player uses a brand new fighter NFT.

Yet the consequences of selling and buying fighters on the secondary market have not been fully taken into account. There are multiple mechanisms where the points per fighter are conflated with the points per player, which causes unintended negative consequences for the players. Some examples of these (which I submitted as findings):

• If a player sells a fighter with a positive stakeAtRisk, the new owner can drain the lost stake of the previous owner if he plays in the same round. Without any need for staking.
• The balance of mergingPool points is not stored on the player address who won the points. It is only stored on the fighter so if the fighter is sold, the player loses his entire balance.

4. Claiming NRN

After a round has been concluded, players can claim rewards. Their rewards will be calculated by comparing their points with the total points generated in the round.

6. Code Quality / Test Quality

Code

Overall, the quality of the code is very good. There are some points which can be improved:

• Some require statements are incomplete and allow bad input to pass
• Floating pragma's should be avoided
• Consider using constants instead of magic numbers
• Events do not make use of indexed fields
• require statements should have descriptive messages
• NatSpec is missing various tags throughout the protocol

Testing

There clearly has been an effort to achieve near 100% test coverage, which is very good. However, the tests only explore the expected happy paths and do not consider unhappy paths and/or random input.

An example of this is the reRoll(uint8 tokenId, uint8 fighterType) function.
There are 2 tests for this function:

• testReroll() which tests the happy path.
• testRerollRevertWhenLimitExceeded() which checks that the maxReroll is correctly applied.

It would seem everything is working fine. But if a user calls reroll with the wrong fightertype, he instantly rerolls his NFT to the highest rarity for all attributes. A critical error which would destroy the value of all AiArena NFTs and one that would be instantly found with the most basic of fuzz tests. Therefore I would recommend that the testing should be expanded to at least include basic fuzzing.

7. Risks

Centralisation

1. Arrays of Admins.

In various contracts the owner can set an entire array of Admins, who have far reaching power. The greater the amounts of admins, the greater the chance that a malicious actor can sneak in or that a normal actor turns malicious. Therefore it is recommended to limit the amount of actors with admin power to the utmost, since they have the capability to severely damage the protocol.

Admin powers in each contract:

• RankedBattle:
  • setRankedNRNDistribution
  • setBpsLostPerLoss
  • setNewRound
• GameItems:
  • setAllowedBurningAddresses
  • createGameItem
• MergingPool:
  • updateWinnersPerPeriod
  • pickWinner
• VoltageManager:
  • adjustAllowedVoltageSpenders

2. One-Step ownership change.

For all the contracts, the change in ownership happens immediately in one step. This carries a very high degree of risk since inputting the wrong address or having the owner hacked and hacker taking the ownership of the protocol cannot be blocked. It is recommended to implement a two-step system whereby a or multiple admins would have to approve the change in ownership.

Systemic

1. Frontrunning & Arbitrum Decentralisation

The protocol in its current state would be severily damaged if players could frontrun a loss by unstaking. Currently this is impossible since the sequencer of arbitrum is still centralised. However, the decentralisation is planned and will happen. The protocol should invest sufficient time and effort to adapt the functionality of the protocol to make frontrunning on a decentralised Arbitrum (or other chains) as difficult as possible.

2. Lack of Upgradability

Even though immutability is regarded as one of the hallmarks strong points of web3, most protocols have included some form of upgradabilty in order to deal with the inevitable bugs and flaws that will show themselves over time. No project has perfect code.

For AiArena, the developers have not chosen any of the common upgradabilty proxy patterns but instead they have provided functions in each contract to change the address of the related contracts. If for example, a flaw was found in the GameItems contract, a new contract would be deployed and the admins can set the gameItemsContractInstance to the new address in every contract that requires. So not a system of upgrades, but more of a replacement. If this is used for contracts that hold state (FighterFarm/RankedBattle) however, the redeployment would effectively mean wiping out the entire protocol and starting with a blank slate. Which would significant reputation damage and the risk that users will not risk staking in the protocol.

8. Time spent

58 Hours

Time spent:

58 hours