AI Arena - Josh4324's results

Lines of code

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

Vulnerability details

Impact

The fighter nft is transferable by default but it cannot be transferred when a user has staked it, this staking occurs when a user create a stake in order to play a game. The transferFrom and saferFrom has the _ableToTransfer(uint256 tokenId, address to), this functions checks three conditions before it can transfer the fighter to another user, the conditions are owner of the nft, maximum number of fighter nft held by the user and if the fighter is currently staked.

The problem arise from the fact that the function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) was not taken into consideration, the FighterFarm.sol inherits the ERC721 openzepplin contract, the mentioned safeTransferFrom is different from the function safeTransferFrom( address from, address to,uint256 tokenId), the first has 4 parameters while the second has 3 parameters. The safeFrom with 3 parameters and the transferFrom has the _ableToTransfer(uint256 tokenId, address to) check but the safeTransferFrom with 4 parameters do not have this check which allows anyone one to transfer their fighter nft to others even when the fighter nft is staked.

Proof of Concept

 function testTransferringFighter1() public {
        // create two users
        address user1 = vm.addr(5);
        address user2 = vm.addr(6);
        
         // fund user1 with neuron tokens
        _mintFromMergingPool(user1);

        // Add staker role to user1
        _fighterFarmContract.addStaker(user1);

        // update user1 fighter staking status
        vm.prank(user1);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
      
        // Send fighter nft to user2
        vm.prank(user1);
        _fighterFarmContract.safeTransferFrom(user1, user2, 0, "");
        
        // Check if user2 has the fighter nft
        assertEq(_fighterFarmContract.ownerOf(0) == user2, true);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add the _ableToTransfer(uint256 tokenId, address to) to safeTransferFrom(address from,address to, uint256 tokenId, bytes memory data) Reviewing all safeTransferFrom paths to ensure consistency

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When creating a game item, an admin can set the transferability of the game item to true or false, game items that are set to true can be transfered from one user to another and game items set to false cannot be transfered. According to the design this should be fine as the safeTransferFrom has a check to know if the game item is transferable or not.

require(allGameItemAttributes[tokenId].transferable)

The problem arise from the fact that ERC1155 also has a safeBatchTransferFrom which the GameItems.sol contract inherited and it was not overwritten to have a require statement to check if the game is transferable.

The main impacts of this issue are:

Users can freely transfer game items meant to be locked Breaches game design intentions and incentives Harms fairness and integrity of the game economy

Proof of Concept

  function testTransferNonTransferableGameItems() public {
        // create two users
        address user1 = vm.addr(5);
        address user2 = vm.addr(6);

        // fund user1 with neuron tokens
        _fundUserWith4kNeuronByTreasury(user1);
         
        // create a new game item and set the transferability to false
        _gameItemsContract.createGameItem(
            "Gloves", "https://ipfs.io/ipfs/gloves", false, false, 10_000, 1 * 10 ** 18, 10
        );
        
        //Mint the new game item to user1
        vm.prank(user1);
        _gameItemsContract.mint(1, 1);
        
        // create a new array with one item, the item is an array containing the token id, it will also be used as an array containing the number of items.
        uint256[] memory arr1 = new uint256[](1);
        arr1[0] = 1;
        
        // User 1 sends the game item to users
        vm.prank(user1);
        _gameItemsContract.safeBatchTransferFrom(user1, user2, arr1, arr1, "");
        
        // checks that user2 has the game item sent by user1
        assertEq(_gameItemsContract.balanceOf(user2, 1), 1);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

To address this, I recommend:

Adding the require(allGameItemAttributes[tokenId].transferable) to safeBatchTransferFrom() Reviewing all safeTransferFrom paths to ensure consistency Adding unit tests for non-transferable item restrictions

Fixing this oversight will properly enforce game item transferability rules as intended.

Assessed type

Other