AI Arena - emrekocak's results

Lines of code

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

Vulnerability details

Impact

After a certain point, newly registered players and players who have stopped playing for a long time cannot claim the rewards.

Proof of Concept

After a few years roundId will reach very high numbers. Since newly registered player's numRoundsClaimed[player] starts with 0, all newly registered players cannot execute RankedBattle::claimNRN and MergingPool::claimRewards functions because of high gas consumption. It'll be higher than block gas limit. 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

Tools Used

Manual Review

Recommended Mitigation Steps

Newly registered player's numRoundsClaimed value should start current roundId. If numRoundsClaimed[msg.sender] == 0, it's unnecessary to make remaining calculations inside the loop like below. This change will reduce spent gas.

diff --git a/src/RankedBattle.sol#L299-L305
        uint32 lowerBound = numRoundsClaimed[msg.sender];
+       uint256 accumulatedPoints;
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
+           accumulatedPoints = accumulatedPointsPerAddress[msg.sender][currentRound];
            if (accumulatedPoints == 0){
                lowerBound += 1;
            }
+           else{
                nrnDistribution = getNrnDistribution(currentRound);
                claimableNRN += (
-                   accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution
+                   accumulatedPoints * nrnDistribution  
                ) / totalAccumulatedPoints[currentRound];
-               numRoundsClaimed[msg.sender] += 1;
+               lowerBound += 1;
+           }
        }
+       numRoundsClaimed[msg.sender] = lowerBound;

Assessed type

DoS

G-1) If numRoundsClaimed[msg.sender] == 0 it's unnecessary to calculate claimableNRN

If a player is newly registered or has stopped playing for a long time, there will be lots of 0 value in numRoundsClaimed. For each 0 value there is unnecessary claimableNRN calculation. Similar issue also can be found here

Also numRoundsClaimed[msg.sender] should be cached. (This is mentioned in bot races but I wrote it in recommendation to make it clear)

Recommended Mitigation Steps

diff --git a/src/RankedBattle.sol#L299-L305
        uint32 lowerBound = numRoundsClaimed[msg.sender];
+       uint256 accumulatedPoints;
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
+           accumulatedPoints = accumulatedPointsPerAddress[msg.sender][currentRound];
            if (accumulatedPoints == 0){
                lowerBound += 1;
            }
+           else{
                nrnDistribution = getNrnDistribution(currentRound);
                claimableNRN += (
-                   accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution
+                   accumulatedPoints * nrnDistribution  
                ) / totalAccumulatedPoints[currentRound];
-               numRoundsClaimed[msg.sender] += 1;
+               lowerBound += 1;
+           }
        }
+       numRoundsClaimed[msg.sender] = lowerBound;

G-2) Same thing done twice in constructor

This line run already in addAttributeProbabilities function.

Recommended Mitigation Steps

diff --git a/src/AiArenaHelper.sol#L44-L51
        // Initialize the probabilities for each attribute
        addAttributeProbabilities(0, probabilities);

        uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
-           attributeProbabilities[0][attributes[i]] = probabilities[i];
            attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
        }