AI Arena - 0xRiO's results

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#L484-L531

Vulnerability details

Description

• When a user won in Merging pool allows the user to batch claim rewards for multiple rounds through the function below.

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;
            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);
        }
    }

• This makes a call _fighterFarmInstance.mintFromMergingPool which then passes the inputs to _createNewFighter function of RankedBattle contract as follows.

function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

• The _createNewFighter function includes a check to determine whether the call is for a custom Fighter or a regular one, based on the first element in customAttributes. However, this check solely relies on the value 100, which can be accidentally done by users resulting in obtaining a regular Fighter for an inflated price.

        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }

• This check is not enough as if user passes the first element in customAttributes of his custom Fighter is 100, the user will end up getting a regular Fighter for a overpriced amount.

Tools Used

Manual Analysis

Recommended Mitigation Steps

• There should be some other checks to check weather the call is for a custom Fighter or a regular Fighter

Assessed type

Other

uint256 element = dna % numElements[generation[fighterType]];
uint256 weight = dna % 31 + 65;

if (customAttributes[0] == 100) {
  (element, weight, newDna) = _createFighterBase(dna, fighterType);
} else {
  element = customAttributes[0];
  weight = customAttributes[1];
  newDna = dna;
}

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/RankedBattle.sol#L294-L311

Vulnerability details

Impact

• The gas inefficiency in the claimRewards function significantly impacts the experience of new users, as increasing high gas costs during reward claiming.
• Moreover, as the number of rounds increases, the cumulative gas consumption will surpass the block gas limit, making the feature unusable.
• New winners would be unable to claim their customFighter rewards, thereby undermining the platform's functionality and discouraging user engagement.

Proof of Concept

• This is a example test to show if the user won in roundId = 0 and will call the claim function after n rounds

function testDosOfClaimRewards() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        
        for (uint i = /* update number of rounds here */; i != 0; ){
            uint256[] memory _winnerOfNext100Rounds = new uint256[](2);
            _winnerOfNext100Rounds[0] = 1;
            _winnerOfNext100Rounds[1] = 2;
            _mergingPoolContract.pickWinner(_winnerOfNext100Rounds);
            unchecked {
                --i;
            }
        }

        string[] memory _modelURIs = new string[](1);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](1);
        _modelTypes[0] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        // winner of roundId 0 claims rewards
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    }

• By using this test in MergingPool.t.sol with the command below will show the gas report of the following test

forge test --mt testDosOfClaimRewards --gas-report

• Below are some of the results of gas used depending on after how many rounds the user calls the function

-> after 100 rounds
    | src/MergingPool.sol:MergingPool contract |                 |        |        |        |         |
    |------------------------------------------|-----------------|--------|--------|--------|---------|
    | Deployment Cost                          | Deployment Size |        |        |        |         |
    | 912202                                   | 4340            |        |        |        |         |
    | Function Name                            | min             | avg    | median | max    | # calls |
    | claimRewards                             | 624524          | 624_524| 624524 | 624524 | 1       |
-> after 200 rounds
    | src/MergingPool.sol:MergingPool contract |                 |        |        |        |         |
    |------------------------------------------|-----------------|--------|--------|--------|---------|
    | Deployment Cost                          | Deployment Size |        |        |        |         |
    | 912202                                   | 4340            |        |        |        |         |
    | Function Name                            | min             | avg    | median | max    | # calls |
    | claimRewards                             | 838124          | 838_124| 838124 | 838124 | 1       |
-> after 13853 rounds 
-> At this point the function is unusable and, both the new users after this and the winner of roundId = 0 will be unable to claim reward
    | src/MergingPool.sol:MergingPool contract |                 |          |          |          |         |
    |------------------------------------------|-----------------|----------|----------|----------|---------|
    | Deployment Cost                          | Deployment Size |          |          |          |         |
    | 912202                                   | 4340            |          |          |          |         |
    | Function Name                            | min             | avg      | median   | max      | # calls |
    | claimRewards                             | 30000932        |30_000_932| 30000932 | 30000932 | 1       |

Tools Used

Manual Review

Recommended Mitigation Steps

• Using the view function getUnclaimedRewards to give the count of unclaimed reward of the user and the claim function can just verify if the input lengths is equal to the number of reward user can claim if that is equal function can proceed with creating new fighter acording to inputs user has given.

Instance

• Same kind of DoS can be happen in RankedBattle::claimNRN function where also we are iterating over history of every round to claimNRN. https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L294-L311

Assessed type

DoS

Gas Analysis Report

[G-1] AiArenaHelper::attributes.length can be initialized as a constant

Description

The attributes.length variable is used throughout the contract and can be initialized as constant. This constant declaration oversight can lead to increased gas costs during contract deployment and function executions. Also array attributes is not changing throught the contract

Cause

The cause of the issue is the absence of a constant declaration for the attributes.length variable. As a result, each reference to attributes.length incurs gas costs for storage reads, impacting contract efficiency.

Proposed Correction:

uint8 constant public attributesLength = 6;

[G-2] FighterFarm::redeemMintPass require statement can be made more gas efficient

Description

The redeemMintPass function contains redundant require statements to validate the lengths of input arrays, leading to inefficient gas usage.

Cause

The cause of the issue is the redundant require statements used to validate the lengths of input arrays (mintpassIdsToBurn, mintPassDnas, fighterTypes, modelHashes, and modelTypes). These redundant require statements contribute to gas inefficiency.

Proposed Correction:

• Making a variable length and then comparing with

// Validate lengths of input arrays
uint256 length = mintpassIdsToBurn.length;
require(
    length == mintPassDnas.length && 
    length == fighterTypes.length && 
    length == modelHashes.length &&
    length == modelTypes.length,
    "Array lengths do not match"
);

for (uint16 i = 0; i < length; i++) {
    require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]), "Sender is not the owner of the mint pass");
    _mintpassInstance.burn(mintpassIdsToBurn[i]);
    _createNewFighter(
        msg.sender, 
        uint256(keccak256(abi.encode(mintPassDnas[i]))), 
        modelHashes[i], 
        modelTypes[i],
        fighterTypes[i],
        iconsTypes[i],
        [uint256(100), uint256(100)]
    );
}

[G-3] Using != instead of < or > in loops/conditions will gave gas

PoC

• This is a function with loop that does nothing execution cost 212 gas when using i>0 and 206 when using i!=0 with increasing iteration this difference can quickly increase resulting in high gas cost for calling functions.

function checkingGasOfLoop() public {
        for (uint256 i =1;i!=0;){
        unchecked{
            --i;
        }
     }
    }

• One more example where we can save gas

if (points > 0) {}

Instances

Total 22 instance throughout the scope https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L48 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L73 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L99 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L136 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L148 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L178 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L211 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L249 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L124 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L149 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L152 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L176 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L178 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L207 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L131 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L299 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L390 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L469 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L488 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L164 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L245 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L306

[G-4] Updating numRoundsClaimed[msg.sender] mapping in RankedBattle::claimNRN function can be made gas efficent.

Description

The claimNRN function contains inefficiencies in updating the numRoundsClaimed mapping, leading to unnecessary gas consumption.

Proposed Correction:

function claimNRN() external {
    require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
    uint256 claimableNRN = 0;
    uint256 nrnDistribution;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        nrnDistribution = getNrnDistribution(currentRound);
        claimableNRN += (
            accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
        ) / totalAccumulatedPoints[currentRound];
-       numRoundsClaimed[msg.sender] += 1;
    }
    // Update numRoundsClaimed mapping outside the loop
+    numRoundsClaimed[msg.sender] = uint32(roundId-1); 
    if (claimableNRN > 0) {
        amountClaimed[msg.sender] += claimableNRN;
        _neuronInstance.mint(msg.sender, claimableNRN);
        emit Claimed(msg.sender, claimableNRN);
    }
}

Gas difference

• Before Function gas cost : 58_844
• After updating it outside loop Function gas cost : 38_860

Instance

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L299-L305 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L149-L163