AI Arena - Tekken's results

Lines of code

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

Vulnerability details

Description:

Both claimFighters and mintFromMergingPool call to _createNewFighter, which then calls to _createFighterBase, with predictable dna. The dna consists of keccak256(abi.encode(msg.sender, fighters.length)). msg.sender is known and addresses can be generated programatically to generate a more fitting address for a better dna sequence, and fighters.length is publicly accessible because we can see how many tokens were minted in total.

The exception of FighterFarm::reRoll, is that the predictable dna looks like-so: keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])). And calls directly to _createFighterBase.

_createFighterBase is a function that generates pseudo-random values:

    function _createFighterBase(uint256 dna, uint8 fighterType) public view returns (uint256, uint256, uint256) {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

By being able to predict the dna we can derive element and weight and newDna. newDna is then used in AiArenaHelper::createPhysicalAttributes(which is used during both reRoll and _createNewFighter functions) and the rarityRank can be derived directly from that value for all the attributes(head, eyes, mouth, body, hands, feet).

    function createPhysicalAttributes(
        uint256 dna, 
        uint8 generation, 
        uint8 iconsType, 
        bool dendroidBool
    ) 
        external 
        view 
        returns (FighterOps.FighterPhysicalAttributes memory) 
    {
            .
            .
            .
            for (uint8 i = 0; i < attributesLength; i++) {
                if (
                  i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
                  i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
                  i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
                ) {
                    finalAttributeProbabilityIndexes[i] = 50;
                } else {
=======HERE=======> uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100; <====HERE======
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
                    finalAttributeProbabilityIndexes[i] = attributeIndex;
                .
                .
                .
        }
    }

Inside the line in question, the attributeToDnaDivisor is accessible, which aids in predicting the outcome.

Impact:

The impact is severe since whoever is minting can time when to mint for a fitting tokenId, and address to mint it from, to precisely predict the outcome of the claiming, minting or rerolling.

The aiarena documentation of nft makeup states that dna sequence is to be generated off-chain, but it is not implemented.

Recommended Mitigation:

Use Chainlink VRF or introduce signatures for minting these like in claimFighters. However the signature needs to involve the dna sequence or a second signature for solely handling dna sequence randomness needs to be introduced, so it protects from tinkering and predicting the values based on the dna.

Assessed type

Other

Low

[L-1] MergingPool::getFighterPoints should take two arguments instead of one, to be able to preview a range of fighter's points, instead of iterating through all of them up to the maxId

Impact: The function getFighterPoints currently does not provide a way to set a starting index for points to be retrieved. It always starts from the beginning (i = 0). If you want to provide a way to get fighter points inside a range, you should include a startId parameter to the function.

Recommended Adjustment:

    function getFighterPoints(uint256 startId, uint256 endId) public view returns(uint256[] memory) {
        uint256 range = endId - startId;
        uint256[] memory points = new uint256[](range);
        for (uint256 i = startId; i < range; i++) {
            points[i] = fighterPoints[i];
        }
        return points;
    }
}

[L-2] Unused event Neuron::TokensMinted which is missing from the Neuron::constructor and Neuron::mint, to emit of the event of minting tokens

Description: Event TokensMinted is not emitted during minting in the constructor and mint functions. The ERC20's event for the _mint function only displays a Transfer function, which could be cofusing when inspecting the events, even-though it is a transfer, it's still a minting function and that should be made clear by utilizing the unused TokensMinted event.

Impact: It's bad practice to miss out on emitting events for essential state modifications or significant function calls.

Recommended Adjustment:

    constructor(address ownerAddress, address treasuryAddress_, address contributorAddress)
        ERC20("Neuron", "NRN")
    {
        _ownerAddress = ownerAddress;
        treasuryAddress = treasuryAddress_;
        isAdmin[_ownerAddress] = true;
        _mint(treasuryAddress, INITIAL_TREASURY_MINT);
        _mint(contributorAddress, INITIAL_CONTRIBUTOR_MINT);
+       emit TokensMinted(treasuryAddress, INITIAL_TREASURY_MINT)
+       emit TokensMinted(contributorAddress, INITIAL_CONTRIBUTOR_MINT)
    }
    .
    .
    .
    function mint(address to, uint256 amount) public virtual {
        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
+       emit TokensMinted(to, amount)
    }

Informational

[I-1] In the function Neuron::claim, transferFrom is not checked for success

Recommended Adjustment:

    function claim(uint256 amount) external {
        require(
            allowance(treasuryAddress, msg.sender) >= amount, 
            "ERC20: claim amount exceeds allowance"
        );
-       transferFrom(treasuryAddress, msg.sender, amount);
-       emit TokensClaimed(msg.sender, amount);
+       bool success = transferFrom(treasuryAddress, msg.sender, amount);
+       if (success) {
+           emit TokensClaimed(msg.sender, amount);
+       }
    }