AI Arena - devblixt's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L546 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Impact

Staked fighers generated by FighterFarm can be transferred, even though it is not supposed to be transferrable while being staked.

Proof of Concept

The FighterFarm contract tracks the status of token IDs, ie, fighters being staked and disallows their transfer while being staked through the FighterFarm#_ableToTransfer hook, which is inserted in both transferFrom and safeTransferFrom functions. However, ERC721 contains an overloaded safeTransferFrom function with a different function signature (function safeTransferFrom(address from,address to,uint256 tokenId,bytes memory data)). Any user can transfer their staked fighter token by calling this overloaded function.

This is demonstrated by the following test PoC which can be added to the FighterFarm.t.sol test contract and can be executed in foundry :

        function testStakingFighterExploit() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        vm.prank(_ownerAddress);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, address(1), 0, bytes(""));
        //if this works, fighter would be still marked as being staked
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        //checking owner of tokenID 0 which got transferred
        assertEq(_fighterFarmContract.ownerOf(0),address(1));
    }

The function would revert if the tokenID was not transferred. However, we are successful in transferring it.

Tools Used

Manual Review

Recommended Mitigation Steps

Add the _ableToTransfer hook to all transfer functions

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L301 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

Impact

Non-transferrable game items can be transferred by using the safeBatchTransfer function in GameItems contract.

Proof of Concept

The GameItems contract facilitates the buying of GameItems within AIArena and places certain restrictions depending on the type of items- it could be transferrability, allowance per day, etc. It is an ERC1155-inherited contract which overrides the safeTransferFrom function and inserts a hook to check if the current tokenId (Game Item type) is transferrable or not. If it is not, the function call reverts. However, this can be easily bypassed by calling the ERC1155 function safeBatchTransferFrom, which is inherited by the contract, but not overrided to include the appropriate hook.

This is showcased by the following code PoC which can be added to GameItems.t.sol :

    function testExploitNonTransferrable() public {
        _gameItemsContract.createGameItem("something", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(1, 10);
        uint256[] memory a = new uint256[](1);
        uint256[] memory b = new uint256[](1);
        a[0] = 1;
        b[0] =10;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, address(1), a, b, bytes(""));
    }

This function call does not revert and successfully conducts the transfer. Thus, a potential exploit.

Tools Used

Manual Review.

Recommended Mitigation Steps

Override all transfer functions and add the appropriate hook.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The AAMintPass does not differentiate between AI Champion and Dendroid mintpasses and results in enabling users to mint any fighter type they want with a mint pass. FighterFarm#redeemMintPass cannot verify what kind of fighter type is associated with a mint pass.

Proof of Concept

The AAMintPass#claimMintPass function takes in a uint8[2] parameter numToMint which contains the number of AI Champion fighters to mint and the number of Dendroid mintpasses to mint in its 0th and 1st indices respectively. Now, after the function verifies that the signature provided by the user is indeed signed by the delegated address, it sums the total number of mint passes to be generated here. The contract then mints the total number of mint passes to the appropriate EOA account.

However, there is no distinction between the two fighter types while minting the pass. This means that any contract which accepts this pass will not be able to validate whether a pass is for an AI Champion fighter or a dendroid. In fact, we can see how FighterFarm#redeemMintPass takes in parameters from the user here. The redeemMintPass function takes in a uint8[] parameter fighterTypes which contains the fighter type to be associated to each mint pass. This means that even if a user has been allocated only AI Champion mint passes, they can mint dendroid fighters.

The redeemMintPass allows the user to mint any type of fighter they want, and this is facilitated by the mint pass not carrying any sort of identification as to which kind of fighter type it is meant for. Since dendroids are supposed to be a more rarer variety, this can result in unintended consequences, such as the dendroids' rarity being diluted by malicious users.

Tools Used

Manual Review

Recommended Mitigation Steps

There are many ways to mitigate this. The easiest one would be this :

1. Keep a mapping(uint256 tokenId => bool fighterType) inside the contract which would store the fighter type associated to each tokenId.
2. Create a view function in AAMintPass which lets you view what kind of fighter type each mint pass can get the user.
3. In the FighterFarm contract, determine the fighter type by calling the view function in the AAMintPass contract.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Lack of functions for updation of FighterFarm#numElements variable will make FighterFarm contract unusable in later generations of fighters.

Proof of Concept

The number of elements for each generation is stored in a mapping(uint8 => uint8) numElements variable in the FighterFarm contract. The number of elements for the 0th generation is set to 3 in the contract's constructor. This is then later used in the FighterFarm#_createFighterBase function for calculating the element of the fighter.

However, on later generations, this will revert and the FighterFarm contract will be rendered unusable. This is because the contract does not have a function to update the number of elements for later generations. In other words, after incrementing a fighter's generation through the FighterFarm#incrementGeneration function, this operation in _createFighterBase will revert:

        uint256 element = dna % numElements[generation[fighterType]];

This is because numElements[generation[fighterType]] will return 0 when generation > 0.

If needed, this coded PoC can be added to FighterFarm.t.sol to confirm the issue :

    function testNumGenError() public {
        _mintFromMergingPool(_ownerAddress);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _fighterFarmContract.incrementGeneration(0);
        _neuronContract.addSpender(address(_fighterFarmContract));
        vm.expectRevert();
        _fighterFarmContract.reRoll(0, 0);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

1. Add a function for setting numElements.
2. While incrementing generation, check if number of elements for that generation has been already set. If it is not, revert.

Assessed type

Other