AI Arena - favelanky's results

Lines of code

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

Vulnerability details

Impact

The contracts of the Ai Arena ecosystem are designed with a way to change addresses. In case of some emergency, it should be possible to revoke given roles.

Proof of Concept

Neuron.sol contract is a token contract which can give different privileges like MINTER, SPENDER, STAKER roles to other contracts. The AccessControl contract is used to implement role mechanics. However, it requires to set an DEFAULT_ADMIN_ROLE for admin. The contract is not doing this, what leads to impossibility to revoke given roles.

POC (add it to Neuron.t.sol contract):

    function testRevokeRole() public {
        address minter = vm.addr(3);
        _neuronContract.addMinter(minter);
        // this will revert
        _neuronContract.revokeRole(_neuronContract.MINTER_ROLE(), minter);
    }

Running 1 test for test/Neuron.t.sol:NeuronTest
[FAIL. Reason: revert: AccessControl: account 0x7fa9385be102ac3eac297483dd6233d62b3e1496 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000] testRevokeRole() 

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider adding DEFAULT_ADMIN_ROLE to the owner of the contract.

    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);
+       _grantRole(DEFAULT_ADMIN_ROLE, _ownerAddress);
    }
	function transferOwnership(address newOwnerAddress) external {
		require(msg.sender == _ownerAddress);
		_ownerAddress = newOwnerAddress;
		_grantRole(DEFAULT_ADMIN_ROLE, newOwnerAddress);
		revokeRole(DEFAULT_ADMIN_ROLE, _ownerAddress);
	}

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

The user will know the attributes of the NFT before minting and can exploit it to get the wanted ones. He can simply create infinite number of EOAs to bruteforce attributes locally.

Proof of Concept

The _createFighterBase is used to determine some parameters of the NFT. Parameters are determined using pseudo-randomness which allows to any user to know what they will mint. The dna parameter is equal to to uint256(keccak256(abi.encode(msg.sender, fighters.length))) for creating and uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId]))) for rerolling NFT.

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

Tools Used

Manual review

Recommended Mitigation Steps

Do not rely on pseudo-randomness to determine NFT attributes. Consider using a decentralized oracle for the generation of random numbers, such as Chainlinks VRF.

Assessed type

Other

[G-1] Use Merkle Tree

Use Merkle Tree for airdropping tokens. This will save a decent amount of gas and will require only storing the root inside the contract.

    function setupAirdrop(address[] calldata recipients, uint256[] calldata amounts) external {
        require(isAdmin[msg.sender]);
        require(recipients.length == amounts.length);
        uint256 recipientsLength = recipients.length;
        for (uint32 i = 0; i < recipientsLength; i++) {
            _approve(treasuryAddress, recipients[i], amounts[i]);
        }
    }

    /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
    /// @param amount The amount to be claimed
    function claim(uint256 amount) external {
        require(allowance(treasuryAddress, msg.sender) >= amount, "ERC20: claim amount exceeds allowance");
        transferFrom(treasuryAddress, msg.sender, amount);
        emit TokensClaimed(msg.sender, amount);
    }

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

[G-2] Sum at the end

Updating global variables inside a loop is quite expensive. Here it is possible to add the value once.

    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];
            // @gas we can sum up at the end
-           numRoundsClaimed[msg.sender] += 1;
        }
+       numRoundsClaimed[msg.sender] += roundId - lowerBound;
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            // @high time bomb probably, we should transfer from the treasury
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

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

[G-3] Address saved twice

The contract stores the address and instance, which is redundant. Simply use StakeAtRisk(_stakeAtRiskAddress) where needed. The _stakeAtRiskInstance can be removed.

contract RankedBattle {
	...
	address _stakeAtRiskAddress;
	...
	StakeAtRisk _stakeAtRiskInstance;
}

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