AI Arena - yovchev_yoan's results

Lines of code

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

Vulnerability details

Description:

The MegingPool::claimRewards function loops through all the rounds. Then loop through (in a nested loop) the current round's winners and check if the msg.sender is in the winnerAddresses mapping. If he is a winner, he is minted a Figher NFT. The function continues until he claims all his won fighters. The issue is that if the msg.sender has a lot of unclaimed Fighters this function could be very costly or even introduce a denial of service (DoS).

Impact

The gas cost for the winner that has a lot of unclaimed fighers can be unreasonably high, potentially preventing him from claiming his rewards.

Proof of Concept

Place the following test into MergingPool.t.sol

// Testing claimRewards when the _ownerAddress has 6 rounds won and has not claimed his Fighter NFTs.
function testPickWinnerAudit() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // Filling the arrays to claim the NFTs
        string[] memory _modelURIs = new string[](8);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[3] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[4] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[5] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[6] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[7] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](8);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        _modelTypes[3] = "original";
        _modelTypes[4] = "original";
        _modelTypes[5] = "original";
        _modelTypes[6] = "original";
        _modelTypes[7] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](14);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);

        _customAttributes[1][1] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][1] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        _customAttributes[1][1] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][1] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        _customAttributes[1][1] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][1] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        _customAttributes[1][1] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][1] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        _customAttributes[1][1] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][1] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        _customAttributes[1][1] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][1] = uint256(1);
        _customAttributes[2][1] = uint256(80);

        // Picking the winners for 6 rounds. _ownerAddress has won all 6

        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 2 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 3 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 4 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 5 are picked
        _mergingPoolContract.pickWinner(_winners);

        // _ownerAddress decides to claim his rewards after the 6th round
        uint256 gasStart = gasleft();
        vm.prank(_ownerAddress);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        uint256 gasEnd = gasleft();

        // The gas that the _ownerAddress has to pay to call .claimRewards
        uint256 gasUsed = (gasStart - gasEnd);
        console.log("Gas used: ", gasUsed);
    }

Tools Used

Foundry Test

Recommended Mitigation Steps

Consider making the function such that a user can only claim one NFT for a specified roundId. The function will cost significantly less gas and a user can choose for which round he wants to claim his Fighter. This method will remove the need for a nested loop.

Assessed type

DoS

[L-1] In GameItems the instantiateNeuronContract function could lead to revert if not initialized

Description: In GameItems the instantiateNeuronContract function instantiates the NRN contract address. If someone calls mint before instantiation, it will revert.

Impact: The function mint could revert if nrnAddress is not instantiated.

Recommended Mitigation: Recommended mitigations are:

1. Be sure as soon as the contract is deployed the instantiateNeuronContract function is called with the right address.
2. Instantiate the _neuronInstance in the constructor and add a changeNeuronInstance function in order to change it.

[L-2] In GameItems the createGameItem function only emits an event, only if the item is not transferable and not when it is transferable

Description: In GameItems the createGameItem function is responsible for creating a game item, but an event is emitted only when the item is not transferable. In the contract, there is an event called Unlocked, responsible for emitting an event for transferable items. Emit the Unlocked event if transferable param is equal to true.

Impact: Not communicating information on the blockchain if the item created is transferable.

Recommended Mitigation: Emit the Unlocked event if transferable param is equal to true

    if (!transferable) {
        emit Locked(_itemCount);
+   } else {
+       emit Unlocked(_itemCount);
+    }

[L-3] In GameItems the _replenishDailyAllowance function casts uint32 to uint256, which can lead to integer overflow

Description: In GameItems the _replenishDailyAllowance function updates the allowances. In the update of dailyAllowanceReplenishTime there is an unnecessary cast from uint256 to uint32. Parsing from a higher to a lower integer number is not a recommended practice.

Impact: The unsafe cast from uint32 to uint256, can potentially lead to integer overflow

Recommended Mitigation: Remove the casting

- dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + 1 days);
+ dailyAllowanceReplenishTime[msg.sender][tokenId] = block.timestamp + 1 days;

[L-4] In Neuron the functions transferOwnership and adjustAdminAccess do not emit an event after changing storage

Description: In Neuron the functions transferOwnership and adjustAdminAccess change the state of the blockchain, therefore should emit an event.

Impact: Not communicating information on the blockchain if the ownership is transfered, or an address has been granted the role Admin.

Recommended Mitigation: Add these events and emit them after the function is executed:

+ event TransferOwnership(address newOwnerAddress);

function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
        _ownerAddress = newOwnerAddress;
+       emit TransferOwnership(newOwnerAddress);
    }

+ event AdminAccessAdjusted(address adminAddress, bool access);

function adjustAdminAccess(address adminAddress, bool access) external {
        require(msg.sender == _ownerAddress);
        isAdmin[adminAddress] = access;
+       emit AdminAccessAdjusted(adminAddress, access)
    }

[L-5] In MergingPool the functions updateWinnersPerPeriod, transferOwnership, and adjustAdminAccess do not emit an event after changing storage

Description: In MergingPool the functions updateWinnersPerPeriod, transferOwnership, and adjustAdminAccess change the state of the blockchain, therefore should emit an event.

Impact: Not communicating information on the blockchain if the ownership is transfered, updateWinnersPerPeriod is updated, or an address has been granted the role Admin.

Recommended Mitigation: Add these events and emit them after the function is executed:

+ event TransferOwnership(address newOwnerAddress);

function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
        _ownerAddress = newOwnerAddress;
+       emit TransferOwnership(newOwnerAddress);
    }

+ event AdminAccessAdjusted(address adminAddress, bool access);

function adjustAdminAccess(address adminAddress, bool access) external {
        require(msg.sender == _ownerAddress);
        isAdmin[adminAddress] = access;
+       emit AdminAccessAdjusted(adminAddress, access)
    }

+ event WinnerPerPeriodUpdate(uint256 newWinnersPerPeriod);

function updateWinnersPerPeriod(uint256 newWinnersPerPeriodAmount) external {
        require(isAdmin[msg.sender]);
        winnersPerPeriod = newWinnersPerPeriodAmount;
+       emit WinnerPerPeriodUpdate(newWinnersPerPeriodAmount);
    }

[L-6] In FighterFarm the functions instantiateAIArenaHelperContract, instantiateMintpassContract, instantiateNeuronContract, and setMergingPoolAddress could lead to revert if not instantiated

Description: In FighterFarm the functions instantiateAIArenaHelperContract, instantiateMintpassContract, instantiateNeuronContract, and setMergingPoolAddress function instantiate important addresses in the contract. If not called right after the contract deployment, some functions could be unusable.

Impact: Not instantiating these addresses could lead to some functions being unusable.

Recommended Mitigation: Recommended mitigations are:

1. Be sure as soon as the contract is deployed all functions mentioned above should be called with the right addresses.
2. Instantiate the needed addresses in the constructor and add functions in order to change them.

[I-1] In GameItems the function setTokenURI can set the tokenURI to a nonexistent game item

Description: In GameItems the function setTokenURI is responsible for setting the tokenURI to a specific tokenID. However, the function is public and onlyAdmin, meaning that an admin could without intention set a tokenURI to a non-existent tokenId.

Recommended Mitigation: Check if the tokenId is bigger than the _itemCount.

    function setTokenURI(uint256 tokenId, string memory _tokenURI) public {
+       if (tokenId > _itemCount) revert CustomError();
        require(isAdmin[msg.sender]);
        _tokenURIs[tokenId] = _tokenURI;
    }

[I-2] GameItems::createGameItem no check for params itemPrice and dailyAllowance to be greater than 0

Description: In the GameItems contract, the function createGameItem has no check for important parameters - itemPrice and dailyAllowance.

Recommended Mitigation: Add the following check

+ if (itemPrice <= 0 && dailyAllowance <= 0) revert CustomError();

[I-3] FighterOps::viewFighterInfo returns the address that is passed at owner.

Description: In contract FighterOps the function viewFighterInfo returns the address that is passed to it in the parameter address owner. The address is not used for any operations and is just returned.

[I-4] Neuro::contributorAddress initialized but never used

Description: In contract Neuron the contributorAddress is supposed to distribute NRNs to early contributors. But the address is only initialized and never used.

[I-5] VoltageManager::_ownerAddress use the private keyword for better readability

Description: In contract VoltageManager the variable _ownerAddress has not been specified visibility. For better readability use the private keyword.

Recommended Mitigation: Add the private keyword

    /// The address that has owner privileges (initially the contract deployer).
-    address _ownerAddress;
+    address private _ownerAddress;

[G-1] In GameItems the variables name and symbol should be set to constants in order to save gas

Description: In the GameItems contract the name and symbol variables are initialized at the start and never changed. Make them constant in order to save gas.

Recommended Mitigation:

    /// @notice The name of this smart contract.
-   string public name = "AI Arena Game Items";

    /// @notice The symbol for this smart contract.
-   string public symbol = "AGI";

    /// @notice The name of this smart contract.
+   string public constant name = "AI Arena Game Items";

    /// @notice The symbol for this smart contract.
+   string public constant symbol = "AGI";

[G-2] In GameItems save the string return in contractURI to a constant variable

Description: In the GameItems contract the function contractURI returns a string. The more gas-efficient way would be to store that string as a constant variable and then return it.

Proof of Concept: Paste this code in Remix.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

contract TestConstantGas {
string private constant URI = "ipfs://bafybeih3witscmml3padf4qxbea5jh4rl2xp67aydqvqsxmyuzipwtpnii";

    function getConstantPubilc() public pure returns(string memory) {
        return URI;
    }

    function returnString() public pure returns(string memory) {
        return "ipfs://bafybeih3witscmml3padf4qxbea5jh4rl2xp67aydqvqsxmyuzipwtpnii";
    }

}

Recommended Mitigation: Make a constant storage variable to store the string

+ string public constant URI = "ipfs://bafybeih3witscmml3padf4qxbea5jh4rl2xp67aydqvqsxmyuzipwtpnii"

function contractURI() public pure returns (string memory) {
-       return "ipfs://bafybeih3witscmml3padf4qxbea5jh4rl2xp67aydqvqsxmyuzipwtpnii";
+       return URI;
    }

[G-3] In GameItems the transferOwnership, adjustAdminAccess, adjustTransferability and instantiateNeuronContract should use onlyOwner modifier to save gas

Description: In GameItems the transferOwnership, adjustAdminAccess, adjustTransferability and instantiateNeuronContract functions all share a common require statement. Make a modifier with the require in order to save gas.

Recommended Mitigation: Add an onlyOwner modifier to all the functions mentioned above and remove the require statement in the function.

+   modifier onlyOwner() {
+       require(msg.sender == _ownerAddress);
+       _;
+   }

+ function transferOwnership(address newOwnerAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function adjustAdminAccess(address adminAddress, bool access) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function adjustTransferability(uint256 tokenId, bool transferable) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function instantiateNeuronContract(address nrnAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

[G-4] In GameItems the treasuryAddress variable should be immutable to save gas

Description: In GameItems the treasuryAddress is initialized in the constructor and never changed. Make it immutable in order to save gas.

Recommended Mitigation:

    /// @notice The address that recieves funds of purchased game items.
-   address public treasuryAddress;
+   address public immutable treasuryAddress;

[G-5] In Neuron the transferOwnership, addMinter, addStaker, addSpender and adjustAdminAccess should use onlyOwner modifier to save gas

Description: In Neuron the transferOwnership, addMinter, addStaker, addSpender and adjustAdminAccess functions all share a common require statement. Make a modifier with the require in order to save gas.

Recommended Mitigation: Add an onlyOwner modifier to all the functions mentioned above and remove the require statement in the function.

+   modifier onlyOwner() {
+       require(msg.sender == _ownerAddress);
+       _;
+   }

+ function transferOwnership(address newOwnerAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function addMinter(address newMinterAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function addStaker(address newStakerAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function addSpender(address newSpenderAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function adjustAdminAccess(address adminAddress, bool access) external onlyOwner {
-        require(msg.sender == _ownerAddress);

[G-6] In Neuron the setupAirdrop use uint256 instead of uint32 in the for loop to save gas

Description: In Neuron the setupAirdrop uses a uint32 for the for loop. It would be safer and more gas efficient to use uint256 instead.

Proof of Concept: When tested uint256 uses significantly less gas than uint32.

Paste this in Remix

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

contract TestUintForLoop {
    uint256[] addresses;

    function uint32Test() external {
        for (uint32 i = 0; i < 1000; i++) {
            addresses.push(i);
        }
    }

    function uint256Test() external {
        for (uint256 i = 0; i < 1000; i++) {
            addresses.push(i);
        }
    }
}

Recommended Mitigation: Replace uint32 with uint256

- for (uint32 i = 0; i < recipientsLength; i++) {
+ for (uint256 i = 0; i < recipientsLength; i++) {

[G-7] In VoltageManager the transferOwnership and adjustAdminAccess should use onlyOwner modifier to save gas

Description: In VoltageManager the transferOwnership and adjustAdminAccess functions share a common require statement. Make a modifier with the require in order to save gas.

Recommended Mitigation: Add an onlyOwner modifier to the functions mentioned above and remove the require statement in the function.

+   modifier onlyOwner() {
+       require(msg.sender == _ownerAddress);
+       _;
+   }

+ function transferOwnership(address newOwnerAddress) external onlyOwner {
-        require(msg.sender == _ownerAddress);

+ function adjustAdminAccess(address adminAddress, bool access) external onlyOwner {
-        require(msg.sender == _ownerAddress);