code-423n4 / 2024-02-ai-arena-findings

4 stars 3 forks source link

Since you can reroll with a different fighterType than the NFT you own, you can reroll bypassing maxRerollsAllowed and reroll attributes based on a different fighterType #306

Open c4-bot-4 opened 8 months ago

c4-bot-4 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L372

Vulnerability details

Impact

Can reroll attributes based on a different fighterType, and can bypass maxRerollsAllowed.

Proof of Concept

maxRerollsAllowed can be set differently depending on the fighterType. Precisely, it increases as the generation of fighterType increases.

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

The reRoll function does not verify if the fighterType given as a parameter is actually the fighterType of the given tokenId. Therefore, it can use either 0 or 1 regardless of the actual type of the NFT.

This allows bypassing maxRerollsAllowed for additional reRoll, and to call _createFighterBase and createPhysicalAttributes based on a different fighterType than the actual NFT's fighterType, resulting in attributes calculated based on different criteria.

function reRoll(uint8 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 is PoC.

  1. First, there is a bug that there is no way to set numElements, so add a numElements setter to FighterFarm. This bug has been submitted as a separate report.

    function numElementsSetterForPoC(uint8 _generation, uint8 _newElementNum) public {
        require(msg.sender == _ownerAddress);
        require(_newElementNum > 0);
        numElements[_generation] = _newElementNum;
    }
  2. Add a test to the FighterFarm.t.sol file and run it. The generation of Dendroid has increased, and maxRerollsAllowed has increased. The user who owns the Champion NFT bypassed maxRerollsAllowed by putting the fighterType of Dendroid as a parameter in the reRoll function.

    function testPoCRerollBypassMaxRerollsAllowed() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model
        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);
            uint8 exceededLimit = maxRerolls + 1;
            uint8 tokenId = 0;
            uint8 fighterType = 0;
    
            // The Dendroid's generation changed, and maxRerollsAllowed for Dendroid is increased
            uint8 fighterType_Dendroid = 1;
    
            _fighterFarmContract.incrementGeneration(fighterType_Dendroid);
    
            assertEq(_fighterFarmContract.maxRerollsAllowed(fighterType_Dendroid), maxRerolls + 1);
            assertEq(_fighterFarmContract.maxRerollsAllowed(fighterType), maxRerolls); // Champions maxRerollsAllowed is not changed
    
            _neuronContract.addSpender(address(_fighterFarmContract));
    
            _fighterFarmContract.numElementsSetterForPoC(1, 3); // this is added function for poc
    
            for (uint8 i = 0; i < exceededLimit; i++) {
                if (i == (maxRerolls)) {
                    // reRoll with different fighterType
                    assertEq(_fighterFarmContract.numRerolls(tokenId), maxRerolls);
                    _fighterFarmContract.reRoll(tokenId, fighterType_Dendroid);
                    assertEq(_fighterFarmContract.numRerolls(tokenId), exceededLimit);
                } else {
                    _fighterFarmContract.reRoll(tokenId, fighterType);
                }
            }
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Check fighterType at reRoll function.

function reRoll(uint8 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");
+   require((fighterType == 1 && fighters[tokenId].dendroidBool) || (fighterType == 0 && !fighters[tokenId].dendroidBool), "Wrong fighterType");

    _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] = "";
    }
}

Assessed type

Invalid Validation

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #17

c4-pre-sort commented 8 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #212

c4-pre-sort commented 8 months ago

raymondfam marked the issue as sufficient quality report

raymondfam commented 8 months ago

This report covers three consequences from the same root cause of fighter type validation: 1. more re-rolls, 2. rarer attribute switch, 3. generation attribute switch, with coded POC.

c4-pre-sort commented 8 months ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 8 months ago

raymondfam marked the issue as primary issue

c4-sponsor commented 8 months ago

brandinho (sponsor) confirmed

c4-judge commented 8 months ago

HickupHH3 marked the issue as selected for report

HickupHH3 commented 8 months ago

212's fix is a little more elegant

c4-judge commented 8 months ago

HickupHH3 changed the severity to 3 (High Risk)

SonnyCastro commented 6 months ago

Mitigated here