Duplicate Winner Selection Vulnerability in MergingPool Contract's pickWinner Function

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

MergingPool's pickWinner function lacks validation, exposing it to manipulation. Incorrect winner count or invalid token IDs can disrupt reward distribution, while premature point resets might break later logic. Additionally, depending on context, the lack of authorization checks could enable unintended actors to influence winner selection. This combined vulnerability poses a risk to the integrity and fairness of the reward system.

Proof of Concept

Technical Description:

   function pickWinner(uint256[] calldata winners) external {
        require(isAdmin[msg.sender]);
        require(winners.length == winnersPerPeriod, "Incorrect number of winners");
        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;
        roundId += 1;
    }

Observation: An attacker reviews the MergingPool smart contract code and identifies that the pickWinner function does not validate the uniqueness of winner token IDs.

Point Accumulation: The attacker acquires or controls multiple fighter NFTs and earns points in ranked battles to position these NFTs as potential winners.

Exploitation: During the winner selection process, the attacker influences the admin or utilizes an admin account to call pickWinner with an array containing duplicate token IDs for their fighter NFT.

Contract Execution: The MergingPool contract accepts the array with duplicates and assigns rewards to the same token ID multiple times, as there are no checks against the winnerAddresses mapping for the current roundId.

Claiming Rewards: The attacker calls claimRewards and receives multiple rewards for the same fighter NFT, exploiting the contract's logic flaw.

Attack Scenario:

In a blockchain-based game, the MergingPool contract is responsible for distributing NFT rewards based on performance in ranked battles. An attacker, Bob, discovers that the contract's pickWinner function does not enforce unique winner selection. Bob strategically earns points for his fighter NFTs and, when winners are being selected, ensures that his NFT token ID is listed multiple times in the input array to pickWinner.

The contract processes the input without validation and marks the same NFT as a winner multiple times. Bob then uses the claimRewards function to collect more rewards than legitimately due. This exploit leads to an unfair advantage and potential economic imbalance in the game's ecosystem.

Tools Used

manual review

Recommended Mitigation Steps

To address the duplicate winner vulnerability in the MergingPool contract, the following recommendations should be considered:

Implement Uniqueness Checks: Update the pickWinner function to include a check that ensures all winner token IDs are unique. This can be done by using a temporary mapping to track which token IDs have already been added to the winners array.

Use a Set Data Structure: Consider using a set data structure, if available in the Solidity version or through a library, to manage the collection of winners, as sets inherently disallow duplicates.

Limit Admin Control: Introduce additional controls or multi-sig requirements for admin functions, especially those that can significantly impact the game's economy, such as selecting winners.

Automate Winner Selection: If possible, automate the winner selection process based on transparent and deterministic criteria, reducing the need for manual admin intervention and the associated risks.

Audit and Test: Conduct a comprehensive audit of the contract, including all administrative functions, to identify and fix potential vulnerabilities. Implement a robust suite of unit and integration tests to cover edge cases and prevent regressions.

Monitor and Alert: Set up monitoring and alerting systems to detect unusual activity, such as the same NFT winning multiple times in a short period, which could indicate exploitation of the vulnerability.

Emergency Stop Mechanism: Implement a circuit breaker or pause function that can be triggered to halt winner selection and reward distribution in case a vulnerability is being exploited.

Transparent Communication: Communicate with the user base about the vulnerability and the steps being taken to address it. Transparency can help maintain trust within the community.

Assessed type

Other

[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)