AI Arena - Greed's results

Lines of code

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

Vulnerability details

Impact

There are some tokens representing GameItems that must not be transfered. The issue allows a user to bypass this restriction and be able to transfer these tokens.

Proof of concept

In the GameItems.sol contract, in order to create a new item, an admin can call the createGameItem() function.

The transferability of these tokens can be set using the transferable parameter and is enforced by the safeTransferFrom() function that has been overridden.

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

function createGameItem(
    string memory name_,
    string memory tokenURI,
    bool finiteSupply,
    bool transferable,
    uint256 itemsRemaining,
    uint256 itemPrice,
    uint16 dailyAllowance
) 

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

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

However this restriction can be bypassed using the safeBatchTransferFrom from ERC1155, giving the ability to transfer all GameItems (even those that should not be)

Tools used

Manual analysis

Recommended mitigation steps

Override the safeBatchTransferFrom() function and make sure all the tokens given in the array parameter are not transferable

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

If a user has minted a fighter after the 255th, he won't ever be able to reRoll() it.

This completely breaks the dedicated functionality of the protocol

Proof of concept

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

The FighterFarm::reRoll() function allows a user to reroll his fighter to change his attributes.

Here is the function declaration :

function reRoll(uint8 tokenId, uint8 fighterType) public

The tokenId represents the fighter to reroll.

As you can see, this parameter is a uint8 meaning it can only go up to 255.

If a user owns the NFT number 260 for example, he won't be able to reroll it and utilize the functionality.

Tools used

Manual analysis

Recommended mitigation steps

Change the type of the tokenId parameter of the reRoll() function from uint8 to uint256 like such :

function reRoll(uint256 tokenId, uint8 fighterType) public

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L93-L96 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L101-L104 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L109-L112

Vulnerability details

Impact

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

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

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

Once a role is given to an address, it will persist forever with no way to revoke it.

If either RankedBattle.sol, GameItems.sol, FighterFarm.sol is set to a new address (after an upgrade for example), their old implementation will still possess the roles meaning they are still able to perform privileged actions.

These privileged actions include minting, approving, staking NRN tokens

In some scenarios, these roles could also be temporarly given to an address but there would be no way to remove their privileges

Proof of concept

In order to revoke a role given to an address, the admin of this specific role has to call the revokeRole(bytes32 role, address account) function from Openzeppelin AccessControl.sol

function revokeRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
    _revokeRole(role, account);
}

This function checks that the msg.sender has the required privileges to manage the role (that he is admin of this role) using the modifier onlyRole(getRoleAdmin(role))

function getRoleAdmin(bytes32 role) public view virtual override returns (bytes32) {
    return _roles[role].adminRole;
}

// ...

modifier onlyRole(bytes32 role) {
    _checkRole(role);
    _;
}

// ...

function _checkRole(bytes32 role) internal view virtual {
    _checkRole(role, _msgSender());
}

// ...

function _checkRole(bytes32 role, address account) internal view virtual {
    if (!hasRole(role, account)) {
        revert(
            string(
                abi.encodePacked(
                    "AccessControl: account ",
                    Strings.toHexString(uint160(account), 20),
                    " is missing role ",
                    Strings.toHexString(uint256(role), 32)
                )
            )
        );
    }
}

If this condition is not met, the transaction will revert which, in the case of the Neuron.sol contract, always will because neither the admin role nor the admin are set.

Tools used

Manual analysis

Recommended mitigation steps

First, add the admin roles in the storage

bytes32 public constant MINTER_ADMIN_ROLE = keccak256("MINTER_ADMIN_ROLE");
bytes32 public constant SPENDER_ADMIN_ROLE = keccak256("SPENDER_ADMIN_ROLE");
bytes32 public constant STAKER_ADMIN_ROLE = keccak256("STAKER_ADMIN_ROLE");

Second, set role admin in the constructor

_setRoleAdmin(MINTER_ROLE, MINTER_ADMIN_ROLE);
_setRoleAdmin(SPENDER_ROLE, SPENDER_ADMIN_ROLE);
_setRoleAdmin(STAKER_ROLE, STAKER_ADMIN_ROLE);

Finally, implement functions to set the role admin

function assignMinterAdmin(address minterAdmin) external {
    require(msg.sender == _ownerAddress);
    _setupRole(MINTER_ADMIN_ROLE, minterAdmin);
}

function assignSpenderAdmin(address spenderAdmin) external {
    require(msg.sender == _ownerAddress);
    _setupRole(SPENDER_ADMIN_ROLE, spenderAdmin);
}

function assignStakerAdmin(address stakerAdmin) external {
    require(msg.sender == _ownerAddress);
    _setupRole(STAKER_ADMIN_ROLE, stakerAdmin);
}

Now the role admin can use the following functions

revokeRole(MINTER_ROLE, oldMinter);
revokeRole(SPENDER_ROLE, oldSpender);
revokeRole(STAKER_ROLE, oldStaker);

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Users can lose the NRN tokens they earned as rewards

Proof of concept

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

When users win a match, they may earn points which are stored in the accumulatedPointsPerAddress mapping.

These points can later on be used to claim NRN tokens using the RankedBattle::claimNRN() function.

This function goes through every round that the caller didn't claim rewards, in a for loop.

If the user didn't claim his rewards for a very long time and kept accumulating them, there is a risk that the loop consumes all the gas available and makes the transaction revert.

As a result, the user won't be able to claim his rewards ever.

Tools used

Manual analysis

Recommended mitigation steps

Add a new function to allow users to claim their rewards for a particular round.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

User can predict the attributes of their fighters and get an unfair advantage compared to other players.

He could reroll them so they all have a diamond attribute, which is particularly rare, or get the best element and weight.

Proof of concept

The reRoll() function is used to modify the attributes of a particular fighter.

These attributes are determined using the internal _createFighterBase() function which takes the dna parameter to generate attributes.

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

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        numRerolls[tokenId] += 1;
        uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
        (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
        fighters[tokenId].element = element;
        fighters[tokenId].weight = weight;
        fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            fighters[tokenId].iconsType,
            fighters[tokenId].dendroidBool
        );
        _tokenURIs[tokenId] = "";
    }
} 

Since the dna parameter is computed based upon the value of msg.sender, in case it changes, the returned value will be completely different.

A user can simulate the reRoll() function and if the outcome is not the one he expected, he can transfer his fighter to another address he controls and repeat the process until the rerolled fighter is the one he wants (a fighter with diamond attribute for example).

Tools used

Manual analysis

Recommended mitigation steps

When performing RNG, a smart contract should rely on an external party to make the number generated unpredictable.

Use an oracle to generate the dna number that is used to derivate the fighter's attributes

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

The restrictions regarding the mint() function in the GameItems.sol contract can by bypassed, allowing a user to own more tokens than the allowanceRemaining and the dailyAllowanceReplenishTime should enforce.

Proof of concept

The mint() function makes sure the caller can only claim a defined amount of tokens daily using the dailyAllowanceReplenishTime and the allowanceRemaining mappings

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

require(
    dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp || 
    quantity <= allowanceRemaining[msg.sender][tokenId]
);

However, this restriction can be bypassed, allowing a user to obtain more tokens than the daily amount allowed by mint() using 1 wallet and transfering the token to another wallet.

In order to exploit the issue, a user can do the following (infinitely) :

Assume the user has a main wallet

1. Create a new wallet
2. Using the new wallet, call the mint() function with the maximum of quantity allowed by allGameItemAttributes
3. Use the safeTransferFrom() function to transfer all the tokens minted to the main wallet
4. Repeat as much as wanted

Tools used

Manual analysis

Recommended mitigation steps

In the safeTransferFrom() overridden function, add more checks regarding the receiver's allowanceRemaining and dailyAllowanceReplenishTime mappings

Assessed type

Invalid Validation