AI Arena - matejdb's results

Lines of code

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

Vulnerability details

Impact

Core protocol functionality is broken because a user can still transfer around his staked Fighter NFTs thus bypassing _ableToTransfer() in FighterFarm.sol.

The issue is that FighterFarm.sol does not override the ERC721 safeTransferFrom(address, address, uint256, bytes memory).

Proof of Concept

Paste this into FighterFarm.t.sol. We can see that a staked NFT has been successfully transferred.

    function testSafeTransferFrom() public {
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // Add owner address as staker
        _fighterFarmContract.addStaker(_ownerAddress);

        // Stake the fighter nft with token id 0
        _fighterFarmContract.updateFighterStaking(0, true);

        // Ensure fighter is staked
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        // Expect transfer to revert
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);

        // Expect this transfer to pass
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "0x0");
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Implement a function in FighterFarm.sol that looks like the following:

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        bytes memory data
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _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#L289-L303

Vulnerability details

Impact

Items that are set as untransferable by setting their corresponding id to false by calling adjustTransferability(id, false) on GameItems.sol can still be transferred therefore breaking core protocol functionality.

Proof of Concept

Paste this in GameItems.t.sol

    function testSafeTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.adjustTransferability(0, false);

        // Expect this to fail since the game item is not transferable.
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // This should work
        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256 [] memory amounts = new uint256[](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

Implement safeBatchTransferFrom on GameItems.sol as such:

    function safeBatchTransferFrom(
        address from, 
        address to, 
        uint256[] memory tokenIds,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeBatchTransferFrom(from, to, tokenIds, amounts, data);
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Details

When calling redeemMintPass users can burn their AAMintPass NFT in order to mint their Fighter NFT.

In doing so they specify themselves all the features the NFT will have. This is a vulnerability since DNA should be a pseudorandom feature. In this case it is NOT since the user explicitly specifies it.

There is no pseudorandom feature in this line: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L254

as can be found in other _createNewFighter calls: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L214

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

Here pseudorandonimity is provided by the length of the fighters array.

Not only that but since all function arguments are provided by users they can manipulate their wanted attributes in on-chain calculations: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L499C1-L506C10

This way they can calculate off-chain the best outcome of these calculations and send those arguments on-chain.

Impact

Users who redeem their mint pass gain unfair advantage in the protocols' battle gameplay. They can easily manipulate their stakers' on-chain feature by passing arguments that benefit their attribute on-chain calculation.

Sponsor confirmed this should not be the case.

You could argue for this to be a design choice but it breaks the games' fairness and transparency - and it should be mentioned in the audit report.

Proof of Concept

1. User redeems his Mint Pass with best feature outcome arguments.
2. His fighter gains optimal on-chain feature like element and weight.
3. User is better fitted in battles but not fairly.

Tools Used

Manual review

Recommended Mitigation Steps

Write the features of Fighter NFT on AAMintPass NFTs and then fetch them on-chain during minting in FighterFarm instead of letting the user send them in redeemMintPass.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

User can change his Fighter attributes by calling FighterFarm::reRoll with the fighterType argument different than his type.

This allows him to increase his chances in off-chain battling because the system fetches on-chain attributes of a fighter id when battling as can be interpreted from docs:

Weight: Determines the battle attributes

and is confirmed by sponsor.

This allows a user with a certain Fighter NFT to acquire those attributes of a different - more favorable fighterType.

This lack of check of fighter type also allows him to re-roll his fighter more times than allowed for his Fighter.

Attributes affected:

fighters[tokenId].element = element;
fighters[tokenId].weight = weight;

Proof of Concept

This test will pass - paste it into FighterFarm.t.sol. This directly affects element, weight, newDna and physicalAttributes variables which are then stored on-chain or used in other calculations that are stored Fighter struct.

    function testRerollingToChangeFighterType() public {

        // This mints a fighterType = 0
        _mintFromMergingPool(_ownerAddress); 

        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
            uint8 tokenId = 0;

            // Here we change the fighter type to 1 and then try to reroll it
            uint8 fighterType = 1;

            _neuronContract.addSpender(address(_fighterFarmContract));
            _fighterFarmContract.reRoll(tokenId, fighterType);

            assertEq(_fighterFarmContract.numRerolls(0), 1);
            assertEq(neuronBalanceBeforeReRoll > _neuronContract.balanceOf(_ownerAddress), true);
        }
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Store fighterType into FighterOps::Fighter struct and check user is passing correct argument in reRoll function.

Assessed type

Invalid Validation