AI Arena - al88nsk's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L365

Vulnerability details

Impact

The contract FighterFarm overrides ERC-721 functions transferFrom(address,address,uint256) and safeTransferFrom(address,address,uint256) to disallow to transfer staked fighters or to get more than MAX_FIGHTERS_ALLOWED fighters. However, the function safeTransferFrom(address,address,uint256,bytes memory) is not overridden, and that allows to bypass the restriction.

Proof of Concept

A user who has accumulated points for a staked fighter can transfer the staked fighter to another account. Then, when the _addResultPoints is called, the battle result can be one of the below:

1. The fighter wins. In this case accumulated points are incremented for the new owner of the fighter, and after the round is over, both accounts (the new owner and the old one) can claim NRNs for accumulated points
2. The fighter loses. In this case the function will always revert. The condition accumulatedPointsPerFighter[tokenId][roundId] > 0 is true, because it is bound to a fighter, but accumulatedPointsPerAddress[fighterOwner][roundId] is 0, because it is bound to the fighter owner. The fighterOwner currently (after transfer) is the new account with no accumulated points, and subtracting points from accumulatedPointsPerAddress[fighterOwner][roundId] reverts because the result is negative, but the variable type is uint:

if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
	/// If the fighter has a positive point balance for this round, deduct points 
	points = stakingFactor[tokenId] * eloFactor;
	if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
		points = accumulatedPointsPerFighter[tokenId][roundId];
	}
	accumulatedPointsPerFighter[tokenId][roundId] -= points;
	accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
	totalAccumulatedPoints[roundId] -= points;
	if (points > 0) {
		emit PointsChanged(tokenId, points, false);
	}
}

After the round is over, the old fighter owner can claim NRNs for accumulated points, despite the points must be deducted after lose battle.

Here is a test for PoC that can be added to RankedBattleTest (RankedBattle.t.sol):

function testTransferStakedFighter() public {
    address staker = vm.addr(3);
    _mintFromMergingPool(staker);
    _fundUserWith4kNeuronByTreasury(staker);
    vm.prank(staker);
    _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
    assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);

    address claimee = vm.addr(4);
    _mintFromMergingPool(claimee);
    _fundUserWith4kNeuronByTreasury(claimee);
    vm.prank(claimee);
    _rankedBattleContract.stakeNRN(4_000 * 10 ** 18, 1);
    assertEq(_rankedBattleContract.amountStaked(1), 4_000 * 10 ** 18);
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

    address anotherAccount = vm.addr(44);
    vm.prank(staker);
    _fighterFarmContract.safeTransferFrom(staker, anotherAccount, 0, "");

    vm.prank(address(_GAME_SERVER_ADDRESS));
    vm.expectRevert(); // reverts when battle is lose
    _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

    vm.prank(address(_GAME_SERVER_ADDRESS));
    // accumulates points for the new owner when the battle is won
    _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
    _rankedBattleContract.setNewRound();

    vm.prank(staker);
    _rankedBattleContract.claimNRN();
    // the old owner claims NRNs
    assertEq(_rankedBattleContract.amountClaimed(staker) > 0, true);
    vm.prank(anotherAccount);
    _rankedBattleContract.claimNRN();
    // the new owner claims NRNs
    assertEq(_rankedBattleContract.amountClaimed(anotherAccount) > 0, true);
}

Tools Used

Manual review

Recommended Mitigation Steps

Override the safeTransferFrom(address,address,uint256,bytes memory) function from ERC721 for FighterFarm the same way as safeTransferFrom(address,address,uint256).

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/GameItems.sol#L291-L303

Vulnerability details

Impact

Game items can be transferable or not, and the owner can adjust transferability using adjustTransferability function. The GameItems contract overrides ERC1155 safeTransferFrom function and requires the item to be transferable. However, there is a safeBatchTransferFrom function in ERC1155 contract that GameItems contract does not override, and it allows to transfer non-transferable items.

Proof of Concept

For PoC I added 2 tests to GameItems.t.sol that shows that for non-transferable item safeTransferFrom reverts, but safeBatchTransferFrom successfully transfers the item:

function testSafeTransferFromNonTransferableItem() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.adjustTransferability(0, false);
    _gameItemsContract.mint(0, 1);
    
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
}

function testSafeBatchTransferFromNonTransferableItem() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.adjustTransferability(0, false);
    _gameItemsContract.mint(0, 1);

    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = 0;

    uint256[] memory amounts = new uint256[](1);
    amounts[0] = 1;

    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, tokenIds, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

Tools Used

Manual review

Recommended Mitigation Steps

Override safeBatchTransferFrom function and add a check whether the item is transferable.

Assessed type

Token-Transfer

G-1: Redundant calculations

Description:

In function _addResultPoints initial value of points is 0, and it only changes when stakeAtRisk is 0. However, there is a bunch of calculations with points that are processed even when points are not changed, and those calculations have no reason as the result is 0.

Recommendation:

Move the calculations with points inside the if statement when the points is changed to non-zero value.

G-2: Redundant condition

Description:

The operator or is used in the require statement, the first operand of or is allGameItemAttributes[tokenId].finiteSupply == false, and the second is allGameItemAttributes[tokenId].finiteSupply == true && quantity <= allGameItemAttributes[tokenId].itemsRemaining. The part allGameItemAttributes[tokenId].finiteSupply == true is redundant as this is always true because of short-circuit boolean.

G-3: 'attributeProbabilities' are updated twice

Description:

Constructor of AiArenaHelper updates attributeProbabilities for generation 0 twice - once in function addAttributeProbabilities and another time after the function is called.