AI Arena - 0xDetermination's results

Lines of code

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

Vulnerability details

Impact

Users can obtain an unfair advantage when earning NRN/points in RankedBattle, increasing their own rewards at the expense of other users.

(When a user's points are increased, rewards for all other users are decreased since NRN rewards are calculated according to the total points earned. https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L302-L303)

Proof of Concept

Notice below that when RankedBattle.updateBattleRecord() submits a loss, the check if (accumulatedPointsPerFighter[tokenId][roundId] > 0) prevents users from losing any stake to the StakeAtRisk contract.

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

This causes an edge case where a user has a tiny amount of points (for example, 1 point) and can then stake a huge amount of NRN into the contract. The user essentially can only win the next fight, because if they lose the consequences in RankedBattle will be negligible. On the other hand, if the user wins then they can win a huge amount of points since they staked a large amount of NRN into the contract. This allows the accumulation of a large amount of points without the proper risk of losing NRN.

Run the below test case in RankedBattle.t.sol:

    function testStakeAtRiskExploit() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        vm.startPrank(player);
        //player stakes a dust amount
        _rankedBattleContract.stakeNRN(1, 0);
        //fighter win and accumulate dust points
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        //player stakes a large amount
        vm.startPrank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        //fighter loses while the large amount is staked
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        //stakeAtRisk for the fighter is still zero
        console.log(_stakeAtRiskContract.getStakeAtRisk(0));
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Refactor updateBattleRecord() so that it's possible for users to both lose points and have their stake transferred to StakeAtRisk after losing a battle.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Fighters can't reroll, protocol functionality lost.

Proof of Concept

See the below code:

    function reRoll(uint8 tokenId, uint8 fighterType) public {

Although it's expected that there will be more than 256 fighters, the tokenId parameter type is uint8. This will prevent fighters with a tokenId greater than 255 from rerolling.

Tools Used

Manual Review

Recommended Mitigation Steps

The tokenId parameter type should be greater than uint8:

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Unauthorized minting of fighter NFTs.

Proof of Concept

An attacker with multiple rounds of rewards to claim can mint extra fighter NFTs by reentering MergingPool.claimRewards(). The function calls FighterFarm.mintFromMergingPool(), which calls OpenZeppelin's ERC721._safeMint(). Since _safeMint() uses the _checkOnERC721Received() hook, reentrancy is possible:

   function claimRewards(
        ...
                    _fighterFarmInstance.mintFromMergingPool(

    function mintFromMergingPool(
        ...
        _createNewFighter(

    ...

    function _createNewFighter(
        _safeMint(to, newId);

    function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data), //reentrancy hook
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }
    ...
        function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert("ERC721: transfer to non ERC721Receiver implementer");
                } else {
                    /// @solidity memory-safe-assembly
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true;
        }
    }

Now look at the MergingPool.claimRewards() function:

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

An attacker with multiple rounds of rewards to claim can reenter the function to mint extra NFTs. Example:

1. The attacker has one NFT each to claim in round 0 and round 1. The current roundId is 2.
2. The attacker contract calls claimRewards(), and the NFT for round 0 is minted. numRoundsClaimed[attackerContract] is now 1, and the attacker uses the minting hook to reenter the contract and call claimRewards() again.
3. The attacker is minted an NFT for round 1 by the reentrancy call.
4. The first call to claimRewards() continues to execute, and another NFT for round 1 is minted. The result in this example is that the attacker has minted an extra NFT. The more rewards and more rounds to claim in for the attacker, the more NFTs can be minted via reentrancy.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a reentrancy guard, or consider modifying FighterFarm so that the _checkOnERC721Received() hook is not called.

Assessed type

Reentrancy

Judge has assessed an item in Issue #1794 as 2 risk. The relevant finding follows:

[L-6] MergingPool.claimRewards() can mint an NFT with out-of-range fighter weight

Lines of code

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

Vulnerability details

Impact

User reward NFTs may become permanently unclaimable.

Proof of Concept

Notice the code below; MergingPool.claimRewards() calls FighterFarm.mintFromMergingPool(), which reverts if the NFT balance of the receiver is at the MAX_FIGHTERS_ALLOWED limit:

    function claimRewards(
        ...
                    _fighterFarmInstance.mintFromMergingPool(

    uint8 public constant MAX_FIGHTERS_ALLOWED = 10;
    ...
    function mintFromMergingPool(
        ...
        _createNewFighter(
    ...
    function _createNewFighter(
        ...
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);

Therefore, if a user has more NFTs to claim than MAX_FIGHTERS_ALLOWED, then those NFT rewards will be permanently unclaimable.

Note that this issue can also occur with FighterFarm.claimFighters() if there is admin error (admin signs for more than 10 fighters to be minted to one address by FighterFarm.claimFighters()).

Test case below should revert, run in MergingPool.t.sol:

    function testClaimRewardsRevertTooManyNFTs() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](11);
        _mergingPoolContract.updateWinnersPerPeriod(11);
        _mergingPoolContract.pickWinner(_winners);
        string[] memory _modelURIs = new string[](11);
        string[] memory _modelTypes = new string[](11);
        uint256[2][] memory _customAttributes = new uint256[2][](11);
        _mergingPoolContract.pickWinner(_winners);
        //revert, too many fighters
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    }

Use -vvv flag to notice that fighters are minted to the address until it reaches MAX_FIGHTERS_ALLOWED, then the tx reverts.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Allow users to call MergingPool.claimRewards() only up to a certain roundId so that the number of NFTs minted in one transaction can be decreased.

Assessed type

Other

Judge has assessed an item in Issue #1794 as 2 risk. The relevant finding follows:

[L-4] New users to the protocol joining a significant amount of time after project launch will incur huge gas fees when calling RankedBattle.claimNRN()

Lines of code

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

Vulnerability details

Impact

The MergingPool genetic/evolution process can be ignored; users can arbitrarily determine the attributes of their reward NFTs.

Proof of Concept

The AiArena docs state that "It is important to note that NFTs created from the Merging Pool are different from those that are purchased through primary drops. These NFTs are created via a genetic algorithm¹ that adopt the skills of the NFTs that were submitted into pool." (https://docs.aiarena.io/gaming-competition/merging)

However, notice below that the model training hash and other elements can be determined by the user due to a lack of input validation:

    /// @param modelURIs The array of model URIs corresponding to each round and winner address.
    /// @param modelTypes The array of model types corresponding to each round and winner address.
    /// @param customAttributes Array with [element, weight] of the newly created fighter.
    function claimRewards(
        string[] calldata modelURIs, //notice there is no input validation for these arguments
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

The arbitrary arguments passed to claimRewards() are used to determine the modelHash, modelType, and custom attributes of the newly minted fighter NFT:

    /// @param modelHash The hash of the ML model associated with the fighter.
    /// @param modelType The type of the ML model associated with the fighter.
    /// @param customAttributes Array with [element, weight] of the newly created fighter.
    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

As a result, reward NFTs don't have to be minted as intended by the protocol; users can mint the reward NFTs with arbitrary properties.

Tools Used

Manual Review

Recommended Mitigation Steps

Use a signature verification mechanism to ensure that users mint rewards with the proper arguments.

Assessed type

Other

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

Vulnerability details

Impact

Users can strategically mint (or reroll) rarer fighter NFTs, which may negatively affect the NFT tokenomics of the protocol.

Proof of Concept

As an example, see the FighterFarm.reRoll() function:

    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"); //@Gas this is not rly necessary

        _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] = "";
        }
    }    
    ...
    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

The dna, which is pseudorandom, determines the fighter's element and weight in _createFighterBase(), and determines the physical attributes in AiArenaHelper.createPhysicalAttributes() (https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L121).

Because these properties are deterministic, users can strategically mint/reroll to obtain a more valuable fighter. See the 'Levels of Rarity' section of the docs, which describes that physical attributes have rarity levels (https://docs.aiarena.io/gaming-competition/nft-makeup). Also note that AiArenaHelper.sol calculates rarities for physical attributes (https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L99-L111).

Tools Used

Manual Review

Recommended Mitigation Steps

Use a randomness oracle like Chainlink VRF instead of pseudorandomness.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L302-L303 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L270-L290 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L460-L463 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L472-L487 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L253-L255

Vulnerability details

Impact

A malicious user can abuse this bug to never lose a match, accruing an inappropriately large amount of points in RankedBattle.sol. This enables the user to claim an oversized portion of the rewards, and reduce rewards for other players.

Note that when a user's points are increased, rewards for all other users are decreased since NRN rewards are calculated according to the total points earned. (https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L302-L303)

(Explanation in PoC is long, skip to commented Foundry test if desired)

Proof of Concept

Firstly, notice that the rewards distributed in RankedBattle.sol are determined competitively based on the total amount of points in the pool (see inline comment):

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

Now we describe how a malicious player can carefully forge an exploit and never lose a match. The first step is to make a fighter NFT transferable with nonzero stake. This can be done by unstaking fully for a fighter that has some NRN locked in the StakeAtRisk contract. Notice below that after unstaking fully, NFT transfer is unlocked, and after winning a match, the fighter's amountStaked can be increased without re-locking transfer of the fighter:

(see inline comments)

    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); //unlock NFT transfer if fully unstaking
            }
            emit Unstaked(msg.sender, amount);
        }
    }

    //This code runs in updateBattleRecord() when a fighter wins
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        ...
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {
            ...
            if (curStakeAtRisk > 0) { //amount to reclaim can be nonzero if there is nonzero stakeAtRisk for the fighter NFT
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk; //amountStaked is incremented without re-locking NFT transfer
            }

Therefore, it's possible to transfer a fighter that has nonzero stake. The attacker can then take advantage of underflow in RankedBattle by transferring the fighter to a fresh address with zero accumulated points, and the fighter will not be able to lose. Notice below that calls to updateBattleRecord() for a loss will revert:

(see inline comments)

        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            //stakingFactor is calculated based on amountStaked, which is nonzero for the attacker's NFT
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }
        ...
        } else if (battleResult == 2) {
            /// If the user lost the match
            ...
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor; //points will be nonzero since stakingFactor is nonzero
                ...
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
                accumulatedPointsPerAddress[fighterOwner][roundId] -= points; //underflow will occur here since points is nonzero and accumulatedPointsPerAddress is zero (NFT is owned by a fresh address). The fighter can't lose
                totalAccumulatedPoints[roundId] -= points;

Furthermore, due to a check in RankedBattle.stakeNRN(), the attacker can stake more NRN for the fighter without relocking NFT transfer:

            if (amountStaked[tokenId] == 0) { //NFT transfer is only re-locked if the amountStaked is zero
                _fighterFarmInstance.updateFighterStaking(tokenId, true);
            }

After the fighter wins and points are increased, the attacker can transfer the NFT to a new address- once again granting immunity to loss.

Combine this issue with the fact that transferring the fighter NFT to a new address grants full voltage, and it's very possible for the attacker to gain a huge amount of points.

See commented PoC below, run in RankedBattle.t.sol:

    function testTransferringNFTWithNonzeroStakeAndLossImmunity() public {
        //setup things for the player/exploiter
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(player, 8_000 * 10 ** 18);
        //do some setup so that the total points in the round is nonzero, otherwise new round can't be set. This simulates a live environment
        address _setUp = vm.addr(6);
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(_setUp, 8_000 * 10 ** 18);
        _mintFromMergingPool(_setUp);
        vm.startPrank(_setUp);
        _rankedBattleContract.stakeNRN(8_000 * 10 ** 18, 1);
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);

        //Start exploit
        //player stakes
        vm.startPrank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        //fighter loses and tokens are sent to StakeAtRisk
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        //player unstakes remaining stake to unlock NFT transfer
        vm.startPrank(player);
        _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(0), 0);
        //fighter wins and as a result its amountStaked is increased (becomes nonzero). (Fighter can now be transferred with nonzero stake, but we won't use this yet)
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        console.log(_rankedBattleContract.amountStaked(0)); //nonzero
        //set new round (NFTs can't unstake and then restake in the same round)
        vm.startPrank(_ownerAddress);
        _rankedBattleContract.setNewRound();
        //fighter wins so that it has nonzero points in the round
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        //player transfers NFT to another address while amountStaked is nonzero (this is an exploit)
        address playerAlt = vm.addr(4);
        vm.startPrank(player);
        _fighterFarmContract.transferFrom(player, playerAlt, 0);
        _neuronContract.transfer(playerAlt, 3_000 * 10 ** 18); //give the alt address some NRN for later use
        //alt address can change amountStaked freely (just can't fully unstake)
        vm.startPrank(playerAlt);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        //alt address can't lose due to underflow (updateBattleRecord will revert if the fighter lost)
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert(); //can check call trace to see "Arithmetic over/underflow" error message
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        //address can win
        console.log(_rankedBattleContract.accumulatedPointsPerFighter(0, 1)); //log points before win
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        console.log(_rankedBattleContract.accumulatedPointsPerFighter(0, 1)); //log new points, increased
        //alt address can still transfer the NFT
        vm.startPrank(playerAlt);
        address playerAlt1 = vm.addr(5);
        _fighterFarmContract.transferFrom(playerAlt, playerAlt1, 0);
        //fund the new address
        vm.startPrank(_treasuryAddress);
        _neuronContract.transfer(playerAlt1, 3_000 * 10 ** 18);
        //new address can change amountStaked freely, is immune to losing, and can win points
        vm.startPrank(playerAlt1);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        _rankedBattleContract.unstakeNRN(2_000 * 10 ** 18, 0);
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert(); //can check call trace to see "Arithmetic over/underflow" error message
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        //address can win
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        console.log(_rankedBattleContract.accumulatedPointsPerFighter(0, 1)); //log new points, increased
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

The root cause of this exploit is the ability to transfer a fighter with nonzero stake to an address with zero points; this can be prevented by adding a check to updateBattleRecord() so that it will re-lock transfer for an NFT whose points are being set from zero to nonzero.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

DoS of winning and loss of funds for the affected player.

Proof of Concept

When a fighter with funds in the StakeAtRisk contract wins a battle, RankedBattle.updateBattleRecord() will reclaim those funds by calling StakeAtRisk.reclaimNRN(). The issue arises because this function tracks NRN to reclaim by the user, not just by the fighter NFT. See the code below and inline comment:

    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        require(
            stakeAtRisk[roundId][fighterId] >= nrnToReclaim, 
            "Fighter does not have enough stake at risk"
        );

        bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
        if (success) {
            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[roundId] -= nrnToReclaim;
            amountLost[fighterOwner] -= nrnToReclaim; //This line can underflow
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

Because the function decrements the amountLost for the NFT owner, it's possible for an NFT with existing stakeAtRisk to be transferred to a new owner, subsequently win a battle, decrement the new owner's amountLost and cause underflow and the winning transaction to revert. There are 2 cases for which this can happen- the first is where the new owner has zero amountLost, and any win by the transferred NFT will revert; the second is where the new owner has nonzero amountLost, the transferred NFT wins and reduces the amountLost, and then underflow occurs when another fighter NFT owned by the transferee wins. The impact is a DoS of winning and loss of NRN funds for the user, as the user is unable to win his funds back and the stakeAtRisk is swept and transferred to the protocol at the end of every round.

See the below test case and inline comments, run in RankedBattle.t.sol:

    function testTransferringNFTDoSWinning() public {
        //setup things for the player/exploiter
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(player, 3_000 * 10 ** 18);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(10 ** 18, 0); //player stakes a tiny amount
        //setup for the victim
        address victim = vm.addr(6);
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(victim, 3_000 * 10 ** 18);
        _mintFromMergingPool(victim);
        vm.startPrank(victim);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 1); //victim stakes a substantial amount

        //victim loses a substantial amount of stake to StakeAtRisk contract
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true);
        //player loses a tiny amount of stake to StakeAtRisk contract
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        //player unstakes fully and transfers NFT to victim
        vm.startPrank(player);
        _rankedBattleContract.unstakeNRN(_rankedBattleContract.amountStaked(0), 0);
        _fighterFarmContract.transferFrom(player, victim, 0);
        //transferred NFT wins and reduces victim's `StakeAtRisk.amountLost`
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        //victim's other NFT wins, but the call to updateBattleRecord() reverts and the victim's substantial amount of stakeAtRisk is not recovered
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

The easiest fix is to delete the StakeAtRisk.amountLost state variable, as it's only used to track data.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L105-L110 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L333-L346

Vulnerability details

Impact

Since voltage restrictions can be ignored and an unlimited amount of fights can be initiated for one fighter NFT, the following impacts can occur:

The point distribution for all the players may become inappropriately adjusted, since the exploiter can initiate an unlimited amount of fights. The exploiter may earn too many points, and the exploiter may inappropriately reduce other players' points.

Gas griefing of the game server with a high damage/cost ratio by initiating an unlimited amount of fights.

Proof of Concept

Because voltage is tracked by addresses (NFT owners) instead of by the individual NFTs, players can easily bypass voltage restrictions simply by transferring their fighters to another address. If they have an existing stake and wish to continue earning points, they can unstake their NRN and restake using the new address.

Notice below how voltage is tracked by owner address rather than NFT id. VoltageManager:

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

RankedBattle calls spendVoltage() with the NFT owner address:

    function updateBattleRecord(
        ...
        address fighterOwner = _fighterFarmInstance.ownerOf(tokenId);
        ...
        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);

Therefore, players can initiate an unlimited amount of battles, which is not supposed to be allowed. This enables gas griefing of the game server, as well as inappropriate player point updates that potentially result in benefit to the attacker, as described in the Impact section.

Test case for gas griefing below, run in RankedBattle.t.sol. The gas cost for transferring the NFT is around 60_000, and the gas cost for updating the battle record 10 times is around 350_000. See the console.logs. (Foundry does not include the EVM G_transaction base fee of 21_000 paid for every transaction.)

    function testUnboundedVoltageGasGrief() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        //10 battles
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        //transfer NFT to new address
        address player1 = vm.addr(4);
        vm.startPrank(player);
        //log gas used in transfer
        uint gas = gasleft();
        _fighterFarmContract.transferFrom(player, player1, 0);
        console.log(gas - gasleft());

        //log gas griefed
        gas = gasleft();
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        //unauthorized 10 battles
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        console.log(gas - gasleft());
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Track voltage based on NFT IDs instead of addresses.

Assessed type

Other

QA report by 0xDetermination

[L-1]<a name="l-1"></a> Discrepancy between docs and code- false initiatorBool argument for RankedBattle.updateBattleRecord() should not change a fighter's points according to docs

Links

https://docs.aiarena.io/gaming-competition/nrn-rewards#0e10abc361f64aebae0ed48432213724

Description

From the docs linked above: 'Only the Challenger NFT accrues Points during a match. For the Opponent NFT, the outcome does not impact their Point total.' However, the code in RankedBattle.updateBattleRecord() only uses the initiatorBool to check whether or not the challenger has sufficient voltage. (Link to the function: https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L322-L349)

Therefore, a non-challenger will have its points updated.

Recommended Fix

Refactor the code to reflect the docs, or update the docs.

[L-2]<a name="l-2"></a> Small stake amounts can't be lost to StakeAtRisk due to precision loss

Links

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

Description

Notice how RankedBattle.updateBattleRecord() calculates the potential points earned and potential stakeAtRisk to take from the player:

    function _addResultPoints(
        ...
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4; //Precision loss in stakeAtRisk calculation
        ...
            points = stakingFactor[tokenId] * eloFactor; //Points are calculated based on stakingFactor

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

Because there is precision loss in the curStakeAtRisk calculation and the user's stakingFactor is set to 1 if it was calculated as 0, a user can have a zero curStakeAtRisk while earning a nonzero amount of points.

Users with small stake can essentially play risk-free and can earn more points than they should. However, because they won't win many points due to the very small stake, the protocol might desire to ignore this issue.

Recommended Fix

Don't set the stakingFactor to 1 if it's calculated as 0.

[L-3]<a name="l-3"></a> RankedBattle.claimNRN() provides extra attack surface in case of an attacker inflating claimableNRN value

Links

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

Description

Notice below that the reward NRN is minted directly instead of being distributed from a reward pool:

        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);

If an attacker is able to inflate the claimableNRN value somehow, an uncapped amount of NRN may be minted. This could destroy the protocol's tokenomics.

Recommended Fix

Consider enforcing a reasonable max minting value, or transferring existing NRN instead of minting.

[L-4]<a name="l-4"></a> New users to the protocol joining a significant amount of time after project launch will incur huge gas fees when calling RankedBattle.claimNRN()

Links

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

Description

Notice in the below code that the for loop will run for every completed round:

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

Since there are at least 4 SLOADS in the loop and 1 SSTORE, the gas cost of each loop will be around or over 5000. If a user joins a significant amount of time after protocol launch, they could incur a huge gas cost and lose a lot of gas fees. For example, 200 rounds may pass after 2 years (the sponsor has stated that each round will be last for around half a week). In this case, the user may have to consume 1 million gas for the transaction.

At current average Arbitrum gas prices, this will cost around 14 USDC.

Recommended Fix

Allow users to set the lowerBound for the loop to an arbitrary round and update their numRoundsClaimed to that round.

[L-5]<a name="l-5"></a> FighterFarm.IncrementGeneration() should enforce a fighterType of 0 or 1

Links

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

Description

There are only two fighter types (0 or 1), so it should not be possible to call incrementGeneration() with an arbitrary uint8. This may result in an increased attack surface.

    /// @param fighterType Type of fighter either 0 or 1 (champion or dendroid).
    /// @return Generation count of the fighter type.
    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

Recommended Fix

Enforce the fighter type, either with a require statement or changing the parameter type to bool.

[L-6]<a name="l-6"></a> MergingPool.claimRewards() can mint an NFT with out-of-range fighter weight

Links

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L139-L159 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L313-L331 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L484-L527

Description

MergingPool.claimRewards() doesn't perform input validation on the customAttributes arg, allowing the user to mint an NFT with an out-of-range weight. (Weight range is [65-95] https://discord.com/channels/810916927919620096/1201981655850426399/1208908219754221568)

See call trace below:

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

    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter( //next call
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }
    ...
    function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
        uint256 element; 
        uint256 weight;
        uint256 newDna;
        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else { //this block runs and sets arbitrary args
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }
        ...
        fighters.push(
            FighterOps.Fighter(
                weight,
                element,
                attrs,
                newId,
                modelHash,
                modelType,
                generation[fighterType],
                iconsType,
                dendroidBool
            )
        );

Recommended Fix

Perform appropriate input validation in MergingPool.claimRewards().

[L-7]<a name="l-7"></a> Ensure off-chain validation is performed to check player voltage and prevent the game server from being gas griefed

Links

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

Description

RankedBattle.updateBattleRecord() validates that a game initiator has enough voltage before updating the record. If this validation is only done on-chain, a malicious user may be able to spam initiate fights to gas grief the game server.

    function updateBattleRecord(
        ...
        require(
            !initiatorBool ||
            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
        );

Recommended Fix

Ensure player voltage is checked off-chain before allowing them to start a battle.

[I-1]<a name="i-1"></a> Comments for win/lose case can be put on the same line as the conditional statement for clarity

Links

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L440-L441 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L472-L473

Description

The comments for win/lose case of 0/2 in RankedBattle._addResultPoints() are below the conditional statements. These can be put inline with the conditional statements for clarity:

        if (battleResult == 0) {
            /// If the user won the match
        ...
        } else if (battleResult == 2) {
            /// If the user lost the match

Recommended Fix

Move the comments one line up.

[I-2]<a name="i-2"></a> Consider calling GameItems.instantiateNeuronContract() in the constructor

Links

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

Description

The constructor in GameItems.sol doesn't set the NRN contract:

    constructor(address ownerAddress, address treasuryAddress_) ERC1155("https://ipfs.io/ipfs/") {
        _ownerAddress = ownerAddress;
        treasuryAddress = treasuryAddress_;
        isAdmin[_ownerAddress] = true;
    }

Consider setting the NRN contract in the constructor to avoid accidentally leaving it unset and the contract not working properly.

Recommended Fix

[I-3]<a name="i-3"></a> success bool checks on NRN transfers are not necessary because the OZ implementation will not silently fail

Links

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

Description

There are many places in the codebase that follow the pattern:

        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {

These checks are unnecessary, because the OZ implementation of ERC20 will always revert if the transfer has failed.

Recommended Fix

Remove the transfer success caches/checks.

[I-4]<a name="i-4"></a> Explicitly declare state variable visibility for code clarity

Links

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

Description

Consider explicitly declaring the below state variables in RankedBattle.sol as internal.

    /// The StakeAtRisk contract address.
    address _stakeAtRiskAddress;

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

    /// @notice The address in charge of updating battle records.
    address _gameServerAddress;

Recommended Fix

See description.

[I-5]<a name="i-5"></a> Ambiguous NatSpec for MergingPool.claimRewards()

Links

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

Description

The NatSpec below is somewhat confusing/ambiguous.

    /// @dev The user can only claim rewards once for each round.
    /// @param modelURIs The array of model URIs corresponding to each round and winner address.
    /// @param modelTypes The array of model types corresponding to each round and winner address.
    /// @param customAttributes Array with [element, weight] of the newly created fighter.
    function claimRewards(

'The user can only claim rewards once for each round' may imply that users can only claim one NFT per round, but that is not true since the pickWinner() function can pick the same address as a winner multiple times in a round (https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L125).

Likewise, the phrasing of 'The array of model URIs corresponding to each round and winner address' somewhat implies that users can only claim one NFT per round. Also, 'corresponding to each round and winner address' implies that the arguments may include NFT data for more than one user, but this is not the case. (Each user can only claim for themselves.)

Recommended Fix

Refactor the NatSpec.

[I-6]<a name="i-6"></a> Consider adding a minId param to MergingPool.getFighterPoints()

Links

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

Description

MergingPool.getFighterPoints() allows the caller to specify a max token ID up to which fighter points will be retrieved. Consider adding a minId as well to improve readability since there will be many tokens. This will not introduce extra attack surface area since the function is not used anywhere in the code.

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

Recommended Fix

Set i to a user-provided minId.

[I-7]<a name="i-7"></a> Neuron.burnFrom() is not used in the protocol and can be removed to reduce attack surface area

Links

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

Description

Since the burnFrom() function is not used anywhere in the code, it can be deleted to reduce attack surface area.

Recommended Fix

Delete burnFrom().

[I-8]<a name="i-8"></a> FighterFarm.reRoll() NatSpec should specify pseudo-randomness

Links

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

Description

The NatSpec 'Rolls a new fighter with random traits' is somewhat misleading; trait determination is pseudorandom.

Recommended Fix

Change the comment to 'Rerolls a fighter with pseudorandom traits.'

[I-9]<a name="i-9"></a> Fighter generations can't increase past 255; FighterFarm.incrementGeneration() will fail with overflow

Links

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

Description

Notice below that due to the type of maxRerollsAllowed, fighter generation can't be incremented past 255:

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

Recommended Fix

The type of maxRerollsAllowed and related variables can be changed to a larger uint type if the protocol anticipates incrementing fighter generations past 255.

[I-10]<a name="i-10"></a> Consider using interfaces instead of importing contracts directly

Links

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

Description

The contracts in the codebase follow a pattern of importing contracts directly instead of using interfaces. (The common practice is to use interfaces, although this seems to be a style choice.)

Recommended Fix

Use interfaces if the protocol desires that style choice.

[I-11]<a name="i-11"></a> FighterFarm.sol doesn't need to inherit ERC721 since it already inherits ERC721Enumerable

Links

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.9/contracts/token/ERC721/extensions/ERC721Enumerable.sol#L14 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L16

Description

Solidity inheritance is ordered right-to-left and ERC721Enumerable already inherits ERC721. The overrides and super calls will be unchanged if the contract only inherits ERC721Enumerable.

contract FighterFarm is ERC721, ERC721Enumerable {

Recommended Fix

Only inheriting ERC721Enumerable.

[I-12]<a name="i-12"></a> Fighter NFT properties may change unexpectedly after being transferred to a buyer (for example, if the fighter entered a match just before the sale)

Links

n/a

Description

Since fighters are always available for battles, the NFT properties (such as win stats) could change unexpectedly for a buyer. This can happen if the fighter entered a match just before the sale. Note that whether or not the fighter is in a battle is not recorded on-chain.

Recommended Fix

There should be an offchain mechanism for buyers to see whether or not an NFT is engaged in battle. It may not be practical to attempt to prevent this issue on-chain since fighters should always be available for battles.

Gas report by 0xDetermination

[G-1]<a name="g-1"></a> Neuron.claim() allowance check is not necessary because transferFrom() also checks allowance

Links

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

Description

The require statement below can be removed since transferFrom() will check/update the allowance.

    function claim(uint256 amount) external {
        require(
            allowance(treasuryAddress, msg.sender) >= amount, 
            "ERC20: claim amount exceeds allowance"
        );
        transferFrom(treasuryAddress, msg.sender, amount);
        emit TokensClaimed(msg.sender, amount);
    }

Recommended Fix

Delete the require statement.

[G-2]<a name="g-2"></a> Neuron.claim() can call code in transferFrom() implementation directly instead of calling transferFrom()

Links

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.9/contracts/token/ERC20/ERC20.sol#L158-L163 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L143

Description

See the OZ transferFrom() implementation:

    function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
        address spender = _msgSender();
        _spendAllowance(from, spender, amount);
        _transfer(from, to, amount);
        return true;
    }

Neuron.claim() can run the above code directly without spending extra gas for the function call and to cache msg.sender.

Recommended Fix

    function claim(uint256 amount) external {
        require(
            allowance(treasuryAddress, msg.sender) >= amount, 
            "ERC20: claim amount exceeds allowance"
        );
-       transferFrom(treasuryAddress, msg.sender, amount);
+       _spendAllowance(treasuryAddress, msg.sender, amount);
+       _transfer(treasuryAddress, msg.sender, amount);
        emit TokensClaimed(msg.sender, amount);
    }

[G-3]<a name="g-3"></a> Neuron.burnFrom() allowance check is not necessary

Links

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

Description

Note below that if the amount exceeds the allowance, the decreasedAllowance calculation will underflow and cause a revert. The require allowance check can be removed.

    function burnFrom(address account, uint256 amount) public virtual {
        require(
            allowance(account, msg.sender) >= amount, 
            "ERC20: burn amount exceeds allowance"
        );
        uint256 decreasedAllowance = allowance(account, msg.sender) - amount;
        _burn(account, amount);
        _approve(account, msg.sender, decreasedAllowance);
    }

Recommended Fix

Remove the require statement.

[G-4]<a name="g-4"></a> RankedBattle._getStakingFactor() accesses the same storage variable a second time whenever it's called (in stakeNRN(), unstakeNRN(), and updateBattleRecord())

Links

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L528 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L253-L258 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L272-L277 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L342

Description

RankedBattle._getStakingFactor() accesses the storage variable amountStaked[tokenId] after the three mentioned functions access the same variable (see links). This variable should be cached to decrease storage reads and gas costs.

Recommended Fix

Cache amountStaked[tokenId] and pass it to _getStakingFactor().

[G-5]<a name="g-5"></a> NRN success checks are unnecessary because the OZ implementation will not fail without reverting

Links

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

Description

The pattern in the link provided (checking for NRN transfer success) is used widely in the codebase, but these checks are unnecessary since transfers won't fail without reverting.

Recommended Fix

Remove the NRN success checks.

[G-6]<a name="g-6"></a> RankedBattle.claimNRN() is gas inefficient for addresses that don't have any points in previous rounds

Links

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

Description

The for loop in claimNRN() will start at zero for new users and spend thousands of gas accessing/setting storage in each loop, although the user will not have any rewards in earlier rounds.

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

Recommended Fix

Allow users to set their numRoundsClaimed to an arbitrary value.

[G-7]<a name="g-7"></a> RankedBattle.updateBattleRecord() doesn't need to check voltage since VoltageManager.spendVoltage() will revert if voltage is too low

Links

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

Description

updateBattleRecord() checks that the initiator's voltage is sufficient:

        require(
            !initiatorBool ||
            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
        );

This check is not necessary, since VoltageManager.spendVoltage() will revert if the voltage is too low:

    function spendVoltage(address spender, uint8 voltageSpent) public {
        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
        if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
            _replenishVoltage(spender);
        }
        ownerVoltage[spender] -= voltageSpent; // UNDERFLOWS AND REVERTS IF VOLTAGE IS TOO LOW
        emit VoltageRemaining(spender, ownerVoltage[spender]);
    }

Note that this check can also be performed offchain before calling updateBattleRecord().

Recommended Fix

Remove the require statement.

[G-8]<a name="g-8"></a> RankedBattle._addResultPoints() can put code inside an existing conditional statement to avoid incrementing storage variables by zero

Links

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

Description

See the following code from the RankedBattle._addResultPoints() below. The points variable can be zero, such as if the player's mergingFactor is 100. The lines updating storage variables with points should be put inside the if block.

            accumulatedPointsPerFighter[tokenId][roundId] += points;
            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
            totalAccumulatedPoints[roundId] += points;
            if (points > 0) {
                emit PointsChanged(tokenId, points, true);
            }

Recommended Fix

-           accumulatedPointsPerFighter[tokenId][roundId] += points;
-           accumulatedPointsPerAddress[fighterOwner][roundId] += points;
-           totalAccumulatedPoints[roundId] += points;
            if (points > 0) {
+               accumulatedPointsPerFighter[tokenId][roundId] += points;
+               accumulatedPointsPerAddress[fighterOwner][roundId] += points;
+               totalAccumulatedPoints[roundId] += points;
                emit PointsChanged(tokenId, points, true);
            }

[G-9]<a name="g-9"></a> RankedBattle._addResultPoints() executes unnecessary lines of code in case of a tie

Links

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

Description

RankedBattle._addResultPoints() runs all of the below code in case of a tie; it's all setup for the win/lose code blocks, which make up the rest of the function. There's no more code that runs in case of a tie. The only state modification in this function in case of a tie is updating the stakingFactor if it was not already updated, but since points/stake does not change in case of a tie the update is not necessary.

        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;
        // THE REST OF THE CODE IN THIS FUNCTION ONLY RUNS IN CASE OF A WIN OR LOSS

Recommended Fix

Cut the code above and paste it at the top of both the win and lose blocks.

[G-10]<a name="g-10"></a> Unnecessary equality comparison in RankedBattle._updateRecord()

Links

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

Description

There are only three cases for battleResult, so below it's not necessary to check the result 3 times. An else block can replace the second else if block.

    function _updateRecord(uint256 tokenId, uint8 battleResult) private {
        if (battleResult == 0) {
            fighterBattleRecord[tokenId].wins += 1;
        } else if (battleResult == 1) {
            fighterBattleRecord[tokenId].ties += 1;
        } else if (battleResult == 2) {
            fighterBattleRecord[tokenId].loses += 1;
        }
    }

Recommended Fix

-       } else if (battleResult == 2) {
+       } else {

[G-11]<a name="g-11"></a> FighterFarm.sol inherits ERC721Enumerable but the codebase doesn't use any of the enumerable methods

Links

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

Description

Solely inheriting ERC721 in the FighterFarm contract would save on deployment costs since the codebase doesn't use the enumerable methods.

Recommended Fix

If desired, don't inherit from ERC721Enumerable.

[G-12]<a name="g-12"></a> Storing variable types smaller than 32 bytes costs more gas than types with 32 bytes and doesn't save cold SLOAD gas in FighterFarm.sol

Links

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

Description

See below code for an example:

    /// @notice Mapping to keep track of how many times an nft has been re-rolled.
    mapping(uint256 => uint8) public numRerolls;

The EVM will spend more gas to store a smaller type like uint8 compared to a 32 byte type like uint.

See the links for more occurrences.

Recommended Fix

Consider changing the mapped-to types to 32 byte types like uint.

[G-13]<a name="g-13"></a> Balance check in FighterFarm.reRoll() is not necessary since transferFrom() will revert in case of insufficient balance

Links

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

Description

In the function below, the balance require check is not necessary due to transferFrom() implementation.

    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"); //Can delete

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost); //Will revert if balance is insufficient

Recommended Fix

Delete the balanceOf() require check.

[G-14]<a name="g-14"></a> FighterFarm._beforeTokenTransfer() can be deleted to save an extra function call

Links

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L451 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.9/contracts/token/ERC721/extensions/ERC721Enumerable.sol#L60

Description

Note the code below:

contract FighterFarm is ERC721, ERC721Enumerable {
    ...
    function _beforeTokenTransfer(address from, address to, uint256 tokenId)
        internal
        override(ERC721, ERC721Enumerable)
    {
        super._beforeTokenTransfer(from, to, tokenId);
    }

Inheritance in solidity is right-to-left and super._beforeTokenTransfer(from, to, tokenId); will call the _beforeTokenTransfer() method in ERC721Enumerable. Therefore, this function can be deleted and the functionality will be the same while eliminating an extra function call and saving gas.

Recommended Fix

Remove FighterFarm._beforeTokenTransfer().

[G-15]<a name="g-15"></a> FighterFarm._createFighterBase() performs unnecessary check when minting dendroid

Links

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L484-L531 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L462-L474 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L83-L94

Description

Notice below that _createNewFighter() only uses newDna when calling AiArenaHelper.createPhysicalAttributes():

    function _createNewFighter(
        ...
        uint256 element; 
        uint256 weight;
        uint256 newDna;
        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }
        uint256 newId = fighters.length;

        bool dendroidBool = fighterType == 1;
        FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            iconsType,
            dendroidBool
        );
        //newDna is not used after this point
    ...
    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); //This check is not needed
        return (element, weight, newDna);
    }

The above check in _createFighterBase() that sets dendroid fighters' (fighterType equals 1) newDna to a different value is not necessary, because newDna will not be used in AiArenaHelper.createPhysicalAttributes() when creating a dendroid and there are only two fighter types:

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

Recommended Fix

Instances of newDna can be removed and dna used instead, since dna will not change for non-dendroid fighters and newDna is not used when creating a dendroid fighter.

[G-16]<a name="g-16"></a> MergingPool.claimRewards() should update numRoundsClaimed after the loop ends instead of incrementing it in every loop

Links

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

Description

The below code is not gas efficient due to SSTORE/SLOAD of the numRoundsClaimed state variable in every loop.

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;

Recommended Fix

Instead of incrementing the state variable in every loop, increment a local variable and then update the state variable after the loop ends.

[G-17]<a name="g-17"></a> MergingPool.pickWinner() and MergingPool.claimRewards() can be refactored so that users don't have to iterate through the entire array of winners for every round

Links

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

Description

Notice how claimRewards() detects if the caller has rewards to claim in a round:

            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {

This method is very gas inefficient since winnerAddresses is in storage.

Recommended Fix

Instead of iterating through all the winner addresses in every round, MergingPool.pickWinner() and MergingPool.claimRewards() could be refactored to increment a new state variable tracking number of rewards (per round, if desired).

[G-18]<a name="g-18"></a> DNA rarity calculation in AiArenaHelper.dnaToIndex() can be redesigned to save gas

Links

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

Description

dnaToIndex() is called inside a loop for all the fighter physical properties (see first link), which consumes a lot of gas due to storage loads. The system of calculating attributeProbabilityIndex by looping through the probabilities and comparing the cumProb to rarityRank could be refactored to retain the same functionality but save a lot of gas.

dnaToIndex() function below for reader convenience:

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

Recommended Fix

Refactor attribute index/rarity calculation.

[G-19]<a name="g-19"></a> AiArenaHelper.createPhysicalAttributes() should check for iconsType before running icons-relevant code since icons fighters are rare

Links

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

Description

Since icon fighter NFTs are rare, the below checks related to icons should not run for non-icon fighters.

            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 {
                    //set finalAttributeProbabilityIndexes[i] a different way

Recommended Fix

Instead of running through all 3 icons checks every time, check to see if iconsType is greater than zero before running through the 3 checks.

[G-20]<a name="g-20"></a> AiArenaHelper constructor sets probabilities twice

Links

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

Description

Notice below that the constructor sets the probabilities twice:

    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];
            attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
        }
    } 

Recommended Fix

Remove the for loop.