AI Arena - zaevlad's results

Lines of code

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

Vulnerability details

Impact

Missing DEFAULT_ADMIN_ROLE in the Neuron contract will block admins to manage roles.

Proof of Concept

Neuron contract inherits from AccessControl contract to implement role access for some core functions. However a constructor is missing DEFAULT_ADMIN_ROLE setup for a contract owner and other roles.

DEFAULT_ADMIN_ROLE is used to manage other roles in the contract and revoke access from users in case of any problems.

Right now it creates two problems:

1. Wrong address can not be revoked from the contract as there is no admin for Staker, Minter and Spender roles;

2. If the admin will be changed the new admin will not be granted with an admin role for the contract;

So the Access control system for the Neuron contract is broken.

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding the next lines in the constructor:

    constructor(address ownerAddress, address treasuryAddress_, address contributorAddress)
        ERC20("Neuron", "NRN")
    {
        _ownerAddress = ownerAddress;
        treasuryAddress = treasuryAddress_;
        isAdmin[_ownerAddress] = true;
        _mint(treasuryAddress, INITIAL_TREASURY_MINT);
        _mint(contributorAddress, INITIAL_CONTRIBUTOR_MINT); 
+        _setupRole(DEFAULT_ADMIN_ROLE,ownerAddress);
+        _setRoleAdmin(MINTER_ROLE,DEFAULT_ADMIN_ROLE);
+        _setRoleAdmin(SPENDER_ROLE,DEFAULT_ADMIN_ROLE);
+        _setRoleAdmin(STAKER_ROLE,DEFAULT_ADMIN_ROLE);
    }

Assessed type

Governance

Lines of code

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

Vulnerability details

Impact

Users will not be able to cliam rewards from Merging Pool after some game rounds passed.

Proof of Concept

After some rounds of the game users are able to claim their rewards in Merging Pool contract by calling claimRewards:

    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;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

The problem is it tryes to send rewards for multiple rounds at once.

Imagine the following situation.

To play a game any user must have at least one NFT Fighter. As we can see in Fighter Farm contract that quantity of NFTs for each address is limited to 10:

uint8 public constant MAX_FIGHTERS_ALLOWED = 10;

So if user with 1 NFT will have rewards for 9 rounds he will not be able to redeem it.

The problem can be severe if user has more NFTs: the more NFTs he has the less rewards needed to block redeeming process.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Consider providing a function for users to let them redeem NFTs for a specific round by themselves.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Users can exceed daily allowance for game items by transfering them from another game account.

Proof of Concept

There is a special daily allowance system for game items to keep the balance and avoid unfair situations between players.

Users can mint some game items with some limitaions on a daily basis:

    function mint(uint256 tokenId, uint256 quantity) external {
        require(tokenId < _itemCount);
        uint256 price = allGameItemAttributes[tokenId].itemPrice * quantity;
        require(_neuronInstance.balanceOf(msg.sender) >= price, "Not enough NRN for purchase");
        require(
            allGameItemAttributes[tokenId].finiteSupply == false || 
            (
                allGameItemAttributes[tokenId].finiteSupply == true && 
                quantity <= allGameItemAttributes[tokenId].itemsRemaining
            )
        );
@>        require(
            dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp || 
            quantity <= allowanceRemaining[msg.sender][tokenId]
        );

        _neuronInstance.approveSpender(msg.sender, price);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, price);
        if (success) {
            if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
                _replenishDailyAllowance(tokenId);
            }
            allowanceRemaining[msg.sender][tokenId] -= quantity;
            if (allGameItemAttributes[tokenId].finiteSupply) {
                allGameItemAttributes[tokenId].itemsRemaining -= quantity;
            }
            _mint(msg.sender, tokenId, quantity, bytes("random"));
            emit BoughtItem(msg.sender, tokenId, quantity);
        }
    }

As you can see for each game item can be specified its own limit that is took into consideration while minting.

However user can trasfer NFT from one account to another without any limitations. For example, here is overriden function:

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

And it only checks if the game item can be trasfered, but does not account any limitaions.

So a malicious user can double his items with a help of a second game account.

Tools Used

Manual review

Recommended Mitigation Steps

Consider providing a limitaion check for a game item during it transfer. A good example can be transfer checks in FighterFarm contract:

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

Assessed type

Context