AI Arena - fnanni's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L333-L365

Vulnerability details

Impact

Fighter holders can bypass the transfer restrictions by calling safeTransferFrom(from, to, tokenId, data), which is not overriden in FighterFarm.sol. Since it's not overriden, it is not checked if the fighter NFT has been staked and if the receiver will surpass the MAX_FIGHTERS_ALLOWED balance.

Proof of Concept

FighterFarm.sol overrides the ERC721 transfer functions in order to add the checks at _ableToTransfer(). This is done for transferFrom(from, to, tokenId) and safeTransferFrom(from, to, tokenId) but not for safeTransferFrom(from, to, tokenId, data).

Tools Used

Manual Review

Recommended Mitigation Steps

Add the following to the FighterFarm contract:

/// @notice Safely transfers an NFT from one address to another.
/// @dev Add a custom check for an ability to transfer the fighter.
/// @param from Address of the current owner.
/// @param to Address of the new owner.
/// @param tokenId ID of the fighter being transferred.
function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    bytes memory data
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _safeTransfer(from, to, tokenId, "");
}

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

Game items that are set to non-transferable can be transferred.

Proof of Concept

GameItems.sol inherits from ERC1155 which contains two transfer functions: safeBatchTransferFrom and safeTransferFrom.

The contract overrides safeTransferFrom in order to check if the item is transferable or not: require(allGameItemAttributes[tokenId].transferable);. However, safeBatchTransferFrom is not overriden and can still be called to transfer tokens without this restriction.

Place the following test inside GameItems.t.sol to see the bug.

    /// @notice Test transferring non-transferable items.
    function testTransferability() public {
        _gameItemsContract.adjustTransferability(0, false);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        // Doesn't revert, ignoring the transferability check
        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

Override safeBatchTransferFrom adding

require(allGameItemAttributes[tokenId].transferable);

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L261 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L484-L515 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107-L109

Vulnerability details

Impact

Users redeeming their mint passes for fighters can brute force mintPassDnas values to get particular weights, elements and attributes even if rare, taking an unfair advantage and breaking the rarity model given by the attributes probabilities.

Proof of Concept

redeemMintPass takes mintPassDnas as input without imposing any restrictions nor input value validation. Then the function uses these values to get the DNA of the new fighter given by keccak256(abi.encode(mintPassDnas[i])). This is a deterministic value that can be known in advance. Even though it's not possible to determine which mintPassDna will output a certain DNA, a simple script can be implemented to get DNAs of certain characteristics through trial and error. Here is a python pseudo code for that:

while true:
    randomMintPassDna = getRandomText()
    dna = Web3.keccak(text=randomMintPassDna)
    if dnaHasWantedFeatures(dna):
        break

To predict the weight and element, the following formulas used in _createFighterBase have to be checked:

    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;

To predict the attributes of the fighter, the formula used in createPhysicalAttributes and dnaToIndex have to be checked:

    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
    finalAttributeProbabilityIndexes[i] = attributeIndex;

Additionally, an existing fighter's dna could be replicated by simply using a mintPassDna string which bytes match the source of randomness of the fighter being replicated. In the case of fighters minted from the MergingPool or from a delegated signature, the string would have to match abi.encode(address, fighters.length). In the case of replicating a fighter minted using a mint pass, just use the same string and that's it.

Tools Used

Manual Review

Recommended Mitigation Steps

Add stronger randomness into the DNA selection and require DNAs to be unique.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Not checking the fighterType in reRoll has the following consequences:

1. maxRerollsAllowed can be exceeded if opposite fighter type has assigned a greater value of maxRerollsAllowed.
2. If the correct fighter type is a champion, inputting a dendroid fighter type would make the DNA uint256(1). This could break the rarity model, especially if the smallest (more rare) attribute probabilities are placed at index 0 of the attributeProbabilities array.
3. It is possible to reroll a champion using the attributeProbabilities and elements of older champion generations, if generation is smaller for dendroids than for champions.

Proof of Concept

The consequences mentioned above can happen because nowhere along the reRoll() function it is checked whether the fighterType provided as input is correct. A corrupted fighter type will have an effect in the following parts of the code:

1. require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
2. the element definition when _createFighterBase is called.
3. the dna definition when _createFighterBase is called.
4. the createPhysicalAttributes is called using the potentially corrupted DNA and incorrect fighter type generation. This values are used in AiArenaHelper.sol the calculate the new physical attributes of non-dendroids.

Add the following test to FighterFarm.t.sol to check how a champion DNA can be manipulated to uint256(1) using a corrupted dendroid type. Note that there's an additional bug preventing the creation of new fighters when the generation > 0, which makes it difficult to test the other ramifications of this issue. Said generation bug is out of scope of this submission.

    /// @notice Test rerolling a fighter with an incorrect fighter type
    function testRerollCorrupted() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model
        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
            uint8 tokenId = 0;
            uint8 corruptedFighterType = 1;

            (
                uint256 weight,
                uint256 element,
                FighterOps.FighterPhysicalAttributes memory physicalAttributes,
                uint256 id,,,
                uint8 generation,,
                bool dendroidBool
            ) = _fighterFarmContract.fighters(tokenId);
            assertEq(dendroidBool, false);

            _neuronContract.addSpender(address(_fighterFarmContract));
            // User dendroid fighter type for a token id corresponding to a champion
            _fighterFarmContract.reRoll(tokenId, corruptedFighterType);

            (
                weight,
                element,
                physicalAttributes,
                id,,,
                generation,,
                dendroidBool
            ) = _fighterFarmContract.fighters(tokenId);

            // DNA = uint256(1) results in attributeProbabilityIndex of 1 for all its attributes
            assertEq(physicalAttributes.head, 1);
            assertEq(physicalAttributes.eyes, 1);
            assertEq(physicalAttributes.mouth, 1);
            assertEq(physicalAttributes.body, 1);
            assertEq(physicalAttributes.hands, 1);
            assertEq(physicalAttributes.feet, 1);

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

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of letting the caller of reRoll() provide the fighterType, read it from fighter.dendroidBool.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L244-L265 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L322-L349 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L416-L500

Vulnerability details

Impact

Players can stake amounts of NRN as small as 1 wei and they will still get the minimum staking factor of 1, risking a minimum amount of stake at any battle.

Additionally, if less than $\frac{9999}{bpsLostPerLoss}$ is staked, it's possible to battle without ever getting stake at risk after losing a battle. In other words, it's possible to battle for points only.

Proof of Concept

When a battle result is updated, the amount of stake that will become "at risk" can be zero even if the amount staked is greater than zero. This happens for stake amounts in the range [0 ; $\frac{9999}{bpsLostPerLoss}$] because curStakeAtRisk will round down to zero after dividing by $10000$.

In this scenario, the fighter will earn points as usually after winning a battle, but won't lose stake after losing a battle with zero points accumulated because curStakeAtRisk is zero.

What this means is that fighters can participate for points only, without risking stake and without entering the "at risk" zone in which at least one win is needed before the fighter can battle for points again.

The opposite scenario, though harmless, is possible as well: a fighter having at some point $stakeAtRisk < \frac{9999}{bpsLostPerLoss} - 1$ could accidentally unstake everything except for 1 wei. This will cause the rounding of curStakeAtRisk towards zero, but in this case battles will have zero effect on this fighter during the current round because $stakeAtRisk > 0$.

Tools Used

Manual Review

Recommended Mitigation Steps

Require stakes to be greater or equal than 10^18.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The rerolling feature is unavailable for fighters with token ids greater than 255, potentially making them less valuable.

Proof of Concept

The data type of the tokenId input parameter of reRoll is uint8, which means that only values in the range [0-255] are allowed. Providing greater token ids will revert with a value out-of-bounds error.

Tools Used

Manual Review

Recommended Mitigation Steps

Change

function reRoll(uint8 tokenId, uint8 fighterType) public {

to

function reRoll(uint256 tokenId, uint8 fighterType) public {

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

Once incrementGeneration is called for the first time for a fighter type, new fighters of this type cannot be created. In other words, fighters of generation > 0 can't be created.

Proof of Concept

numElements stores the number of elements for every generation. The number of elements for generation zero is hardcoded in the constructor, but it's not updated when the generation is incremented in incrementGeneration() and there's no way to update it otherwise.

When a new fighter with generation > 0 is created, _createFighterBase is called, which performs dna % numElements[generation[fighterType]]. Here $numElements[generation[fighterType]]=0$, causing a revert when trying to perform a modulo operation by 0. Therefore, the following test added in FighterFarm.t.sol will result in the error message "FAIL. Reason: Division or modulo by 0":

    /// @notice Test whether the correct sender of the signature can claim a fighter.
    function testClaimFightersGeneration1() public {
        _fighterFarmContract.incrementGeneration(0);
        _fighterFarmContract.incrementGeneration(1);

        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";

        // Right sender of signature should be able to claim fighter
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.totalSupply(), 1);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Update numElements in incrementGeneration.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L20

Vulnerability details

Impact

Choosing an attribute divisor array that doesn't follow a geometric progression of the form 1, 100, 10000, 1000000, etc. will break the rarity system. In other words, the attributeProbabilities won't work as expected and rarity of attributes will be corrupted, even making certain combinations of attributes unattainable.

In particular, it is concerning that the defaultAttributeDivisor introduces this behavior.

Another consequence is that users could get deceived into thinking they got a rare fighter when it's actually very common and viceversa, misestimating the value of their fighters and potentially trading them for under/overestimated prices.

Proof of Concept

The patterns that we get from

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

are repeated every time $dna\ mod (100 * lcm(attributeToDnaDivisor)) = 0$. What does this mean? Let's look for example at defaultAttributeDivisor. For $divisor=2$, the rarityRank goes through every possible value every 200 increments. In other words, $\frac{dna}{2}\ mod\ 100 = 0$ for all $dna = 200*k$.

We equivalently can say the same for every other divisors 3, 5, 7, 11, 13. Using divisor 3 repeats the pattern every 300 increments, 5 every 500, etc. All of these individual patterns combined reset every $100 * lcm(2, 3, 5, 7, 11, 13)) = 3003000$. It's easy to see that all possible combinations of rarity ranks is actually $100^6 = 1\ trillion$, which is MUCH more than $3003000$. Additionally, not every $3003000$ output is unique. For instance $dna=0$ and $dna=1$ result in the same rarity rank pattern, which means that the amount of combinations is actually a lot less than $3003000$.

As a simplified exercise, let there be 2 attributes A and B with divisors 2 and 3, and let's calculate rarityRank using %10 instead of %100. Can you find a dna value for which $rarityRank(A)=9$ and $rarityRank(B)=0$?

Tools Used

Manual review

Recommended Mitigation Steps

Change AiArenaHelper.sol L107 from

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

to

uint256 rarityRank = (dna / 100 ** i) % 100;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L163 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L313-L331 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L502-L506

Vulnerability details

Impact

NFT winners claiming a fighter from the MergingPool can choose any value for the fighter's weight and element.

Proof of Concept

customAttributes is provided as input in claimRewards and then passed without any validation to the FighterFarm contract for minting

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

The FighterFarm contract takes these values as they are and creates a fighter with them only except for the case in which the element value equals 100. Note that in this latter case, element is in the range [0; numElements[..]] and weight is in the range [65; 96].

if (customAttributes[0] == 100) {
    element = dna % numElements[generation[fighterType]];
    weight = dna % 31 + 65;
    newDna = fighterType == 0 ? dna : uint256(fighterType);
} else {
    element = customAttributes[0];
    weight = customAttributes[1];
    newDna = dna;
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add input validation to the weight and element values provided inside customAttributes when calling claimRewards.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A user might not be able to claim a fighter won on the MergingPool due to the MAX_FIGHTERS_ALLOWED limit even when he hasn't reached the limit.

Proof of Concept

NFTs in the MergingPool are claimed on batch. If a user has won more than one fighter and has not yet claimed any of them, it will only be possible for him to claim all of them at once.

The issue is that the minting of any fighter will update the balance of the beneficiary and will revert if the MAX_FIGHTERS_ALLOWED limit has been reached.

For example, a user who has 2 unclaimed fighters in the MergingPool and is currently holding 9 fighters, won't be able to claim the first fighter even if he should, because the second fighter will get him to a balance=11 and will make the claiming call revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users to claim fighters up until a given round id lower than the current one.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Fighter owners can calculate in advance what weight, element and attributes will their rerolled fighter get. They can do this for any number of rerolls up to maxRerollsAllowed[fighterType]. This makes the process transparent instead of random, because owners can simply choose and buy their fighter from the pool of rerolled fighters.

Additionally, owners can create a wallet address through brute force to get a DNA they want with specific features.

Proof of Concept

The DNA of rerolled fighters is given by

keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId]));

Here msg.sender is the owner of the fighter NFT. The first problem with this is that the DNA can be deterministically calculated for each rerolled number to understand whether it makes sense to reroll a fighter or not. More concerning yet, this knowledge can be used to manipulate the address of the fighter owner.

Even though it's not possible to determine which address will output a certain DNA, a simple script can be implemented to get DNAs of certain characteristics through trial and error. The idea is to randomly create private keys until one corresponds to a wallet address that results in the DNA we want. Once we have the private key, we can transfer the fighter and NRN to the new wallet, call reRoll() and transfer the rerolled fighter back. Here is a python pseudo code to find the wallet:

acc = web3.eth.Account()
while true:
    newWallet = acc.create(getRandomText())
    dna = Web3.keccak(newWallet, tokenId, numRerolls[tokenId])
    if dnaHasWantedFeatures(dna):
        break

To predict the weight and element, the following formulas used in _createFighterBase have to be checked:

    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;

To predict the attributes of the fighter, the formula used in createPhysicalAttributes and dnaToIndex have to be checked:

    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
    finalAttributeProbabilityIndexes[i] = attributeIndex;

Tools Used

Manual Review

Recommended Mitigation Steps

Use a DNA randomness source that cannot be manipulated.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

1 point is mistakenly added to the first element of attributeProbabilities[generation][attribute] while the last element is reduced in 1 point. This means that the intended rarity for attributes is miscalculated. In the edge case in which the last element of attributeProbabilities[generation][attribute] is 1, this index is missed completely, because it becomes unreachable.

Proof of Concept

We can visualize this issue with two simple examples.

Example 1

Let's say that for a given generation and attribute:

attributeProbabilities[generation][attribute] = [10,10,10,10,10,10,10,10,10,10]

The range of values of rarityRank that fall into the cumulative probability of the first iteration of dnaToIndex is [0 - 10]. Note that cumProb=10 >= rarityRank=10 is true. This means that there are eleven values (11%) for which rarityRank will output attributeProbabilityIndex equal to 1.

The range of values of rarityRank that fall into the cumulative probability of the second iteration of dnaToIndex is [11 - 20], i.e. 10%. This is also true for all the following indexes except for the last one, which matches with 9 rarityRank values [91 - 99] (note that rarityRank <= 99). This means that the code is interpreting

attributeProbabilities[generation][attribute] = [10,10,10,10,10,10,10,10,10,10]

as

attributeProbabilities[generation][attribute] = [11,10,10,10,10,10,10,10,10,9]

Example 2

Let's say that for a given generation and attribute:

attributeProbabilities[generation][attribute] = [99,1]

In this case, cumProb=99 >= rarityRank is always true, which means that the second probability is ignored and in practice we get the following:

attributeProbabilities[generation][attribute] = [100,0]

Discussion

It could be argued that this is a setup issue. After all, tweaking the probabilities to [9,10,10,10,10,10,10,10,10,10] and [98,1] in the previous examples and making sure interfaces and contracts potentially querying attributeProbabilities parsed the data correctly according to this hacky fix would solve the issue. However, I think this lucky, counter-intuitive fix shouldn't be a reason to ignore the error. Probability values should mean the same across the array.

Tools Used

Manual Review

Recommended Mitigation Steps

Use if (cumProb > rarityRank) instead of if (cumProb >= rarityRank).

Assessed type

Math