AI Arena - josephdara's results

Lines of code

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

Vulnerability details

Impact

Game Items have a transferable property which is used to prevent item transfers between addresses. However this restriction is ineffective as a check is only implemented on the safeTransferFrom() function. Users can still transfer tokens using just the safeBatchTransferFrom() function which has not been restricted. THis function can be used to transfer one or multiple NFTs.

Proof of Concept

    /// @notice Safely transfers an NFT from one address to another.
    /// @dev Added a check to see if the game item is transferable.
    //@audit safeBatchTransferFrom can be used to transfer nfts
    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);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Override the safeBatchTransferFrom() function as well to enable restriction on the batch transfer functions

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

The ownerAddress in the Neuron.sol is used to set the access control on the different roles within the contract by utilizing AccessControl.sol from openzeppelin. However the address cannot manage any of the roles in the contract apart from adding them. Whenever the owner address tries to remove a role from an address, the transaction reverts because the DEFAULT_ADMIN_ROLE is never set and there is no way for the ownerAddress to claim ownership.

Proof of Concept

As seen in the contract, the default admin is not set 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);
    }

Also in the specialized functions made for adding roles and addresses, it can't be used to remove roles. And revokeRole() cannot be used:

    /// @notice Adds a new address to the minter role.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newMinterAddress The address to be added as a minter
    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

    /// @notice Adds a new address to the staker role.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newStakerAddress The address to be added as a staker
    function addStaker(address newStakerAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(STAKER_ROLE, newStakerAddress);
    }

    /// @notice Adds a new address to the spender role.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newSpenderAddress The address to be added as a spender
    function addSpender(address newSpenderAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(SPENDER_ROLE, newSpenderAddress);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Set the DEFAULT_ADMIN_ROLE in the constructor.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The claimRewards() is used to claim rewards by comparing winner addresses from the beginning of the contract to it's current round. This is done via a loop&compare method. However the game rounds will reach a max limit where loops will exceed the gas limit for a transaction. THis will prevent new winners who have to loop from numRoundsClaimed[msg.sender] = 0 up to current roundds of a 100 or 1000.

Proof of Concept

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        //@audit issue new winners will not be able to claim after multiple rounds  
        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;
                }
            }
        }

For all users, the numRoundsClaimed will start from zero. At the point where the loop gets to large, the transactions will fail for all new claimers,causing lose of funds

Tools Used

Manual Review

Recommended Mitigation Steps

Enable users claim for a specific round, Then maintain a mapping signifying that they have claimed

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L185-L188

Vulnerability details

Impact

The game items contract allows different specialized roles including the allowedBurningAddresses which is set using the setAllowedBurningAddresses function. However unlike other roles in this contract as well as other contracts, the addresses allowed for burning are non removeable. This is particularly an issue as it grants non revokeable access to users tokens.

Proof of Concept

    function setAllowedBurningAddresses(address newBurningAddress) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = true;
    }

As seen above, the boolean value to be set for every new address is hardcoded to zero, therefore admins can never remove access for burning addresses.

Tools Used

Manual Review

Recommended Mitigation Steps

Add an extra parameter to the function to allow passing in a boolean variable. This will enable setting addresses to true or false.

    function setAllowedBurningAddresses(address newBurningAddress, bool allowed) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = allowed;
    }

Assessed type

Access Control