Non-transferable Game Items can be transferred using `safeBatchTransferFrom()`

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

Non-transferable Game Items can be transferred. This breaks the main invariant of soulbound tokens.

It incurs the following impacts:

• It breaks the expected flow of the game, by allowing for example, to transfer batteries in times battery transfers are disabled, or allowing to transfer special soulbound weapons. This gives an unfair advantage to players exploiting this.
• It enables a black market for soulbound tokens which should not exist

Reference from the public Discord channel:

Q: Why do you prevent certain GameItems from transfer, by using transferable?

A: If we wanted to issue a soulbound weapon we want to leave the option available and we also might not want the ability to transfer batteries in a certain period of time.

Note: I'm separating this issue from the safeTransferFrom issue on FighterFarm as the functions, contracts, and impacts are different and completely unrelated.

Vulnerability Details

There are certain Game Items that should not be transferrable.

This can be done on creation for items that should never be transferable, or it can be changed for a certain period to temporary disallow transfers.

This check is enforced on the overriden function safeTransferFrom():

    require(allGameItemAttributes[tokenId].transferable);

GameItems.sol#L301

The problem is that it is still possible to transfer tokens via the inherited ERC1155 safeBatchTransferFrom() function.

Proof of Concept

• Add the test to test/GameItems.t.sol
• Run forge test --mt "testTransferNonTransferableTokens"

function testTransferNonTransferableTokens() public {
    // Create a non-transferable game item
    bool transferable = false;
    _gameItemsContract.createGameItem(
        "Gloves", "https://ipfs.io/ipfs/gloves", false, transferable, 10_000, 1 * 10 ** 18, 10
    );
    
    // Fund user with tokens (using `_ownerAddress` like the rest of the codebase)
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(1, 1);

    // Using `safeTransferFrom()` the token works as expected (not transferred)
    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;

    // But using `safeBatchTransferFrom()` the "non-transferable" token can be transferred!
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 0);
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 1), 1);
}

Recommended Mitigation Steps

Override the GameItems::safeBatchTransferFrom() function, adding the require statement:

+    function safeBatchTransferFrom(
+        address _from,
+        address _to,
+        uint256[] calldata _ids,
+        uint256[] calldata _values,
+        bytes calldata _data
+    )
+        external 
+        override(ERC1155)
+    {
+        require(allGameItemAttributes[tokenId].transferable);
+        super.safeBatchTransferFrom(_from, _to, _ids, _values, _data);
+    }

Assessed type

Token-Transfer

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #18

[c4-pre-sort]

raymondfam marked the issue as duplicate of #575

[c4-judge]

HickupHH3 marked the issue as satisfactory