AI Arena - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

1. Certain GameItems like SoulBound items are never meant to be transferred. It can directly impact in app purchases as Users/Friends can transfer item between each other instead of buying.
2. This makes the game unfair as User can exhaust there daily allowance for a game item (say battery) from multiple account and then transfer to a single account which then let user get unfair advantage in game

Proof of Concept

1. As we can see in below POC, GameItem was transferred even though it was marked non transferrable. The main culprit here is safeBatchTransferFrom which was not overidden with the custom logic

function testBatchSafeTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
		
		uint256[] memory ids = new uint256[](1);
		uint256[] memory values = new uint256[](1);
		ids[0]=0;
		values[0]=1;
		
		_gameItemsContract.adjustTransferability(0,false);
		
		vm.expectRevert();
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, values, "");
		
    }

Result:
[FAIL. Reason: Call did not revert as expected] testBatchSafeTransferFrom() (gas: 221497)

Recommended Mitigation Steps

Override the safeBatchTransferFrom function and add the restriction of failing non transferrable items

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory values,
        bytes memory data
    ) public override(ERC1155) {
                require(allGameItemAttributes[tokenId].transferable);
        super.safeBatchTransferFrom(from, to, ids, values, data);
    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Users can ensure no loss with high reward ratio, making the game unfair. This happens since even the smallest + points are enough to absorb a defeat with huge stake

Proof of Concept

1. User can stake 1 wei token and wins one game
2. Post winning he will get minimal points
3. Now he can stake very large amount and play game
4. If he loses then he lose only the minimal points from step 2
5. If he wins then he gains point for the large sum staked
6. So loss is near to zero. This means player can play with huge stake without any risk of losing fund

POC

1. testNegligibleLoss shows that high staked game loss will not take away user fund and impact negligible points. Hence User loss is per old staked amount
2. testHighProfitNoRisk shows that User gain is as per current stake amount

function testNegligibleLoss() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
		
		// staking smallest possible value 1 wei of token
        _rankedBattleContract.stakeNRN(1, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1);
        
		// Game server mark this battle as won
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);

		// Points will be 1 (staking Factor) * 1500 (eloFactor)
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(tokenId, 0) == 1500, true);
		
		// Player stakes huge fund
		vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
		
		// Battle is lost
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1500, true);
		
		// Bug: No fund loss, only the minimal points obtained are lost but fund are safe
		// Bug: If User directly started with this much fund, he would have lost fund
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(tokenId, 0) == 0, true);
		assertEq(_stakeAtRiskContract.getStakeAtRisk(tokenId) > 0, true);
    }

Outcome:
[FAIL. Reason: Assertion failed.] testNegligibleLoss() (gas: 829347)

	function testHighProfitNoRisk() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
		
		// staking smallest possible value 1 wei of token
        _rankedBattleContract.stakeNRN(1, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1);
        
		// Game server mark this battle as won
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);

		// Points will be 1 (staking Factor) * 1500 (eloFactor)
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(tokenId, 0) == 1500, true);
		
		// Player stakes huge fund
		vm.prank(player);
        _rankedBattleContract.stakeNRN(2_500 * 10 ** 18, 0);
		
		vm.prank(address(_GAME_SERVER_ADDRESS));
		// Battle won
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
		
		//1500(from old win) + sqrt(2500)*1500 = 76500 earned with no risk
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(tokenId, 0) == 76500, true);
    }

Recommended Mitigation Steps

In case of User losing game, his points for current stake amount should be deduced. if existing positive accumulatedPointsPerFighter is < deduced current points then user curStakeAtRisk should be moved and user staked amount should be reduced

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

It seems that point earned for 1 wei and 1e18 NRN token amount would be same. However 1 wei amount of token causes 0 loss even on losing game which is not the case with 1e18 amount tokens This happens since stakingFactor_ is always rounded up to 1 when 0

Proof of Concept

1. On staking very low amount say 1 wei, calculated staking factor will be 0

 uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
// 1/10**18 = 0

2. But a 0 staking factor is automatically rounded up to 1

if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }

3. This means this user will get same points as person staking 1e18 amount

points = stakingFactor[tokenId] * eloFactor;

4. But in case of user losing game, amount lost will be 0 in case of 1 wei amount since

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

// 10*1/10**4 = 0

POC

Below POC show same point for 1 and 1e18 amount token

function testSameGain() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
		
		// staking smallest possible value 1 wei of token
        _rankedBattleContract.stakeNRN(1, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1);
        
		// Game server is going to mark the battle as won
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
		
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0) == 1500, true);
		
		vm.prank(player);
		_rankedBattleContract.unstakeNRN(1, 0);
		
		_rankedBattleContract.setNewRound();
		
		vm.prank(player);
		
		// staking smallest possible value 1 wei of token
        _rankedBattleContract.stakeNRN(1*10**18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1*10**18);
        
		// Game server is going to mark the battle as won
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
		
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0) == 1500, true);

    }

Recommended Mitigation Steps

A minimum staking amount requirement should be placed which will prevent such small deposits

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If you lost a game which moved the stake to stakeAtRisk contract then you can recover those funds without risking any more funds. Even if you lose, you wont lose more funds but if you win you will start recovering back the funds

Proof of Concept

1. Seems like server can update game stats for a user with 0 amountStaked.
2. Lets say User lost the game which kept his X funds on stakeAtRisk contract
3. User can simply unstake rest funds which make amountStaked to 0
4. Now even if he lose the game below code is executed

bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }

5. Since amountStaked is 0 so this reverts and server cannot update user loss
6. In case user wins, he claims back some of the stake at risk

if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

7. Once he recover some part of fund, he immediately unstake it
8. User repeats step 6-7 to recover remaining fund

POC

Below test fails since balance does not decrease on losing game

function testUseRiskFund() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
		
        vm.prank(player);
		
		// stake amount
        _rankedBattleContract.stakeNRN(2500*10**18, 0);
        
		// Game server is going to mark the battle as lost
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1, true);
		
		assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 2500*10**15);
		
		vm.prank(player);
		// unstake amount 
		_rankedBattleContract.unstakeNRN((2500*10**18)-(2500*10**15), 0);
		
		assertEq(_rankedBattleContract.amountStaked(0), 0);
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), 0);
		
		// Game server is going to mark the battle as lost
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1, true);
		
		// no loss for user
// this fails
vm.expectRevert();
		assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 2500*10**15);
		assertEq(_rankedBattleContract.amountStaked(0), 0);
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), 0);
		
				// Game server is going to mark the battle as won
		vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1, true);
		
		// profit from win gets added
		assertEq(_stakeAtRiskContract.getStakeAtRisk(0), (2500*10**15)-(2500*10**12));
		assertEq(_rankedBattleContract.amountStaked(0), 2500*10**12);
		assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), 0);

    }

Result:
[FAIL. Reason: Call did not revert as expected] testUseRiskFund() (gas: 934506)

Recommended Mitigation Steps

Revise the below line:

-- if (amountStaked[tokenId] + stakeAtRisk > 0) {
--            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
++ if (amountStaked[tokenId] > 0) {
++            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);

Assessed type

Other