H-06 MitigationConfirmed

[c4-bot-4]

Lines of code

Vulnerability details

Lines of code

Old lines of code

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

Mitigated lines of code

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/setUpAirdrop-mitigation/src/FighterFarm.sol#L415

Vulnerability details

The issue was reported in #68.

The function FighterFarm.reRoll() permits player recomputing fighter traits, i.e., computing the AiArenaHelper.createPhysicalAttributes() function on a different dna computed according to msg.sender, tokenId which represents fighter ID and numRerolls[tokenId], i.e., the number of times the player tried to recompute that fighter traits. During reRoll() calls, only the numRerolls[tokenId] value changes.

The vulnerability relies on line #370:

FighterFarm.sol#L370

    function reRoll(uint8 tokenId, uint8 fighterType) public {

As we said above, tokenId which represents fighter ID. In general, the fighter ID is a uint256 and so, it can assume every value in [0, 2^255 - 1]. However, the reRoll() function uses a uint8 to represent tokenId. This means that reRoll() can not be successfully called for tokenId values in [2^8, 2^255 - 1]. This has a huge impact on players because they can not recompute fighters' traits if their fighters have an ID greater than 255.

Recommended Mitigation proposed by wardens

The proposed mitigation step is to use uint256 to represent tokenId inside FighterFarm.reRoll() method:

    /// @notice Rolls a new fighter with random traits.
    /// @param tokenId ID of the fighter being re-rolled.
    /// @param fighterType The fighter type.
-   function reRoll(uint8 tokenId, uint8 fighterType) public {
+   function reRoll(uint256 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }  

This solution was implemented by the Ai Arena team

Comment about the Mitigation Proposal

We think this is a valid mitigation proposal. In this way, it is possible to call the FighterFarm.reRoll() method on any tokenId until the maxRerollsAllowed value is reached.

We want to underline that the numRerolls value is not reset when a fighter is transferred from one player to another. So, numRerolls represents a general limit for a specific tokenId. Furthermore, a player can forecast how much reRolls are needed to obtain the rare fighter. Developers tried to mitigate this problem in #16:

    /// @notice Rolls a new fighter with random traits.
    /// @param tokenId ID of the fighter being re-rolled.
    /// @param fighterType The fighter type.
    function reRoll(uint256 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
-           uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
+           uint256 dna = uint256(keccak256(abi.encode(tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }  

In this way, they removed the possibility to manipulate the FighterFarm.reRoll() outcome. However, it is still possible to forecast the next reRolls and decide when to stop calling it.

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as confirmed for report