AI Arena - Aymen0909's results

Lines of code

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

Vulnerability details

Issue Description

In the GameItems contract, the owner can modify the transferability state of a given item NFT using the adjustTransferability function. When allGameItemAttributes[tokenId].transferable is set to false, all the tokens associated with that tokenId cannot be transferred by users. This restriction is ensured by the check inside the safeTransferFrom function:

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

However, the OpenZeppelin ERC1155 token implementation (which the contract inherits from) contains another transfer function, safeBatchTransferFrom, which was not overridden in GameItems. Therefore, token holders can use it directly to transfer their tokens regardless of the transferability state set by the owner.

This loophole can pose significant problems for the protocol and the underlying game, as the owner will have no control over the transferability of game items.

Impact

Users can bypass the game item transferability check using the safeBatchTransferFrom function, allowing them to transfer game items regardless of the owner's decision. This could potentially enable users to manipulate the protocol or exploit vulnerabilities in the game logic.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, the safeBatchTransferFrom function must also be overridden and include the transferability check, similar to the safeTransferFrom function.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Issue Description

The reRoll function allows a fighter owner to randomly change (reroll) the traits of their fighter NFT. Within the FighterFarm contract, the numRerolls mapping tracks and limits the number of times a user can reroll their fighter, with the maximum rerolls allowed for each fighter type denoted by maxRerollsAllowed.

The reRoll function includes the following check to ensure the maximum rerolls are not exceeded:

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

However, when calling the reRoll function, the token owner must provide two arguments: the NFT identifier tokenId and the NFT fighter type fighterType.

The issue arises because the function does not verify that the provided fighterType matches the one associated with the tokenId NFT. Since the reroll maximum limits for each fighter type are handled separately (number of generation is different for both fighter types as they arn't increase at the same time see the incrementGeneration function), users can bypass the limit for their current tokenId fighter type by providing a different fighter type as input to the function.

This loophole enables fighter NFT owners to potentially reroll their fighters more times than intended by the protocol, which contradicts the protocol's intent.

Proof of Concept

Let's illustrate this issue with a simple scenario:

• Bob owns a fighter NFT with tokenId = 9 and fighterType = 1.

• Before Bob calls reRoll function, we had the following states in the contract: numRerolls[tokenId = 9] = 3 & maxRerollsAllowed = [7, 3]

• In the normal condition, Bob shouldn't be allowed to reroll his fighter as he has reached the maximum allowed rerolls, i.e., numRerolls[tokenId = 9] == maxRerollsAllowed[fighterType = 1].

• However, Bob still calls the reRoll function and provides the wrong fighter type fighterType = 0. He bypasses the maximum reroll check as numRerolls[tokenId = 9] = 3 < maxRerollsAllowed[0] = 7.

• Since the reRoll function doesn't validate that the provided fighterType matches the fighter NFT's type, Bob's call succeeds, allowing him to reroll his NFT. This results in the following state after the call: numRerolls[tokenId = 9] = 4 > maxRerollsAllowed[fighterType = 1] = 3.

• Bob has successfully rerolled his fighter NFT, bypassing the maximum reroll check due to the mentioned issue.

Impact

Users may bypass the maximum reroll allowed check in the reRoll function, potentially rerolling their fighter NFTs more times than intended by the protocol.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, two options are available:

• Remove the fighterType argument from the reRoll function and directly use the type stored in storage.

• Add a check to ensure that the provided fighterType in the reRoll function matches the one associated with the provided fighter tokenId.

Assessed type

Context

Lines of code

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

Vulnerability details

Issue Description

In the FighterFarm contract, the numElements mapping represents the number of elements in each generation and is initialized in the constructor for the first generation as numElements[0] = 3.

However, when the generation is increased in the incrementGeneration function, the numElements mapping is not updated:

function incrementGeneration(uint8 fighterType) external returns (uint8) {
    require(msg.sender == _ownerAddress);
    generation[fighterType] += 1;
    maxRerollsAllowed[fighterType] += 1;
    //@audit numElements not updated
    return generation[fighterType];
}

This causes the numElements mapping to always remain equal to 0 from generation 1 onward (numElements[generation[fighterType] > 0] == 0). Consequently, the _createFighterBase function always returns a zero value for the element field, which is essential for the fighter NFT. This issue is apparent in the following code:

function _createFighterBase(
    uint256 dna,
    uint8 fighterType
) private view returns (uint256, uint256, uint256) {
    //@audit numElements[generation[fighterType]] will be 0 when generation[fighterType] > 0
    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = (dna % 31) + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
}

Since numElements[generation[fighterType] > 0] == 0 we will have element = dna % 0 = 0, resulting in the element field always being 0 for generations 1, 2, and so forth. This incorrect feature representation for the fighter NFTs can potentially disrupt the protocol and the game as all the NFT minting function _createNewFighter always call under the hood _createFighterBase.

Impact

The _createFighterBase function will return incorrect values for the fighter NFT element field, leading to inaccurate NFT representation and potentially causing issues for the protocol and the game.

Tools Used

Manual review, VS Code

Recommended Mitigation

To resolve this issue, ensure that the numElements mapping is updated when increasing the fighter generation in the incrementGeneration function.

Assessed type

Context

Lines of code

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

Vulnerability details

Issue Description

In the MergingPool contract, the claimRewards function allows users who have been selected as fighter NFT winners in pickWinner to claim their won NFTs, which are minted through the _fighterFarmInstance.mintFromMergingPool call within claimRewards.

The claimRewards function iterates over each round and mints NFTs to users who are included in the round's winners array:

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++) {
        numRoundsClaimed[msg.sender] += 1;
        winnersLength = winnerAddresses[currentRound].length;
        //@audit reentrancy possible
        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {
                _fighterFarmInstance.mintFromMergingPool(
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );
                claimIndex += 1;
            }
        }
    }
    if (claimIndex > 0) {
        emit Claimed(msg.sender, claimIndex);
    }
}

The issue arises because the _fighterFarmInstance.mintFromMergingPool function calls _safeMint internally to mint the NFT to the user (invoked in the _createNewFighter internal function), which triggers the onERC721Received callback. This callback enables the user to reenter the claimRewards function, potentially claiming more NFTs than intended :

function _createNewFighter(
    address to,
    uint256 dna,
    string memory modelHash,
    string memory modelType,
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
) private {
    ...
    //@audit will trigger `onERC721Received` callback
    _safeMint(to, newId);
    FighterOps.fighterCreatedEmitter(
        newId,
        weight,
        element,
        generation[fighterType]
    );
}

A reentrancy attack can occur if a user has has two or more unclaimed NFTs, in that case the following steps outline how such an attack can be executed:

• The user calls the claimRewards function to claim their won NFTs.
• During execution, claimRewards mints the first NFT won by the user, triggering the onERC721Received callback.
• The user utilizes this callback to call claimRewards again, he won't be able to claim the first token again as numRoundsClaimed is incremented beyond that round but he can mint the second (and third,fourth,...) NFT that he has won in the rounds that followed.
• Upon completion of the reentrancy call, the initial claimRewards call resumes. As the numRoundsClaimed is not checked within the loop, the function continues to increment it beyond the current roundId.
• Consequently, the user can claim more NFTs than intended, and his numRoundsClaimed tracker has been incremented beyond the current roundId so he will no be able to claim again for some rounds but this is not relevant as he was able to claim an NFT for free.

This vulnerability can potentially be exploited by winners to obtain more NFTs than intended, resulting in the minting of NFTs that were not supposed to be created.

Proof of Concept

Consider the following scenario:

• Assume roundId = 15, and Bob has been selected as an NFT winner in both round 2 and round 11.
• ob didn't claim the first time he was selected and decides to claim his rewards now with numRoundsClaimed[Bob] = 0.
• Bob calls the claimRewards function, which will be looping through rounds [0, 14] to mint the NFTs.
• At currentRound = 2, _fighterFarmInstance.mintFromMergingPool is called, minting an NFT to Bob. At this point, numRoundsClaimed[Bob] = 3.
• Bob uses the _safeMint (which triggers the onERC721Received callback) to reenter claimRewards, and this time, the function loop iterates over rounds [3, 14].
• Whenn the loop reaches currentRound = 11, the second NFT won by Bob is minted. Then the loop completes, and the call ends with numRoundsClaimed[Bob] = 15.
• The initial claimRewards call resumes, continuing from currentRound = 3 (as currentRound is indepandent from numRoundsClaimed after the loop has started). At currentRound = 11, the second NFT is again minted to Bob, and the function call ends with numRoundsClaimed[Bob] = 26.
• So Bob managed to claim three NFTs instead of the two he has won, and we notice that the numRoundsClaimed tracker is set to a future round meaning that Bob want be able to claim in those future rounds but this is irrelevant as Bob was able to take a free NFT.

This attack can be done whenever a user has won multiple times and didn't claim his NFTs, and the more he has won the more he can take free NFTs with this attack.

Impact

Winners in the Merging Pool can exploit reentrancy to fraudulently claim additional NFTs when using the claimRewards function.

Tools Used

Manual review, VS Code

Recommended Mitigation

Implement a nonReentrant modifier to prevent reentrancy attacks in the claimRewards function.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L285-L287

Vulnerability details

Issue Description

In the RankedBattle contract, users are required to stake their fighter NFT and a specified amount of NRN tokens to earn NRN rewards generated from won battles. Staking is facilitated by the stakeNRN function, which locks both the fighter NFT (changes state to staked in the FighterFarm contract) and NRN stake amount.

The unstakeNRN function permits users to unstake their staked NRN tokens at their discretion. If all the staked NRN tokens are withdrawn, users can retrieve their fighter NFT.

However, the function contains a loophole that enables users to claim back their NFT (unstake it) while continuing to earn points from battles, and consequently, NRN rewards. This occurs because the function only verifies that amountStaked[tokenId] is zero when unstaking the NFT and does not consider the stakeAtRisk amount:

function unstakeNRN(uint256 amount, uint256 tokenId) external {
    ...

    bool success = _neuronInstance.transfer(msg.sender, amount);
    if (success) {
        //@audit doesn't account for StakeAtRisk amount
        if (amountStaked[tokenId] == 0) {
            _fighterFarmInstance.updateFighterStaking(tokenId, false);
        }
        emit Unstaked(msg.sender, amount);
    }
}

As observed, the function solely checks the current staked amount for the corresponding token ID amountStaked[tokenId]. However, in this game, a portion of the staked amount can be temporarily put at risk when a user loses a game, this amount is deducted from the initial staked amount, and held in the StakeAtRisk contract. The actual at-risk amount can be retrieved using _stakeAtRiskInstance.getStakeAtRisk(tokenId).

Therefore, at any given timestamp, the user's actual stake amount is amountStaked[tokenId] + _stakeAtRiskInstance.getStakeAtRisk(tokenId), where the at-risk stake can be zero if the user hasn't lost games.

In essence, a user could strategically lose games to place a portion of the staked amount at risk, then unstake all remaining amounts until amountStaked[tokenId] == 0, allowing them to claim back the locked NFT. Subsequently, the user can proceed to play again to recover the at-risk amount and continue playing the game, earning points, and therefore NRN rewards without having an NFT staked. And this is possible because the updateBattleRecord function does account for the stakeAtRisk amount when adding game result and points as shown below :

function updateBattleRecord(
    uint256 tokenId,
    uint256 mergingPortion,
    uint8 battleResult,
    uint256 eloFactor,
    bool initiatorBool
)
    external
{
    ...
    _updateRecord(tokenId, battleResult);
    uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
    //@audit stakeAtRisk is checked
    if (amountStaked[tokenId] + stakeAtRisk > 0) {
        _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
    }
    ...
}

The loophole will potentially allow users to play the games and earn NRN rewards without staking any fighter NFT which goes against the intent of the protocol.

Proof of Concept

Let's outline the scenario demonstrating this issue:

• Bob initially stakes his fighter NFT and 1000 NRN tokens, resulting in amountStaked[tokenId] = 1000.

• Bob participates in battles and loses some, leading to a portion of his stake being put at risk. At this point, the contract state is:

amountStaked[tokenId] = 800
_stakeAtRiskInstance.getStakeAtRisk(tokenId) = 200

• Bob calls the unstakeNRN function to withdraw his remaining 800 NRN tokens. Since amountStaked[tokenId] = 0 at the end, the function unstakes the fighter NFT, freeing it for Bob (can transfer it freely).

• Later, Bob participates in more battles and wins, allowing him to earn points and reclaim his at-risk stake amount. After winning some games, the contract reaches the state:

amountStaked[tokenId] = 200
_stakeAtRiskInstance.getStakeAtRisk(tokenId) = 0

• Bob has now recovered all his at-risk amount and can continue participating in battles, earning points and claiming NRN rewards without staking his fighter NFT.

Impact

Users can claim NRN rewards without staking their fighter NFTs, undermining the protocol's intended mechanics.

Tools Used

Manual review, VS Code

Recommended Mitigation

To address this issue, the unstakeNRN function must incorporate the at-risk amount into the final staked amount check before unstaking the NFT. The following modification achieves this:

function unstakeNRN(uint256 amount, uint256 tokenId) external {
    require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
    if (amount > amountStaked[tokenId]) {
        amount = amountStaked[tokenId];
    }
    amountStaked[tokenId] -= amount;
    globalStakedAmount -= amount;
    stakingFactor[tokenId] = _getStakingFactor(
        tokenId,
        _stakeAtRiskInstance.getStakeAtRisk(tokenId)
    );
    _calculatedStakingFactor[tokenId][roundId] = true;
    hasUnstaked[tokenId][roundId] = true;
    bool success = _neuronInstance.transfer(msg.sender, amount);
    if (success) {
--      if (amountStaked[tokenId] == 0) {
++      uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
++      if (amountStaked[tokenId] + stakeAtRisk == 0) {
            _fighterFarmInstance.updateFighterStaking(tokenId, false);
        }
        emit Unstaked(msg.sender, amount);
    }
}

Assessed type

Context