code-423n4 / 2024-04-ai-arena-mitigation-findings

0 stars 0 forks source link

H-04 MitigationConfirmed #25

Open c4-bot-10 opened 5 months ago

c4-bot-10 commented 5 months ago

Lines of code

Vulnerability details

Lines of code

Vulnerability details

C4 issue

H-04: 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

Comments

Previously, users could FighterFarm::reRoll their fighter using a corrupted fighter type, because the fighter type was provided as input and not validated. For this reason, it was possible to re-roll attributes based on a different fighter type and maxRerollsAllowed could be bypassed in some scenarios.

Mitigation

PR #17 Now it is correctly checked whether the fighterType value passed as input corresponds to the type of the fighter being re-rolled:

    function reRoll(uint256 tokenId, uint8 fighterType) public {
        require((fighterType == 1) == fighters[tokenId].dendroidBool, "Type mismatch");

Note that fighterType cannot be greater than 1, because otherwise the call will revert at L418 due to an index out of bounds error trying to access maxRerollsAllowed[fighterType]. Therefore, fighterType is either 1 (dendroid) or 0.

Suggestion

Although the fix solves issue H-04, the fix could have been simpler. The fighter state variable dendroidBool already contains the fighter type information needed, so it is redundant to ask callers of reRoll() for the fighter type. The function could have been changed to:

/// @notice Rolls a new fighter with random traits.
/// @param tokenId ID of the fighter being re-rolled.
function reRoll(uint256 tokenId) public {
    uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0;
    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(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] = "";
    }
}   

Conclusion

LGTM

c4-judge commented 5 months ago

jhsagd76 marked the issue as satisfactory