MergingPool.pickWinner roundId check is always satisfied leading to potentially set incorrect data

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L121

Vulnerability details

Impact

There is a significant risk of setting incorrect contract round and points-related data on the upcoming round in the MergingPool contract, and such actions are irreversible and can be exploited by the round winners.

Description

The MergingPool.pickWinner function require(!isSelectionComplete[roundId], "Winners are already selected") check is always satisfied. The following state changes will happen as long as winners has the expected number of fighters (with valid owners):

• Winners points-related & round data is set.
• The current round is incremented by 1.

function pickWinner(uint256[] calldata winners) external {
    require(isAdmin[msg.sender]);
    require(winners.length == winnersPerPeriod, "Incorrect number of winners");
    // @audit-issue checks for current roundId, always truthy
    require(!isSelectionComplete[roundId], "Winners are already selected");
    uint256 winnersLength = winners.length;
    address[] memory currentWinnerAddresses = new address[](winnersLength);
    for (uint256 i = 0; i < winnersLength; i++) {
        currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
        totalPoints -= fighterPoints[winners[i]];
        fighterPoints[winners[i]] = 0;
    }
    winnerAddresses[roundId] = currentWinnerAddresses;
    isSelectionComplete[roundId] = true; // @audit-issue worst case scenario it locks future rounds
    roundId += 1; // @audit-info increments current roundId by 1
}

Accidentally replaying the transaction results in setting winners not only for the current round but also for the next one, which enables winners to claim rewards twice via the claimRewards function. Additionally, it leads to incorrect contract round and points-related data, potentially causing the round ID to exceed the RankedBattle one.

Proof Of Concept

Add the following tests in MergingPool.t.sol:

function testPickWinnerReplayed() public {
    // Arrange
    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(_DELEGATED_ADDRESS);
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;
    assertEq(_rankedBattleContract.roundId(), 0);
    assertEq(_mergingPoolContract.roundId(), 0);

    // Act
    _mergingPoolContract.pickWinner(_winners);

    // Assert - MergingPool.roundId is incremented and surpasses RankedBattle.roundId
    assertEq(_rankedBattleContract.roundId(), 0);
    assertEq(_mergingPoolContract.roundId(), _rankedBattleContract.roundId() + 1);

    // Act
    _mergingPoolContract.pickWinner(_winners);

    // Assert - MergingPool.pickWinner roundId check is useless and allows to replay txs and have an unsynced roundId
    assertEq(_rankedBattleContract.roundId(), 0);
    assertEq(_mergingPoolContract.roundId(), _rankedBattleContract.roundId() + 2);
}

Tools Used

Tested with forge and manually reviewed.

Recommended Mitigation Steps

A suggested measure would be to modify the MergingPool contract by:

• Decoupling the roundId increment from the pickWinner function, mirroring the structure observed in the RankedBattle contract (on a separate function).
• Implement a check to ensure that MergingPool.roundId does not surpass RankedBattle.roundId.

The following example illustrates this adjustment:

function pickWinner(uint256 _roundId, uint256[] calldata winners) external {
    require(isAdmin[msg.sender]);
    require(winners.length == winnersPerPeriod, "Incorrect number of winners");
    // @audit-info check roundId does not surpasses the RankedBattle one
    require(_roundId <= _rankedBattleAddress.roundId(), "Unsynced roundId");
    require(!isSelectionComplete[_roundId], "Winners are already selected");
    uint256 winnersLength = winners.length;
    address[] memory currentWinnerAddresses = new address[](winnersLength);
    for (uint256 i = 0; i < winnersLength; i++) {
        currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
        totalPoints -= fighterPoints[winners[i]];
        fighterPoints[winners[i]] = 0;
    }
    winnerAddresses[_roundId] = currentWinnerAddresses;
    isSelectionComplete[_roundId] = true;
    // @audit-info do not increment the roundId
}

// @audit-info sets the current round
function setNewRound() external {
    require(isAdmin[msg.sender]);
    uint256 nextRoundId = roundId + 1;
    require(nextRoundId <= _rankedBattleAddress.roundId(), "Unsynced roundId");
    roundId = nextRoundId;

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #60

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)