search

code-423n4 / 2024-02-ai-arena-findings

4 stars 3 forks source link

GameItems can always be transferred #2011

Closed c4-bot-2 closed 6 months ago

c4-bot-2 commented 7 months ago

Lines of code

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

Vulnerability details

Impact

Any game item (an ERC-1155) can be transferred even though the intention of its configuration says otherwise (the GameItemAttributes.transferable member is false).

Description

The GameItems contract lacks measures in certain transfer-related functions to enforce the non-transferability status of items, allowing transfers even when explicitly disallowed by the game attribute configuration:

Therefore, game items can always be transferred by calling the GameItems.safeBatchTransferFrom function.

/// @notice Safely transfers an NFT from one address to another.
/// @dev Added a check to see if the game item is transferable.
function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId,
    uint256 amount,
    bytes memory data
)
    public
    override(ERC1155)
{
    // @audit-issue this check is not present in `safeBatchTransferFrom`
    require(allGameItemAttributes[tokenId].transferable);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

Proof Of Concept

Add the following tests in GameItems.t.sol:

function testSafeBatchTransferFromIsAvailable() public {
    // Arrange
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
    _gameItemsContract.adjustTransferability(0, false);
    (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
    assertEq(transferable, false);

    assertEq(_gameItemsContract.balanceOf(address(this), 0), 2);
    assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 0);

    // Act - safeTransferFrom checks `transferable`
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(address(this), msg.sender, 0, 1, "");

    // Act - safeBatchTransferFrom does not check `transferable`
    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = 0;
    uint256[] memory amounts = new uint256[](1);
    amounts[0] = 1;
    _gameItemsContract.safeBatchTransferFrom(address(this), msg.sender, tokenIds, amounts, "");

    // Assert
    assertEq(_gameItemsContract.balanceOf(address(this), 0), 1);
    assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 1);
}

Tools Used

Forge tests and manually reviewed.

Recommended Mitigation Steps

Override the GameItems.safeBatchTransferFrom function making sure each token is transferable.

Assessed type

Invalid Validation

c4-pre-sort commented 7 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 7 months ago

raymondfam marked the issue as duplicate of #15

c4-judge commented 6 months ago

HickupHH3 marked the issue as not a duplicate

c4-judge commented 6 months ago

HickupHH3 marked the issue as duplicate of #575

c4-judge commented 6 months ago

HickupHH3 changed the severity to 3 (High Risk)

c4-judge commented 6 months ago

HickupHH3 marked the issue as satisfactory