AI Arena - _eperezok's results

Lines of code

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

Vulnerability details

Summary

Fighter NFTs are always transferable, even if they are staked or if the recipient has reached the maximum fighters allowed.

Details

FighterFarm::_ableToTransfer defines the custom logic to determine whether a token is transferable or not:

function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
    return (
      _isApprovedOrOwner(msg.sender, tokenId) &&
      balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
      !fighterStaked[tokenId]
    );
}

To enforce this behavior, the transferFrom(address,address,uint256) and safeTransferFrom(address,address,uint256) functions from the base ERC721 contract are overriden.

function transferFrom(
    address from, 
    address to, 
    uint256 tokenId
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _transfer(from, to, tokenId);
}

function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId
) 
    public 
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _safeTransfer(from, to, tokenId, "");
}

FighterFarm.sol#L333-L365

However, the safeTransferFrom(address,address,uint256,bytes memory) function inherited from the base ERC721 contract is not overriden, and hence it can be used to freely transfer tokens without the custom logic described above.

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);
}

Impact

Players will be able to have more fighters than the maximum allowed, and transfer their fighters even when they are staked.

Tools Used

Manual review.

Recommended Mitigation Steps

Either override the safeTransferFrom(address,address,uint256,bytes memory) function from the base ERC721 contract as well, or even better, override just the _beforeTokenTransfer hook to apply the custom transfer logic in all kind of transfers at the same time (careful with mints and burns).

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Summary

Game items are always transferable, even if the owner set the transferable attribute to false.

Details

Each GameItem has a boolean transferable attribute, which can be modified by the contract owner.

struct GameItemAttributes {
    string name;
    bool finiteSupply;
    bool transferable;
    uint256 itemsRemaining;
    uint256 itemPrice;
    uint256 dailyAllowance;
}

To only allow token transfers for tokenId's whose transferable attribute is set to true, the safeTransferFrom function inherited from the base ERC1155 contract is overriden:

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);
}

GameItems.sol#L289-L303

However, the safeBatchTransferFrom function inherited from the base ERC1155 contract is not overriden, and hence it can be used to freely transfer tokens without the custom logic described above.

function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory amounts,
    bytes memory data
) public virtual override {
    require(
        from == _msgSender() || isApprovedForAll(from, _msgSender()),
        "ERC1155: caller is not token owner nor approved"
    );
    _safeBatchTransferFrom(from, to, ids, amounts, data);
}

Impact

Players will be able to transfer any game item at will, despite the owner of the contract disallowing it.

Tools Used

Manual review.

Recommended Mitigation Steps

Either override the safeBatchTransferFrom function from the base ERC1155 contract as well, or even better, override just the _beforeTokenTransfer hook to apply the custom transfer logic in all kind of transfers at the same time (careful with mints and burns).

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Summary

FighterFarm::_createFighterBase will be unusable after the first generation of Fighters.

Details

FighterFarm::_createFighterBase computes the element, weight and newDna of a Fighter.

function _createFighterBase(
    uint256 dna, 
    uint8 fighterType
) 
    private 
    view 
    returns (uint256, uint256, uint256) 
{
    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
}

To compute the element, it uses the numElements mapping to get the amount of elements for the current generation of Fighters.

mapping(uint8 => uint8) public numElements;

Initially, the number of elements for generation 0 is set in the constructor:

constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
    ERC721("AI Arena Fighter", "FTR")
{
    _ownerAddress = ownerAddress;
    _delegatedAddress = delegatedAddress;
    treasuryAddress = treasuryAddress_;
    numElements[0] = 3;
}

However, there isn’t any other function in the contract to set the number of elements for any other generation, and thus it will be 0.

Back to the _createFighterBase function, when the current generation of Fighters is greater than 0, numElements[generation[fighterType]] will be 0, and a division by 0 will happen when computing the modulo operation:

uint256 element = dna % numElements[generation[fighterType]];

This will make the transaction revert, rendering the _createFighterBase function unusable.

Impact

After the first generation of Fighters (ie. from generation 1 onwards), some of the core functions of the FighterFarm contract will be unusable. The affected functions will be:

• FighterFarm::claimFighters
• FighterFarm::redeemMintPass
• FighterFarm::mintFromMergingPool
• FighterFarm::reRoll

Tools Used

Manual review.

Recommended Mitigation Steps

Create a setter function for the numElements mapping.

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L90-L112

Vulnerability details

Summary

Accounts that have been granted a role in the Neuron token contract, will keep that role forever. This can be an issue if those accounts ever get compromised, either through a vulnerability if they are contract accounts, or through a private key leak if they are EOAs.

Details

The Neuron token uses OpenZeppelin’s AccessControl contract to manage roles.

By default, the admin role for all roles is DEFAULT_ADMIN_ROLE, which means that only accounts with this role will be able to grant or revoke other roles.

However, the DEFAULT_ADMIN_ROLE is never assigned, and instead the contract defines custom functions to grant roles.

function addMinter(address newMinterAddress) external {
    require(msg.sender == _ownerAddress);
    _setupRole(MINTER_ROLE, newMinterAddress);
}

function addStaker(address newStakerAddress) external {
    require(msg.sender == _ownerAddress);
    _setupRole(STAKER_ROLE, newStakerAddress);
}

function addSpender(address newSpenderAddress) external {
    require(msg.sender == _ownerAddress);
    _setupRole(SPENDER_ROLE, newSpenderAddress);
}

Since there aren’t any wrappers around revoking roles, this functionality will be unavailable.

Impact

If an account with a certain role gets compromised, there will be no way of revoking its role, which is a risk for the protocol.

Tools Used

Manual review.

Recommended Mitigation Steps

Assign the DEFAULT_ADMIN_ROLE to the ownerAddress in the constructor of the Neuron contract.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Summary

Users can pass arbitrary customAttributes to MergingPool::claimRewards, which are then used to mint Fighters through FighterFarm::mintFromMergingPool. This results in Fighters having element and weight attributes outside of the expected range.

Details

Fighters are minted using the FighterFarm::_createNewFighter function.

function _createNewFighter(
    address to, 
    uint256 dna, 
    string memory modelHash,
    string memory modelType, 
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
) 
    private 
{  
    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
    uint256 element; 
    uint256 weight;
    uint256 newDna;
    if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else {
        element = customAttributes[0];
        weight = customAttributes[1];
        newDna = dna;
    }
    uint256 newId = fighters.length;
    // ....
}

We can see that if customAttributes[0] == 100, the element and weight attributes of the fighter are generated by the _createFighterBase function; otherwise they are directly taken from the customAttributes tuple.

function _createFighterBase(
    uint256 dna, 
    uint8 fighterType
) 
    private 
    view 
    returns (uint256, uint256, uint256) 
{
    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
}

The sponsor team confirmed that the weight attribute should always be a value between 65 and 95, which is what we see implemented in the code above. Moreover, the range for the element attribute depends on the current generation of Fighters.

To enforce these restrictions on the attributes, all functions that mint Fighters in the FighterFarm contract set the customAttributes[0] to 100, except for mintFromMergingPool.

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to, 
        uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

This function receives arbitrary customAttributes that come from MergingPool::claimRewards, which is an external function that any player can call when they win a reward.

function claimRewards(
    string[] calldata modelURIs, 
    string[] calldata modelTypes,
    uint256[2][] calldata customAttributes
) 
    external 
{
    uint256 winnersLength;
    uint32 claimIndex = 0;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        numRoundsClaimed[msg.sender] += 1;
        winnersLength = winnerAddresses[currentRound].length;
        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {
                _fighterFarmInstance.mintFromMergingPool(
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
		    // @audit-issue -> arbitrary values sent
                    customAttributes[claimIndex]
                );
                claimIndex += 1;
            }
        }
    }
    // ...
}

If the winner calls claimRewards with customAttributes[0] != 100, then unexpected values will be set as the weight and element attributes of the new Fighter.

Impact

Players can mint Fighters with arbitrary weight and element attributes, possibly outside of the expected range, which could break some of the core functionalities of the game.

Tools Used

Manual review.

Recommended Mitigation Steps

Validate the values present in the customAttributes parameter of the MergingPool::claimRewards function.

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

Summary

Claiming rewards in the MergingPool will become increasingly costly over rounds, to the point it might be too expensive for users to even want to claim them.

Details

When a user calls MergingPool::claimRewards, the contract has to iterate over all the rounds since the last claim of the user. For users that have never claimed rewards before, this will mean iterating from the very first round to the last one.

function claimRewards(
    string[] calldata modelURIs, 
    string[] calldata modelTypes,
    uint256[2][] calldata customAttributes
) 
    external 
{
    uint256 winnersLength;
    uint32 claimIndex = 0;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        numRoundsClaimed[msg.sender] += 1;
        winnersLength = winnerAddresses[currentRound].length;
        for (uint32 j = 0; j < winnersLength; j++) {
            if (msg.sender == winnerAddresses[currentRound][j]) {
                _fighterFarmInstance.mintFromMergingPool(
                    msg.sender,
                    modelURIs[claimIndex],
                    modelTypes[claimIndex],
                    customAttributes[claimIndex]
                );
                claimIndex += 1;
            }
        }
    }
    // ...
}

Note that for each iteration, there are several storage read/writes, which contributes to the very high cost of calling this function.

Impact

Claiming rewards in the MergingPool contract will unnecessarily imply a significant monetary cost for users over rounds, which are expected to have a 1-week duration.

Over time, the cost of claiming the reward might even be greater than the value of the reward itself, so users will stop using this functionality of the protocol.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider storing the unclaimed rewards for each user in a more efficient way. For instance, a mapping(address user => uint256[] roundIds) could be used, where all the rounds in which the user won a reward are saved and they only have to iterate over those rounds, removing them from the list as the rewards are claimed.

Assessed type

Loop

Lines of code

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

Vulnerability details

Summary

Claiming NRN in RankedBattle for the first time will be increasingly costly for new players over rounds. This will make them incur large gas fees unnecessarily, which is unfair.

Details

When a user calls RankedBattle::claimNRN, the contract has to iterate over all the rounds since the last claim of the user. For users that have never claimed NRN before, this will mean iterating from the very first round to the last one.

function claimNRN() external {
    require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
    uint256 claimableNRN = 0;
    uint256 nrnDistribution;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
    for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
        nrnDistribution = getNrnDistribution(currentRound);
        claimableNRN += (
            accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
        ) / totalAccumulatedPoints[currentRound];
        numRoundsClaimed[msg.sender] += 1;
    }
    if (claimableNRN > 0) {
        amountClaimed[msg.sender] += claimableNRN;
        _neuronInstance.mint(msg.sender, claimableNRN);
        emit Claimed(msg.sender, claimableNRN);
    }
}

Note that for each iteration, there are several storage read/writes, which contributes to the very high cost of calling this function for the first time.

Impact

Claiming NRN in the RankedBattle contract will imply a significant monetary cost for users over rounds, which are expected to have a 1-week duration. This will particularly affect new players, who will need to iterate over all the previous rounds unnecessarily, since they don’t have any NRN available to claim for those rounds.

Over time, the cost of claiming NRN for the first time might be so high that new players will be discouraged from playing the game.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider adding a fromRound parameter to RankedBattle::claimNRN, so that users can start claiming NRN from a given round (where they actually started playing the game) instead of from the game launch. Ideally, keep track of the rounds a user has NRN available to claim either on-chain or off-chain, so that they only need to iterate over those rounds.

Assessed type

Loop