AI Arena - ReadyPlayer2's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L118

Vulnerability details

Impact

In MergingPool.sol the function claimRewards is vulnerable to a dos attack. The main impact of this attack is that once the cost of calling the function exceeds the block gas limit the user can no longer claim their funds and they remain locked in the protocol permanently.

Proof of Concept

The error is quite evident in analyzing the claimRewards function as follows

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        //First loop
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            //Second loop
            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);
        }
    }

From above it is clear to see two unbounded loops namely the First and Second loop.

The First loop is unbounded because it is dependent on the number of rounds a user has not claimed rewards. For example consider a user that has never had a single victory in the course of a 1000 rounds and finally wins a reward. This means this loop will consume an enormous amount of gas. To further explain consider this excerpt from the code above:

 uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {

It is clear to see that if a user has never claimed their lowerBound will be equal to zero and the roundId will be a larger value since the user has not claimed for multiple rounds.

However this is compounded by the unbounded loop 2, which iterates through the number of winners for each current roundId. Although the owner provides the winners array it is evident that even though the user has no winnings for that roundId, the function still iterates over the loop, causing a massive gas overhead. Consider:

winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {

It is evident that this loop also causes a massive gas overhead. Once the call to claimRewards exceeds the maximum block limit a users rewards are locked permanently in the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

I would recommend the function be overhauled to allow users to claim for only a specific round, or to allow users to claim for a specific range.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L294

Vulnerability details

Impact

The main impact is that user rewards can be locked in the protocol permanently as soon as the cost of calling the function exceeds the block gas limit.

Proof of Concept

Suppose a user named Alice is a user of the protocol.

Alice has been playing for a while and is accumulating rewards but has not claimed any yet, the current roundId in Ranked Battle is 1000. Which means there have been a 1000 total rounds since the beginning of the protocol. So Alice decides to claim by calling the function claimNRN which is as follows

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;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

Taking a code block from above and annotating for the current situation:

        //uint32 lowerBound = 0;
        //Since the user has not claimed before
        uint32 lowerBound = numRoundsClaimed[msg.sender];

        //for (uint32 currentRound = 0; currentRound < 1000; currentRound++) {...
        //Substituting since a 1000 rounds have passed
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {

It is evident that the longer a user takes to claim, the higher the chance of their call to claim exceeding the block gas limit.

Tools Used

Manual Review

Recommended Mitigation Steps

I would recommend that the function be changed to allow user to claim for a specific roundId or a certain range.

Assessed type

DoS