Transfer Restriction Bypass in GameItems Contract via safeBatchTransferFrom Method

[c4-bot-2]

Lines of code

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

Vulnerability details

Summary:

The GameItems contract represents a collection of game items used in AI Arena. These items are created by the admin calling the createGameItem function. A key feature of these items is the admin's ability to designate whether an item is transferable, a setting initially defined upon creation and adjustable thereafter. To enforce this, the GameItems contract overrides the safeTransferFrom function in the ERC1155 standard, introducing a transferability check.

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

However, the contract fails to apply the same restrictions to the safeBatchTransferFrom function, another transfer method provided by ERC1155. This oversight allows users to bypass the intended transfer restrictions.

Impact:

This enables users to bypass the set transfer restrictions on game items by employing the safeBatchTransferFrom method. This loophole may lead to various issues or losses for the protocol, especially if the non-transferability of an item was intended to prevent specific actions or outcomes.

Proof Of Concept

function testByPassTransferLock() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);
    // use of safeTransferFrom will lead to a revert 
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);
        // use trasferBatch to bypass transfer lock
        // array of ids uint8
        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        // array of amounts uint8
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

To align all transfer methods with the protocol's requirements, add the following function to the GameItems contract. This function will override the other safeBatchTransferFrom function, incorporating checks to prevent any bypassing of transfer restrictions:

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override(ERC1155) {
    //loop thorugh all ids and check if transferrable, if not revert
    ...
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Other

[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 changed the severity to 3 (High Risk)

[c4-judge]

HickupHH3 marked the issue as satisfactory