AI Arena - MidgarAudits's results

Lines of code

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

Vulnerability details

Impact

The FighterFarm.sol which inherits from ERC721 has overridden the safeTransferFrom(from, to, tokenId) and added requirements which lie in _ableToTransfer(). However, the ERC721 has another safeTransferFrom(from, to, tokenId, data) which has not been overridden. This means that a user can transfer their fighter and disregard the requirements inside of _ableToTransfer(). A malicious user could therefore, circumvent the requirements of not being able to send more than 10 fighters to a user balanceOf(to) < MAX_FIGHTERS_ALLOWED and being able to transfer a figher even though it is staked !fighterStaked[tokenId].

Proof of Concept

The following POC can be implemented in the FighterFarm.t.sol.

    function test_TransferringFighterWhileStakedSucceeds() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);

        //Stake the fighter, which means that it should not be transferrable
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        // Reverts with the overridden transferFrom function
        vm.expectRevert();
        _fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);

        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // Succeeds with the safeTransferFrom function not overridden
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");

        // The fighter is now owned by the delegated address despite it being staked
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0) != _ownerAddress, true);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Override the safeTransferFrom(from, to, tokenId, data) function and either add a revert inside of the overridden function or add the _ableToTransfer() there.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When creating an item, the admin can decide to make the item non-transferable by setting transferable = false. This requirement is checked in the overridden function safeTransferFrom() in GameItems.sol through require(allGameItemAttributes[tokenId].transferable). However, the contract have not overridden the ERC1155 function safeBatchTransferFrom() and implemented the same restriction. A malicious user could transfer an item that is supposed to be non-transferable, and gain an unfair competitive advantage.

Proof of Concept

The following POC can be tested by putting the function in the GameItems.t.sol file.

    function test_transferNonTransferable() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        // Item adjusted to not be transferable
        _gameItemsContract.adjustTransferability(0, false);
        // The safeTransferFrom should fail because the game item is not transferable.
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // The safeBatchTransferFrom will succeed, since it is not being overwritten
        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        ids[0] = 0;
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
        // The _DELEGATED_ADDRESS is now in possession of the game item and not the _ownerAddress
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Override safeBatchTransferFrom() and add require(allGameItemAttributes[tokenId].transferable) such as below:

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable); 
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Within the function updateBattlerRecord(), there is an internal call to _addResultPoints(), which calculates curStakeAtRisk. However, curStakeAtRisk is rounded down, potentially allowing a player with no points remaining to unstake until they have only 999 NRNs staked. This enables the player to avoid risking their NRNs if they lose a match, allowing them to recover points in a 'risk-free' manner, as no NRNs will be put at risk if they lose. Such strategies decrease the amount of NRNs sent to StakeAtRisk.sol and subsequently the amount sent to the treasury when a new round is initiated.

Proof of Concept

Add the following test to RankedBattle.t.sol:

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

        //Stake 999 NRNs with 0 points
        vm.prank(player);
        _rankedBattleContract.stakeNRN(999, 0);
        emit log_uint(_rankedBattleContract.accumulatedPointsPerAddress(player, 0));
        assertEq(_rankedBattleContract.amountStaked(0), 999);

        // player lost the match with 0 accumulated points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

        // 0 NRNs placed at risk
        assertEq(_rankedBattleContract.amountStaked(0), 999);
    }

Tools Used

Manual review

Recommended Mitigation Steps

The rounding down is in favor of the user if he lost a match, however it should always be in favor of the system. Make sure curStakeAtRisk is never 0 if the player lost the match by adding the following check right after else if (battleResult == 2) L472:

if (curStakeAtRisk == 0) {
                curStakeAtRisk = 1;
}

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

In the FighterFarm.sol contract the numElements[0] is set to 3 in the constructor (representing the 3 elements in the game). However, whenever the owner decides to call incrementGeneration(fighterType), users that try to create a new fighter of that generation with customAttributes (e.g. customAttributes[0] == 100) will revert in _createFighterBase. The same will happen if a user decides to try and reRoll() an existing fighter to a new fighter of that generation. The revert will happen in this line:

uint256 element = dna % numElements[generation[fighterType]]; 

Since numElements only have a mapping from 0 (numElements[0] = 3). The new incremented generation will try to access numElements[1] which will default to 0, thus causing a division by 0 issue. The impact is severe since this means that users will not be able to create or reRoll to fighters of the incremented generation with customAttributes.

Proof of Concept

_createNewFighter() fails

    function test_createNewFighterInNewGenerationReverts() public {
        // Mint a fighter successfully in generation 0
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // Increment AI champion to generation 1 and try to mint it with custom attributes
        _fighterFarmContract.incrementGeneration(0);
        vm.prank(address(_mergingPoolContract));
        vm.expectRevert();
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
    }

reRoll() fails

    function test_reRollNewGenerationReverts() public {
        uint8 tokenId = 0;
        // fund user with 4k NRN for reroll
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // Mint a fighter successfully in generation 0
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        assertEq(_fighterFarmContract.ownerOf(tokenId), _ownerAddress);

        // Increment AI champion to generation 1
        _fighterFarmContract.incrementGeneration(0);
        _neuronContract.addSpender(address(_fighterFarmContract));
        // reRoll fails
        vm.expectRevert();
        _fighterFarmContract.reRoll(tokenId, 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

When incrementing the generation - make sure to create a mapping for numElements as well.

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

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L68-L76

Vulnerability details

Impact

Neuron.sol implements the AccessControl.sol from OpenZeppelin and utilizes the lib to set up the MINTER_ROLE, STAKER_ROLE, and SPENDER_ROLE for different contracts. However, there's currently no way to revoke these roles nor grant new roles. The reason being that the DEFAULT_ADMIN_ROLE is never set. In case, the addresses do get compromised this means that a malicious user could call spend, stake, and most critically mint unlimited NRN.

Tools Used

Manual review

Recommended Mitigation Steps

Set the contract deployer as the DEFAULT_ADMIN_ROLE such as below:

constructor() {
...
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender)
}

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

In the FighterFarm.sol contract the ownerAddress can add a new address to the hasStakerRole mapping. The addresses that are added to this mapping can then call the updateFighterStaking() function to update the fighter staking status. However, if the account for that address becomes compromised there's currently no way to revoke the access - meaning that a malicious user could arbitrarily update the staking status of fighters which could lead to unforseen consequences (e.g. unlocking a fighter even though NRN are being staked and so on).

Tools Used

Manual review

Recommended Mitigation Steps

Consider changing the function to allow for a boolean such as :

    function addStaker(address newStaker, bool allowed) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = allowed;
    }

This follows the same pattern as adjustAllowedVoltageSpenders.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

In the functions claimRewards() and claimNRN() the user can batch claim their rewards in the MergingPool.sol and RankedBattle.sol respectively. However, there are no customizeable upper bound to the for-loops. Right now the loop goes from the numRoundsClaimed to the current roundId.

Proof of Concept

function setUpPickWinners(uint256 rounds) internal {
    // Set the winners to two, since that is required per period
    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(_DELEGATED_ADDRESS);

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

    // Winners are picked for each round
    for(uint256 i = 0; i < rounds; i++) {
        _mergingPoolContract.pickWinner(_winners);
    }
}

function test_ClaimRewardsRunsOutofGas() public {
    // Pick the winners isolated from the claimRewards function
    uint256 rounds = 10;
    setUpPickWinners(rounds); 
    // Winners preparation to claim rewards
    string[] memory _modelURIs = new string[](rounds);
    string[] memory _modelTypes = new string[](rounds);
    uint256[2][] memory _customAttributes = new uint256[2][](rounds);
    for(uint256 i = 0; i < rounds; i++) {
        _modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelTypes[i] = "original";
        _customAttributes[i] = [uint256(1), uint256(80)];
    }

    // winner tries claims rewards but runs out of gas
    vm.expectRevert();
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
}

Tools Used

Manual review

Recommended Mitigation Steps

Add a parameter to decide the upper bound for the loop such as below.

  function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
-       uint256[2][] calldata customAttributes
+       uint256[2][] calldata customAttributes,
+       uint256 _roundId
    ) 
        external 
    {
+       require(_roundId <= roundId, "_roundId cannot be higher than roundId");
        uint256 winnersLength;
        uint32 claimIndex = 0;
-       uint32 lowerBound = numRoundsClaimed[msg.sender];
-       for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
+       uint32 lowerBound = numRoundsClaimed[msg.sender];
+       for (uint32 currentRound = lowerBound; currentRound < _roundId; currentRound++) {
            ...
        }
    }

Assessed type

Loop

Lines of code

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

Vulnerability details

Impact

In the FighterFarm.sol contract, the reRoll() function allows the user to re-roll their fighter with new attributes up until maxRerollsAllowed (which is currently set at three times). The new dna are based on the msg.sender, tokenId and numRerolls[tokenId] parameters as shown below:

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

This means that a user can simulate the new dna off-chain (for example in Remix) since both the msg.sender and numRerolls[tokenId] relies on user input.

Proof of Concept

This can be abused by the user in order to try and create a rare NFT in a certain and deterministic way. Below is a step-by-step way to do it:

1- Set up an off-chain environment with Remix and implement the functions in order to simulate the dna and thus the rarity of your fighter.

2- Start by calling reRoll() with your fighter and EOA address. Do that three (3) times and see if you get a good fighter.

3- If you didn't get a fighter to your liking, create a new EOA and send the fighter over to this EOA together with some NRN.

4- Repeat step 2 and step 3 until you get a rare fighter.

5- Now, use that specific EOA together with the number of re-rolls required and execute it on-chain to get your rare fighter.

Tools Used

Manual review

Recommended Mitigation Steps

Use off-chain randomization for the fighter DNA, similar to how the redeemMintPass() function does it with the param mintPassDnas.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L185-L188

Vulnerability details

Impact

In the GameItems.sol contracts the admin can add a new address to the setAllowedBurningAddresses mapping. The addresses that are added to this mapping can then call the burn() function to burn certain items from accounts. However, if the account for that address becomes compromised there's currently no way to revoke the access - meaning that a malicious user could arbitrarily burn game items from all the accounts (e.g. griefing a user).

Tools Used

Manual review

Recommended Mitigation Steps

Consider changing the function to allow for a boolean such as :

    function setAllowedBurningAddresses(address newBurningAddress, bool allowed) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = allowed; //@audit no way to rescind burning addresses?
    }

This would follow the same pattern as the function adjustAllowedVoltageSpenders.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

If a player has spent their daily allowance, they can easily bypass it by minting from another EOA they control and transferring the token to their initial address. A player would be able to cheat in the game, for example if the GameItem is a healing potion capped at 1 potion per day, the player would be allowed to take far more than 1 potion and wins all the matches which gives him a huge advantage on other players.

Proof of Concept

Add the following lines at the end of the setup() function of RankedBattle.t.sol:

_neuronContract.addSpender(address(_gameItemsContract));
_gameItemsContract.instantiateNeuronContract(address(_neuronContract));
_gameItemsContract.createGameItem("Battery", "https://ipfs.io/ipfs/", true, true, 10_000, 1 * 10 ** 18, 10);
_gameItemsContract.setAllowedBurningAddresses(address(_voltageManagerContract));

Add the following test to RankedBattle.t.sol:

    function testBypassDailyAllowance() public {
        address player = vm.addr(3);
        address newEoa = vm.addr(4);
        _mintFromMergingPool(player);
        assertEq(_gameItemsContract.getAllowanceRemaining(player, 0), 10);
        _fundUserWith4kNeuronByTreasury(player);
        _fundUserWith4kNeuronByTreasury(newEoa);

        vm.prank(player);
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.getAllowanceRemaining(player, 0), 0);

        // transfer fighter to newEoa
        vm.prank(player);
        _fighterFarmContract.safeTransferFrom(player, newEoa, 0);

        // check if newEoa has daily allowance
        assertEq(_gameItemsContract.getAllowanceRemaining(newEoa, 0), 10);

        // mint more gameItems 
        vm.prank(newEoa);
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.getAllowanceRemaining(newEoa, 0), 0);

        // send gameItems back to player
        vm.prank(newEoa);
        _gameItemsContract.safeTransferFrom(newEoa, player, 0, 10, "");
        uint256 balanceOfItems = _gameItemsContract.balanceOf(player, 0);
        assertEq(balanceOfItems, 20);
    }

Now the player has more than the daily allowances.

Tools Used

Manual review

Recommended Mitigation Steps

Checking that the from address holds an NFT in safeTransferFrom() then lock the fighter to make it non transferable if allowanceRemaining = 0 would make such practices less feasible:

• GameItems.sol: add it to safeTransferFrom() require(FighterFarm.balanceOf(from) > 0); if (allowanceRemaining[msg.sender][tokenId] == 0) { FighterFarm.lockFighter();}
• FighterFarm.sol: add it to safeTransferFrom() require(!fighterLocked[tokenId]); Add a function to lock fighters:

function lockFighter(uint256 tokenId, bool action) external {
     require(msg.sender == _gameItemsInstance);
     fighterLocked[tokenId] = action;
}

Assessed type

Other