AI Arena - 0xbranded's results

Lines of code

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

Vulnerability details

Impact

Users can use the safeBatchTransferFrom function to bypass transfer restrictions and transfer gameItems which are marked as non-transferable, effectively nullifying the transferability feature. Given that this functionality is expected to block transfers, it can lead to unintended behavior, such as abuse of gameplay mechanics or third-party markets being created for items that shouldn't be transferable.

Proof of Concept

function testTransferabilityViolation() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);

        // Transferability set to false   
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

        // Transfer succeeds despite transferable set to false        
        uint[] memory ids = new uint[](1); 
        ids[0] = 0;
        uint[] memory amounts = new uint[](1); 
        amounts[0] = 1;       
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");

        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Include a similar override of the safeBatchTransferFrom function, in addition to safeTransferFrom.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

In the case of claimFighters and redeemMintPass, an internal call is made to _createFighterBase to generate the element, weight, dna. However, the element calculation is dna % numElements[generation[fighterType]]. The numElements mapping has only the first entry set, with no way to update future generations. Therefore for any generation after 0, this calculation will be dna % 0, which will always revert.

As a result, any users who may have spent resources to gain a mint pass or claim will not be able to redeem for an nft after gen 0. These nfts are systematically compromised due to this error.

Proof of Concept

In both tests below, the existing testClaimFighters and testRedeemMintpass were copied (which had working mints) with the only modification being incrementing the generation of the fighter right before minting.

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

        assertEq(_mintPassContract.mintingPaused(), false);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
        assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

        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] = "dna";
        _fighterTypes[0] = 0;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

        // Increment to gen 1
        _fighterFarmContract.incrementGeneration(0);
        assertEq(_fighterFarmContract.generation(0), 1);

        // Mint pass redemption will now be broken
        vm.expectRevert();
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
        );

        // Fighter and mint NFT balance of owner did not change
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 0);
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
    }

    function testClaimBroken() public {
        // setup
        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](1);
        claimModelTypes[0] = "original";

        // Increment to gen 1
        _fighterFarmContract.incrementGeneration(0);
        assertEq(_fighterFarmContract.generation(0), 1);

        // Right sender of signature should be able to claim fighter
        // but this will now revert for gen >0
        vm.expectRevert();
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);

        // Balances unchanged
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

In incrementGeneration, add the line numElements[generation[fighterType]] += 1 after the line generation[fighterType] += 1. This will ensure there is always some value and you are never dividing by zero.

Additionally, include some way to update numElements, in case this value should change in a future generation.

Assessed type

Math