AI Arena - lil_eth's results

Lines of code

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

Vulnerability details

Impact

Non-transferable ERC1155 tokens in the GameItems contract can be bypassed via safeBatchTransferFrom implemented in openzeppelin/contracts/token/ERC1155/ERC1155.sol, compromising item transfer restrictions and potentially disrupting the game's economy.

Proof of Concept

GameItems enforces item transferability checks in safeTransferFrom but forgot to override safeBatchTransferFrom which allow any user to transfer any ERC1155 token from this contract, allowing restricted items to be transferred in batches.

Details

Add this test to GameItem.t.sol :

function testBypassNonTransferrable() public {
        address alice = address(8);
        _gameItemsContract.createGameItem("PowerPlus", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10);
        _fundUserWith4kNeuronByTreasury(msg.sender);
        vm.startPrank(msg.sender);
        _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
        _gameItemsContract.mint(1, 2);
        assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 2);
        assertEq(_gameItemsContract.balanceOf(msg.sender, 1),2);
        (,,bool transferrable,,,) = _gameItemsContract.allGameItemAttributes(1);
        assertEq(transferrable,false);
        
        // 1. safeTransferFrom Revert due to transferrable set to false
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(msg.sender,_ownerAddress,1,2,"");
        uint256[] memory ids = new uint256[](2); 
        ids[0] = 0;
        ids[1] = 1;
        uint256[] memory values = new uint256[](2); 
        values[0] = 1;
        values[1] = 2;
        
        // 2. safeBatchTransferFrom doesn't revert because not overriden
        _gameItemsContract.safeBatchTransferFrom(msg.sender,alice,ids,values,"");
        vm.stopPrank();
    }  

Tools Used

VSCode

Recommended Mitigation Steps

Override safeBatchTransferFrom() in GameItems to include transferable checks for batch transfers of Game Items

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

numElements mapping is used in fighterFarm.sol contract to reference number of elements per generation, we can define 2 types of NFT : the dendroid and the champions tokens, this mapping is initialized within fighterFarm constructor : numElements[0] = 3; When a fighter is created without a custom attribute or rerolled, _createFighterBase is called and numElements mapping is used to determine the element of an NFT that will be minted for fighter that have no customAttribute :

// _createNewFighter : 
        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }

// reroll() : 
function reRoll(uint8 tokenId, uint8 fighterType) public {
        .....
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            .....
        }
    } 

//  _createFighterBase() : 
function _createFighterBase(uint256 dna,  uint8 fighterType ) private view  returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

However numElements mapping is never adjusted or incremented, rendering any call to numElements[i] where i > 0 returning 0 , which is a problem if a generation is incremented using incrementGeneration():

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

Because as soon as generation[fighterType] is incremented, numElements[i] where i > 0 returns 0 and dna % numElements[generation[fighterType]] revert rendering the creation of new fighter when a generation of a fighterType is bigger than 1 not possible

And looking at fighterFarm.t.sol where generations are incremented it is a intended case to increment a fighter generation.

Proof of Concept

Add these tests to fighterFarm.t.sol and you will see they revert with FAIL. Reason: Division or modulo by 0:

   function testRevertWhenGenerationIncremented1() public {
        // 1. It works
        testClaimFighters();
        _fighterFarmContract.incrementGeneration(0);
        _fighterFarmContract.incrementGeneration(1);
        // 2. It doesn't work anymore
        testClaimFighters();
    }
    function testRevertWhenGenerationIncremented2() public  {
        // 1. It works
        testRedeemMintPass();
        _fighterFarmContract.incrementGeneration(0);
        _fighterFarmContract.incrementGeneration(1);
        // 2. It doesn't work anymore
        testRedeemMintPass();
    }

Tools Used

VSCode

Recommended Mitigation Steps

Set a value for numElements when a generation is incremented

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The Neuron contract's omission of the DEFAULT_ADMIN_ROLE setup, as recommended by OpenZeppelin's Access Control best practices, limits the contract's access control flexibility. This oversight restricts the ability to dynamically manage role-based permissions, complicating future updates or adjustments to role assignments

Proof of Concept

In the Neuron contract, roles such as MINTER_ROLE, SPENDER_ROLE, and STAKER_ROLE are utilized without establishing a DEFAULT_ADMIN_ROLE that has overarching control over these roles.

Usually, the DEFAULT_ADMIN_ROLE is set to the contract deployer or an initial admin account during contract initialization to allow comprehensive role management:

_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);

The absence of this setup step means the contract lacks a clear administrative hierarchy, potentially leading to difficulties in role management and permission adjustments.

Tools Used

VSCode

Recommended Mitigation Steps

set up DEFAULT_ADMIN_ROLE in Neuron.sol constructor

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The minting process in the described contract relies on a pseudo-random DNA generation mechanism that can be exploited by users, particularly when minting NFT fighters. Due to the fact that _safeMint() is used , a malicious user could use a contract to try to create a fighter using points from MergingPool or a signature, then sees which DNA the fighter he minted has and make the transaction revert if the DNA is not good enough for him/her by making IERC721Receiver(to).onERC721Received revert when _safeMint() is called on openzeppelin ERC721 contract.

Since the DNA, which significantly influences the NFT's rarity and value, is generated based on predictable parameters (msg.sender, fighters.length), users can compute them off-chain or intentionally revert transactions if the resulting NFT does not meet their desired rarity criteria. This vulnerability allows for an unfair advantage, where a user can manipulate the system to generate NFTs with potentially higher value without penalty, undermining the fairness and economic balance of the game.

Proof of Concept

The DNA for new fighters is generated on claimFighters() and mintFromMergingPool using:

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

This process, combined with the fact that transactions can be reverted by a contract caller via the _checkOnERC721Received function if not satisfied with the minted NFT's attributes, provides a loophole for gaming the system.

Malicious users could simply wait other users minting NFTs and retries to have better DNA for an NFT that will represent more value

Tools Used

VSCode

Recommended Mitigation Steps

I don't really know how to mitigate as anyone could refuse an NFT, maybe prevent a contract from using your protocol if it's not 100% needed

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The reclaimNRN function's current logic in StakeAtRisk contract poses a risk of transaction reversion due to decrement operations on a potentially zero-valued variable. This flaw can block the update process for battle outcomes, preventing a user from a win in a battle => leading to a loss of points increment which represents funds so the impact is a loss of funds

Proof of Concept

Here is the reclaimNRN function which is called when a winner wins a battle :

    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;
            // @audit if fighter just transferred it revert
            amountLost[fighterOwner] -= nrnToReclaim;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

The process start with GameServer address that calls RankedBattle.sol which then calls StakeAtRisk.sol#ReclaimNRN() in case curStakeAtRisk > 0 in RankedBattle.sol#addResultPoints():

/// RankedBattle.sol#addResultPoints() : 
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

However having curStakeAtRisk > 0 doesn't indicate that amountLost[fighterOwner] > 0 , in case of a fighter has losen, unstake and transfer it's fighter NFT there is a possibility that :

            stakeAtRisk[roundId][fighterId] > 0;
            totalStakeAtRisk[roundId] > 0;
            amountLost[fighterOwner] == 0;

Then if a new owner wins a battle within the same round it will revert. And it will happen most of the time a new owner buy a new fighter ID because in a game when you buy a new toy (here a fighter NFT), you wants to try it immediately

Details :

Add this test to rankedBattle.t.sol

function testWinnerRevert() public {
        address Bob = vm.addr(3);
        address Alice = vm.addr(4);
        // 1. Bob wins an NFT
        _mintFromMergingPool(Bob);
        _fundUserWith4kNeuronByTreasury(Bob);
        _fundUserWith4kNeuronByTreasury(Alice);
        vm.startPrank(Bob);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        vm.stopPrank();

        // 2. Bob loses 
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, false);
        uint256 amountStakedLeft = 3_000 * 10 ** 18- - 3_000 * 10 ** 15;

        // 3. Bob is angry against the game => unstake and sell token to Alice
        vm.startPrank(Bob);
        _rankedBattleContract.unstakeNRN(amountStakedLeft,0);
        _fighterFarmContract.safeTransferFrom(Bob,Alice,0);
        vm.stopPrank();

        // 4. Alice win a battle but call revert due to underflow
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);
    }

Tools Used

VSCode

Recommended Mitigation Steps

Only decrement the mappings if amountLost[fighterOwner] is > 0

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

AI Arena protocol implements each time a function to give special privileges like in MergingPool.sol where the admin role can be set to true or false in an easy way by an owner :

    function adjustAdminAccess(address adminAddress, bool access) external {
        require(msg.sender == _ownerAddress);
        isAdmin[adminAddress] = access;
    }   

However in GameItems.sol there is a function lack to revoke burning address privileges which poses a security risk. Because when an address is allowed to be a burningAddress , it is an indefinitely access to burn game items without any way to revoke it, if the burning address is no longer trusted there is no way to revoke this right.

Proof of Concept

The contract includes setAllowedBurningAddresses to grant burning privileges but lacks a corresponding mechanism to revoke these privileges, leading to a permanent, irrevocable trust in designated addresses :

    function setAllowedBurningAddresses(address newBurningAddress) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = true;
    }

Tools Used

VSCode

Recommended Mitigation Steps

Add a parameter to setAllowedBurningAddresses to allow admin to set or unset burning privileges, similar to adjustAdminAccess()

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

When created, GameItem tokens have a dailyAllowance which represent how many tokens can be minted to a single user each day , however this daily allowance can be easily bypass by transferring these tokens to another wallet and then incrementing the number of tokens that can be mint each days and bypassing dailyAllowance which is a core metric of the protocol

Proof of Concept

function transferrableResetAllowance() public {
        address Alice = vm.addr(4);
        address Alice2 = vm.addr(3);
        // 1 Alice mints 10 Voltages
        _fundUserWith4kNeuronByTreasury(Alice);
        vm.startPrank(Alice);
        _gameItemsContract.adjustTransferability(0, true);
        _gameItemsContract.mint(0, 10);
            // Alice has no more allowance then cannot mint Voltage anymore
        assertEq(_gameItemsContract.getAllowanceRemaining(Alice, 0), 0);
        vm.expectRevert();
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.safeTransferFrom(Alice,Alice2,0,10,"");
        // 2. Alice transfer tokens and voltage to it's other address in order to mint more
        _neuronContract.transferFrom(Alice,Alice2, 3_000 * 10 ** 18);
        vm.stopPrank();

        vm.startPrank(Alice2);
        // 3. Alice2 mints 10 Voltage and get 20 Voltages at all, bypassing the limit
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.getAllowanceRemaining(Alice2, 0), 0);
        assertEq(_gameItemsContract.balanceOf(Alice2, 0), 20);
        vm.stopPrank();
        // 4. Alice2 can then retransfer these tokens to Alice and bypass the daily allowance
    }

Tools Used

VSCode

Recommended Mitigation Steps

Refactor the daily allowance threshold to only allow a user to have a maximum balance of a particular game item could be a solution

Assessed type

Context