AI Arena - merlinboii's results

Lines of code

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

Vulnerability details

Description

When the players stake NRN for their fighters, those fighters are flagged as being stake and unable to transfer.

However, the players can transfer fighters while they are staked by using overload ERC721::safeTransferFrom(), bypassing this verification shown in the FighterFarm::transferFrom() and FighterFarm::safeTransferFrom().

• The FighterFarm::_ableToTransfer()

File: FighterFarm.sol

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

• The overload ERC721::safeTransferFrom()@v4.7.0

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

Impact

Bypassing the fighter transfer verification that is designed to prevent the transfer of fighters when they are being staked, leading to conflicts in game mechanics and enabling the staked fighter to be traded in the secondary market.

Proof of Concept

Setup

• Put the snippet below into the protocol test suite: test/FighterFarm.t.sol
• Run test: forge test --mt testTransferringFighterWhileStaked_SUCCESS -vvv

The full coded PoC shown below:

<details> <summary>Details PoC</summary>

    function testTransferringFighterWhileStaked_SUCCESS() public {
        address alice = makeAddr("Alice");
        address bob = makeAddr("Bob");
        console.log("=== Addresses ===");
        console.log("Alice: %s", alice);
        console.log("Bob: %s\n", bob);

        _mintFromMergingPool(alice);
        _fighterFarmContract.addStaker(_ownerAddress);

        // flag fighter as being staked
        vm.prank(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        console.log("Staking status of Fighter ID: 0 : %s\n", _fighterFarmContract.fighterStaked(0));
        
        vm.prank(alice);
        // using `ER721::safeTransferFrom(address,address,uint256,bytes)`
        _fighterFarmContract.safeTransferFrom(alice, bob, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0) != alice, true);
        assertEq(_fighterFarmContract.ownerOf(0), bob);

        console.log("=== Alice transfer the staking Fighter:0 to Bob ===");
        console.log("Owner of Fighter:0 : %s (%s)", _fighterFarmContract.ownerOf(0), _fighterFarmContract.ownerOf(0) == bob? "Bob" : (_fighterFarmContract.ownerOf(0) == alice? "Alice" : "None"));
    }

</details>

Results of running the test:

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testTransferringFighterWhileStaked_SUCCESS() (gas: 511035)
Logs:
  === Addresses ===
  Alice: 0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea
  Bob: 0x4dBa461cA9342F4A6Cf942aBd7eacf8AE259108C

  Staking status of Fighter ID: 0 : true

  === Alice transfer the staking Fighter:0 to Bob ===
  Owner of Fighter:0 : 0x4dBa461cA9342F4A6Cf942aBd7eacf8AE259108C (Bob)

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.99ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Recommend applying the verification into the override FighterFarm::_beforeTokenTransfer() to verify transferable fighters in every fighter transfer.

Applying the recommended code below will execute the hook and verify transferable fighters in transferFrom(), safeTransferFrom(), and overloaded safeTransferFrom() executions.

Therefore, applying the recommended code can remove the need for overriding FighterFarm.transferFrom() and FighterFarm.safeTransferFrom().

File: FighterFarm.sol

    function _beforeTokenTransfer(address from, address to, uint256 tokenId)
        internal
        override(ERC721, ERC721Enumerable)
    {   
+        if(from != address(0) && to != address(0)){
+            require(_ableToTransfer(tokenId, to), "unable to transfer");
+        }
        super._beforeTokenTransfer(from, to, tokenId);
    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Description

the players can transfer non-transferable game items using ERC1155::safeBatchTransferFrom(), bypassing this verification shown in the GameItems::safeTransferFrom().

require(allGameItemAttributes[tokenId].transferable);

The ERC1155::safeBatchTransferFrom()@v4.7.0

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Impact

The ability to bypass the verification of transferable game items, allows players to possess a specific quantity of items to progress through the game.

For example, the protocol team might choose to temporarily mark battery items as non-transferable to prevent the dumping of batteries from alternate accounts for replenishing and then transferring the items to the main player account for initiating battles.

Proof of Concept

Setup

• Put the snippet below into the protocol test suite: test/GameItems.t.sol
• Run test: forge test --mt testBypassTransferGameItems -vvv

The full coded PoC shown below:

<details> <summary>Details PoC</summary>

    function testBypassTransferGameItems() public {
        (, ,bool transferable, , ,) = _gameItemsContract.allGameItemAttributes(0);
        console.log("Initial Battery item transferable flag: %s\n", transferable);

        //update battery items to be non-transferable
        _gameItemsContract.adjustTransferability(0, false);
        console.log("=== Admin adjust battery to non-transferable  ===");
         (, ,transferable, , ,) = _gameItemsContract.allGameItemAttributes(0);
        console.log("Battery item transferable flag: %s\n", transferable);
        uint256 dailyAllowance = 10;
        
        address alice = makeAddr("Alice");
        address bob = makeAddr("Bob");
        _fundUserWith4kNeuronByTreasury(alice);

        // mint the battery items
        vm.startPrank(alice);
        _gameItemsContract.mint(0, dailyAllowance);
        assertEq(_gameItemsContract.balanceOf(alice, 0), dailyAllowance);
        console.log("=== Alice mint Battery  ===");
        console.log("Alice Battery items balance: %s", _gameItemsContract.balanceOf(alice, 0));
        console.log("Bob Battery items balance: %s\n", _gameItemsContract.balanceOf(bob, 0));

        // alice attemp to transfer to Bob
        uint256 transferAmount = _gameItemsContract.balanceOf(alice, 0);

            // Fail on `safeTransferFrom`
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(alice, bob, 0, transferAmount, "");

            // Success on `safeBatchTransferFrom`
        uint256[] memory craftIds = new uint256[](1);
        craftIds[0] = 0;
        uint256[] memory craftAmounts = new uint256[](1);
        craftAmounts[0] = transferAmount;

        _gameItemsContract.safeBatchTransferFrom(alice, bob, craftIds, craftAmounts, "");
        vm.stopPrank();

        console.log("=== Alice transfer Battery to Bob via safeBatchTransferFrom() ===");
        console.log("Alice Battery items balance: %s", _gameItemsContract.balanceOf(alice, 0));
        console.log("Bob Battery items balance: %s\n", _gameItemsContract.balanceOf(bob, 0));

        console.log("=== Transfer of non-tranferable items SUCCESS ===");
        assertEq(_gameItemsContract.balanceOf(bob, 0), transferAmount);
    }

</details>

Results of running the test:

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testBypassTransferGameItems() (gas: 197141)
Logs:
  Initial Battery item transferable flag: true

  === Admin adjust battery to non-transferable  ===
  Battery item transferable flag: false

  === Alice mint Battery  ===
  Alice Battery items balance: 10
  Bob Battery items balance: 0

  === Alice transfer Battery to Bob via safeBatchTransferFrom() ===
  Alice Battery items balance: 0
  Bob Battery items balance: 10

  === Transfer of non-tranferable items SUCCESS ===

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.58ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Recommend overriding the ERC1155::_beforeTokenTransfer() to verify transferable items in every item transfer.

Overriding ERC1155::_beforeTokenTransfer() will execute the hook and verify transferable items in both safeTransferFrom() and safeBatchTransferFrom() executions.

Therefore, applying the recommended code can remove the need for overriding GameItems.safeTransferFrom().

File: GameItems.sol

+    function _beforeTokenTransfer(
+        address operator,
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) internal override(ERC1155) {
+        if (from != address(0) && to != address(0)){
+            for (uint256 i = 0; i < ids.length; ++i) {
+                require(allGameItemAttributes[ids[i]].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

Description

The FighterFarm::reRoll() allows the player to specify the fighterType to the FighterFarm::reRoll(), which can lead to predictable fighter rerolled traits and ability to reroll fighters more than the maximum allowance for each fighterType.

File: FighterFarm.sol

    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] = "";
        }
    } 

File: FighterFarm.sol

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

File: AiArenaHelper.sol

    function createPhysicalAttributes(
        uint256 dna, 
        uint8 generation, 
        uint8 iconsType, 
        bool dendroidBool
    ) 
        external 
        view 
        returns (FighterOps.FighterPhysicalAttributes memory) 
    {
        if (dendroidBool) {
            return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99);
        } else {
            uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length);

            uint256 attributesLength = attributes.length;
            for (uint8 i = 0; i < attributesLength; i++) {
                if (
                  i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
                  i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
                  i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
                ) {
                    finalAttributeProbabilityIndexes[i] = 50;
                } else {
-->                 uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
                    finalAttributeProbabilityIndexes[i] = attributeIndex;
                }
            }
            return FighterOps.FighterPhysicalAttributes(
                finalAttributeProbabilityIndexes[0],
                finalAttributeProbabilityIndexes[1],
                finalAttributeProbabilityIndexes[2],
                finalAttributeProbabilityIndexes[3],
                finalAttributeProbabilityIndexes[4],
                finalAttributeProbabilityIndexes[5]
            );
        }
    }

Impact

Impact of the Issue:

• Predictable rerolled fighter attributes.

• Ability to reroll fighters more than the maximum allowance for each fighterType.

Proof of Concept

Let's assume that

• The current generation of each fighter type is 0.
• Alice has 1 champion fighter
  • id: 0
  • fighterType:0
  • dendroidBool: false.
• Global FighterFarm::maxRerollsAllowed[fighterType:0]: 3.

Alice rerolls her fighter by passing the fighterType as 1 (dendroid type), which is inconsistent with her current fighter type.

% FighterFarm.reRoll(tokenId: 0, fighterType: 1) 

The sequence and result of execution will be as follows:

1. The maxRerollsAllowed[fighterType: 1] will be checked instead of maxRerollsAllowed[fighterType: 0] which is the actual type of the given fighter.

File: FighterFarm.sol

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
-->     require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

    // SNIPPED

    }

2. The newDna will be set to 1

uint256 newDna = fighterType(1) == 0 ? dna : uint256(fighterType(1));
uint256 newDna = uint256(fighterType(1))

File: FighterFarm.sol

    function reRoll(uint8 tokenId, uint8 fighterType) public {

        // SNIPPED

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

            // SNIPPED

        }
    } 

File: FighterFarm.sol

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

3. This results in the new rerolled fighter for her fighter always being represented as:

FighterOps.FighterPhysicalAttributes(1, 1, 1, 1, 1, 1)

as calling the AiArenaHelper.createPhysicalAttributes() as follows:

fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna: 1, 
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );

File: FighterFarm.sol

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        
        // SNIPPED

        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, // 1
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    } 

File: AiArenaHelper.sol

    function createPhysicalAttributes(
        uint256 dna, 
        uint8 generation, 
        uint8 iconsType, 
        bool dendroidBool
    ) 
        external 
        view 
        returns (FighterOps.FighterPhysicalAttributes memory) 
    {
        if (dendroidBool) {
            return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99);
        } else {
            uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length);

            uint256 attributesLength = attributes.length;
            for (uint8 i = 0; i < attributesLength; i++) {
                if (
                  i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
                  i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
                  i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
                ) {
                    finalAttributeProbabilityIndexes[i] = 50;
                } else {
-->                 uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
                    // uint256 rarityRank = (1 / attributeToDnaDivisor[attributes[i]]) % 100;
                    // uint256 rarityRank = 0
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
                    finalAttributeProbabilityIndexes[i] = attributeIndex;
                }
            }
            return FighterOps.FighterPhysicalAttributes(
                finalAttributeProbabilityIndexes[0],
                finalAttributeProbabilityIndexes[1],
                finalAttributeProbabilityIndexes[2],
                finalAttributeProbabilityIndexes[3],
                finalAttributeProbabilityIndexes[4],
                finalAttributeProbabilityIndexes[5]
            );
        }
    }

    // SNIPPED

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
        public 
        view 
        returns (uint256 attributeProbabilityIndex) 
    {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) { // 0 >= 0
                attributeProbabilityIndex = i + 1; // 0+1
                break;
            }
        }
        return attributeProbabilityIndex;
    }

Tools Used

• Manual Review

Recommended Mitigation Steps

Recommend updating the FighterFarm::reRoll() to use the current fighterType of the given fighter in the reroll mechanism.

File: FighterFarm.sol

-   function reRoll(uint8 tokenId, uint8 fighterType) public {
+   function reRoll(uint8 tokenId) public {
+       uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0;
        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] = "";
        }
    } 

Assessed type

Other

Lines of code

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

Vulnerability details

Description

There is no update to the FighterFarm::numElements[] when a new generation of fighters occurs via FighterFarm::incrementGeneration() as the FighterFarm::numElements[] is hardcoded to support only generation:0 at during deployment.

File: FighterFarm.sol

    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
-->     numElements[0] = 3;
    } 

File: FighterFarm.sol

    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

Let's imagine that when the generation of the fighterType:0 (champions) is increased to generation:1 => generation[fighterType] == 1.

There is no numElements[1] support, making numElements[generation[fighterType]] => numElements[1] == 0 which consequently creates a modulo by zero revert in

    uint256 element = dna % 0; //REVERT

Impact

The transaction reverts on the fighter's element calculation in FighterFarm::_createFighterBase()

Consequently, it's unable to create the new generation fighter in case the input customAttributes == 100, triggering the execution of FighterFarm::_createFighterBase().

The following functions are impacted by this issue:

• FighterFarm::claimFighters()
• FighterFarm::redeemMintPass()
• FighterFarm::mintFromMergingPool()
• FighterFarm::reRoll()
• FighterFarm::_createNewFighter()

File: FighterFarm.sol

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

Proof of Concept

Setup

• Put the snippet below into the protocol test suite: test/FighterFarm.t.sol
• Run test: forge test --mt testCreateFighterRevertWhenNoSupportNumElements -vvv

The full coded PoC shown below:

<details> <summary>Details PoC</summary>

• The PoC code

    function testCreateFighterRevertWhenNoSupportNumElements() public {
        // increase the generation of both fighter type: 0
        _fighterFarmContract.incrementGeneration(0);
        assertEq(_fighterFarmContract.generation(0), 1);
        assertEq(_fighterFarmContract.generation(1), 0);

        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](1);
        claimModelTypes[0] = "original";

        // Right sender of signature should be able to claim fighter => revert
        vm.expectRevert(stdError.divisionError);
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);

    }

</details>

Results of running the test:

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testCreateFighterRevertWhenNoSupportNumElements() (gas: 91208)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.60ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Recommend updating the FighterFarm::numElements[] to support all generation increases.

Assessed type

Error

Judge has assessed an item in Issue #1100 as 2 risk. The relevant finding follows:

L-04: Lack of role revocation functionality in Neuron Contract

Lines of code

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

Vulnerability details

Description

The winners selected by the MergingPool::pickWinner() potentially be unable to claim at least some of their rewards (whenever the winner's owner balance is less than the FighterFarm::MAX_FIGHTERS_ALLOWED) if their balance is insufficient for their maximum rewards from their all rewards.

Since the MergingPool::pickWinner() selects the winner based on the fighter ID, it allows the same winner's owner to be included multiple times in the MergingPool::currentWinnerAddresses[].

File: MergingPool.sol

    function pickWinner(uint256[] calldata winners) external {
        require(isAdmin[msg.sender]);
        require(winners.length == winnersPerPeriod, "Incorrect number of winners");
        require(!isSelectionComplete[roundId], "Winners are already selected");
        uint256 winnersLength = winners.length;
        address[] memory currentWinnerAddresses = new address[](winnersLength);
        for (uint256 i = 0; i < winnersLength; i++) {
-->         currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
            totalPoints -= fighterPoints[winners[i]];
            fighterPoints[winners[i]] = 0;
        }
-->     winnerAddresses[roundId] = currentWinnerAddresses;
        isSelectionComplete[roundId] = true;
        roundId += 1;
    }

File: MergingPool.sol

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

Impact

Winners selected by the MergingPool::pickWinner() be unable to claim some or all of their rewards if their balance is insufficient to cover their maximum rewards.

This issue prevents players from receiving rewards due to their balance being unsatisfactory within the claim rewards iteration

Tools Used

• Manual Review

Proof of Concept

Let's consider the following scenario:

1. Bob owns 9 fighters, and two of his fighters are selected as winners for rewards. This assumes MAX_FIGHTERS_ALLOWED is set to 10, roundId is 0, and winnersPerPeriod is 2.

winnerAddresses[roundId: 0] = [Bob'address, Bob'address];

2. Bob tries to claim his rewards through the MergingPool::claimRewards()

3. Unfortunately, Bob is unable to claim any rewards due to a revert caused by the require statement in the FighterFarm::_createNewFighter().

Imagine within Bob's MergingPool::claimRewards() transaction:

• After the 1st iteration of MergingPool::claimRewards(), Bob's fighter balance becomes 10.
• During the 2nd iteration, a revert occurs due to require(balanceOf(Bob): 10 < MAX_FIGHTERS_ALLOWED: 10);.

As a result, Bob is unable to claim at least one reward from two eligible rewards because the claim rewards transaction reverts, even though he is eligible to claim at least one reward since Bob's balance has not reached the maximum to claim that one.

File: FighterFarm.sol

function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
-->    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
        
        // SNIPPED

    }

Recommended Mitigation Steps

Implement partially claim functionality within the MergingPool contract, or update the MergingPool::claimRewards() function to support the mentioned case in this issue.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L270-L290 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L244-L265 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L545

Vulnerability details

Description

The RankedBattle::unstakeNRN() allows the fighters to unstake all their amountStaked and be flagged as UNSTAKED, while those fighters still hold the remaining stakeAtRisk that has the potential to increase their amountStaked in their next battles, resulting in UNSTAKED fighters potentially having amountStaked > 0.

This allows them to

1. Having a staking balance without being flagged as STAKED status.
2. Bypassing the fighter transfer verification
3. Staking more in other rounds without flagging back to the STAKED status.

Impact

Impact of the Issue:

• Bypassing the fighter transfer verification that is designed to prevent the transfer of fighters when they are being staked, leading to conflicts in game mechanics and enabling the staked fighter to be traded in the secondary market.

• Conflicting logic regarding unstaked fighters as the term UNSTAKED fighter should refer to the fighters having no remaining stake.

Proof of Concept

Consider the following scenario 0. Let's assume that the fighter01 is being staked and then it play through the battles create some stakeAtRisk.

1. The owner of the fighter01 unstake all amountStaked[fighter01] by calling RankedBattle::unstakeNRN() and leaving some stakeAtRisk remaining, leading to that fighter will be flagged as UNSTAKED and gain the ability to transfer.

File: RankedBattle.sol

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        stakingFactor[tokenId] = _getStakingFactor( //@note if user not stake it will be 1
            tokenId, 
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        hasUnstaked[tokenId][roundId] = true;
        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
-->          if (amountStaked[tokenId] == 0) {
-->              _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

2. When the fighter01 win in the next battle at this round, amountStaked[fighter01] will be increased to reclaim its left stakeAtRisk through the RankedBattle::updateBattleRecord() --> RankedBattle::_addResultPoints().

However, the current staking status of the fighter01 still be UNSTAKED, while the amountStaked[fighter01] > 0

File: RankedBattle.sol

    if (curStakeAtRisk > 0) {
-->     _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner); 
-->     amountStaked[tokenId] += curStakeAtRisk; // increase amountStaked
    }

3. As the fighter01 is already flagged as UNSTAKED, and amountStaked[fighter01] > 0, when they call RankedBattle::stakeNRN() in the next round (the check of unstake status of this round is also passed), the execution will bypass this check and not flag the fighter01 back to STAKED.

File: RankedBattle.sol
    function stakeNRN(uint256 amount, uint256 tokenId) external {
        require(amount > 0, "Amount cannot be 0");
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        require(_neuronInstance.balanceOf(msg.sender) >= amount, "Stake amount exceeds balance");
        require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");

        _neuronInstance.approveStaker(msg.sender, address(this), amount);
        bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);
        if (success) {
-->         if (amountStaked[tokenId] == 0) { //Bypass this check
                _fighterFarmInstance.updateFighterStaking(tokenId, true);
            }
            amountStaked[tokenId] += amount;
            globalStakedAmount += amount;
            stakingFactor[tokenId] = _getStakingFactor(
                tokenId, 
                _stakeAtRiskInstance.getStakeAtRisk(tokenId)
            );
            _calculatedStakingFactor[tokenId][roundId] = true;
            emit Staked(msg.sender, amount);
        }
    }

And able to bypass this fighter transfer verification:

File: FighterFarm.sol

    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&    //> approval pass
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&   //> destination not reach the MAX
          !fighterStaked[tokenId]   //> not stake/ not lock
        );
    }

Setup

• Put the snippet below into the protocol test suite: test/RankedBattle.t.sol
• Run test: forge test --mt testIncorrectlyFlagOfStakingStatus -vvv

The full coded PoC shown below:

<details> <summary>Details PoC</summary>

• The PoC code

    function testIncorrectlyFlagOfStakingStatus() public {
        //STAKE
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        console.log("== STAKED at round 0 ==");
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0); //amount id
        console.log("STAKED Flag #1: %s\n", _fighterFarmContract.fighterStaked(0));

        address player2 = vm.addr(4);
        _mintFromMergingPool(player2);
        _fundUserWith4kNeuronByTreasury(player2);
        vm.prank(player2);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 1); //amount id

        //ANOTHER player Win => to increase the point in order to set new round
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, false);

        //BATTLE-1-Lost
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, false);

        //Unstake All
        console.log("== UNSTAKED at round 0 ==");
        vm.prank(player);
        _rankedBattleContract.unstakeNRN(3_000 * 10 ** 18, 0);
        console.log("STAKED Flag #2: %s\n", _fighterFarmContract.fighterStaked(0));

        console.log("== _GAME_SERVER_ADDRESS set new round ==\n");
        //BATTLE-2-Win
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, false);
        _rankedBattleContract.setNewRound();

        console.log("== STAKED at round 1 ==");
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        console.log("STAKED Flag #3: %s\n", _fighterFarmContract.fighterStaked(0));
    }

</details>

Results of running the test:

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testPoC17() (gas: 1646900)
Logs:
  == STAKED at round 0 ==
  STAKED Flag #1: true

  == UNSTAKED at round 0 ==
  STAKED Flag #2: false

  == _GAME_SERVER_ADDRESS set new round ==

  == STAKED at round 1 ==
  STAKED Flag #3: false


Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.68ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

I would recommend two approaches:

1. As the current behavior of unstaking allows fighters to unstake all eligible amountStaked and can be transferred even if the fighter has stakeAtRisk in the current round, which has the potential to increase their amountStaked in the next battles.

   I would recommend changing the unstake condition to flag back as UNSTAKED by including all potential stake factors: amountStaked and stakeAtRisk to ensure all staking and potential staking amounts are included to clarify marking as UNSTAKED.

File: RankedBattle.sol

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId, 
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        hasUnstaked[tokenId][roundId] = true;
        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
+           uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
+           if (amountStaked[tokenId] + stakeAtRisk == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

This still allows fighters to unstake all eligible amountStaked, and it will prevent the fighter from being flagged as UNSTAKED and transferred, in case they have only stakeAtRisk remaining.

2. In case there is a need to maintain the same behavior.

   I would recommend applying the condition to flag fighters back to STAKED whenever the amountStaked[tokenId] increases while they hold the UNSTAKED flag (this may consume more gas than the first approach).

File: RankedBattle.sol

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        uint256 stakeAtRisk;
        uint256 curStakeAtRisk;
        uint256 points = 0;

        /// Check how many NRNs the fighter has at risk
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {
            /// If the user won the match

            /// If the user has no NRNs at risk, then they can earn points
            if (stakeAtRisk == 0) {
                points = stakingFactor[tokenId] * eloFactor;
            }

            /// Divert a portion of the points to the merging pool
            uint256 mergingPoints = (points * mergingPortion) / 100;
            points -= mergingPoints;
            _mergingPoolInstance.addPoints(tokenId, mergingPoints);

            /// Do not allow users to reclaim more NRNs than they have at risk
            if (curStakeAtRisk > stakeAtRisk) {
                curStakeAtRisk = stakeAtRisk;
            }

            /// If the user has stake-at-risk for their fighter, reclaim a portion
            /// Reclaiming stake-at-risk puts the NRN back into their staking pool
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
+               if (!_fighterFarmInstance.fighterStaked(tokenId)) {
+                   _fighterFarmInstance.updateFighterStaking(tokenId, true);
+               }
            }

            /// Add points to the fighter for this round
            accumulatedPointsPerFighter[tokenId][roundId] += points;
            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
            totalAccumulatedPoints[roundId] += points;
            if (points > 0) {
                emit PointsChanged(tokenId, points, true);
            }
        } else if (battleResult == 2) {

            // SNIPPED

        }
    }

Assessed type

Other

Judge has assessed an item in Issue #1100 as 2 risk. The relevant finding follows:

[L-05] Lack of resetting accessible of crucial roles

Judge has assessed an item in Issue #1100 as 2 risk. The relevant finding follows:

[L-05] Lack of resetting accessible of crucial roles

QA Report

Summary

</br>

[L-01] Previous owner retains administrative abilities after ownership transfer<a name="l-01"></a>

As the first owner retrieves the admin accessibility at the deployment of the contract, however, the transferOwnership() does not reset the admin accessibility of the previous owner when transferring ownership.

This leads to the previous owner still retaining administrative abilities until the new owner resets their accessibility via adjustAdminAccess().

There is 5 instances of this issue:

File: GameItems.sol

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

File: MergingPool.sol

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

File: Neuron.sol

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

File: RankedBattle.sol

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

File: VoltageManager.sol

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

Recommendation

Update the transferOwnership() to reset the admin accessibility of the previous owner by adding a line of code to revoke their admin access.

The functions that should be updated for this issue are:

• GameItems::transferOwnership()
• MergingPool::transferOwnership()
• Neuron::transferOwnership()
• RankedBattle::transferOwnership()
• VoltageManager::transferOwnership()

The example recommendation code:

File: GameItems.sol

        function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
+       isAdmin[_ownerAddress] = false;
        _ownerAddress = newOwnerAddress;
    }

</br>

---

[L-02] The Neuron::burnFrom() does not support infinite approval<a name="l-02"></a>

The Neuron::burnFrom() does not support infinite approval, which is inconsistent with the behavior of ERC20::transferFrom() when the spender performs on behalf of the token owner.

There is one instance of this issue:

File: Neuron.sol

    function burnFrom(address account, uint256 amount) public virtual {
        require(
            allowance(account, msg.sender) >= amount, 
            "ERC20: burn amount exceeds allowance"
        );
        uint256 decreasedAllowance = allowance(account, msg.sender) - amount;
        _burn(account, amount);
        _approve(account, msg.sender, decreasedAllowance);
    }

Recommendation

Implement the functionality to support infinite approval within the Neuron::burnFrom() function, and also refactoring the function to consume gas more efficiently.

File: Neuron.sol

    function burnFrom(address account, uint256 amount) public virtual {
+        uint256 currentAllowance = allowance(account, msg.sender);
+        if (currentAllowance != type(uint256).max) {
            require(
-                allowance(account, msg.sender) >= amount
+                currentAllowance >= amount,
                "ERC20: burn amount exceeds allowance"
            );
+           unchecked {
+               _approve(account, msg.sender, currentAllowance - amount);
+           }
+       }
-       uint256 decreasedAllowance = allowance(account, msg.sender) - amount;
        _burn(account, amount);
-       _approve(account, msg.sender, decreasedAllowance);
    }

</br>

---

[L-03] Lack of setter functions for crucial external Addresses<a name="l-03"></a>

The treasuryAddress_ and delegatedAddress addresses are external contracts used to retrieve the NRN and act as signers of the claim fighter messages, respectively.

These two states lack setter functions, which means their addresses cannot be updated. Considering these addresses could potentially be compromised, adding the ability to update them would mitigate this risk.

There is 4 contracts of this issue:

• The FighterFarm contract
• The GameItems contract
• The Neuron contract
• The StakeAtRisk contract

Recommendation

Implementing setter functions for the treasuryAddress_ and delegatedAddress states to allow for their update is recommended.

These setter functions should be restricted to high-level roles to ensure proper control over the modification of these critical addresses.

The contracts that should be updated for this issue are:

• The FighterFarm contract
• The GameItems contract
• The Neuron contract
• The StakeAtRisk contract

</br>

---

[L-04] Lack of role revocation functionality in Neuron Contract<a name="l-04"></a>

The Neuron contract does not designate an account holding theAccessControl::DEFAULT_ADMIN_ROLE.

Therefore, if the contract owner mistakenly assigns roles such as MINTER_ROLE, STAKER_ROLE, or SPENDER_ROLE to unintended addresses, there is no mechanism to revoke these roles from those addresses.

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

There is one instance of this issue:

File: Neuron.sol

    contract Neuron is ERC20, AccessControl {

        // SNIPPED

    }

Recommendation

Assign an account holding the AccessControl::DEFAULT_ADMIN_ROLE in the Neuron contract to enable role management.

Additionally, implement logic to revoke the AccessControl::DEFAULT_ADMIN_ROLE when transferring ownership

File: Neuron.sol

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

    //SNIPPED

    function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
+       _revokeRole(DEFAULT_ADMIN_ROLE, _ownerAddress);
        _ownerAddress = newOwnerAddress;
+       _grantRole(DEFAULT_ADMIN_ROLE, newOwnerAddress);
    }

</br>

---

[L-05] Lack of resetting accessible of crucial roles<a name="l-05"></a>

The FighterFarm and GameItems contracts do not designate an address holding the staker role hasStakerRole and burning role allowedBurningAddresses, respectively.

There is 2 instances of this issue:

File: FighterFarm.sol

    function addStaker(address newStaker) external {
        require(msg.sender == _ownerAddress);
-->     hasStakerRole[newStaker] = true;
    }

File: GameItems.sol

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

Recommendation

Update the logic to flexibly set the accessible status as follows:

File: FighterFarm.sol

-   function addStaker(address newStaker) external {
+   function setStaker(address newStaker, bool status) external {
        require(msg.sender == _ownerAddress);
-       hasStakerRole[newStaker] = true;
+       hasStakerRole[newStaker] = status;
    }

File: GameItems.sol

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

</br>

---

[L-06] Lack of customized replenishment times for game items<a name="l-06"></a>

Each game item should have its replenishment time specified, taking into account factors such as the item's rarity or special abilities. This ensures that the time required to replenish each item is customized to its unique characteristics within the game.

There is one instance of this issue:

File: GameItems.sol

function _replenishDailyAllowance(uint256 tokenId) private {
        allowanceRemaining[msg.sender][tokenId] = allGameItemAttributes[tokenId].dailyAllowance;
-->     dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + 1 days);
    }

Recommendation

Update the logic to dynamically set the dailyAllowanceReplenishTime for each game items:

File: GameItems.sol

function _replenishDailyAllowance(uint256 tokenId) private {
        allowanceRemaining[msg.sender][tokenId] = allGameItemAttributes[tokenId].dailyAllowance;
-       dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + 1 days);
+       dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + allGameItemAttributes[tokenId].dailyAllowanceReplenishPeriod);
    }

File: GameItems.sol

    struct GameItemAttributes {
        string name;
        bool finiteSupply;
        bool transferable;
        uint256 itemsRemaining;
        uint256 itemPrice;
        uint256 dailyAllowance;
+       uint256 dailyAllowanceReplenishPeriod;
    }  

File: GameItems.sol

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance,
+       uint256 dailyAllowanceReplenishPeriod,
    ) 
        public 
    {
        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
                transferable,
                itemsRemaining,
                itemPrice,
                dailyAllowance,
+               dailyAllowanceReplenishPeriod
            )
        );
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

</br>

---

[N-01] Recommend explicitly setting the GameItems::itemsRemaining when the supply is not finite <a name="n-01"></a>

The GameItems::createGameItem() could enhancing the explicit setup of itemsRemaining when creating non-finite supply items.

There is one instance of this issue:

File: GameItems.sol

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 
        public 
    {
        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
                transferable,
                itemsRemaining,
                itemPrice,
                dailyAllowance
            )
        );
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

Recommendation

Explicitly set the GameItems::itemsRemaining when the supply is not finite to infinite itemsRemaining supply.

File: GameItems.sol

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 
        public 
    {
        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
                transferable,
+               finiteSupply ? itemsRemaining: type(uint256).max,
                itemPrice,
                dailyAllowance
            )
        );
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

</br>

---

[N-02] Enhance consistency in the GameItems::remainingSupply() when querying items of non-finite supply<a name="n-02"></a>

The GameItems::remainingSupply() could be improved to provide clearer support for querying the supply of each item by returning the finiteSupply status along with the supply value.

This can explicitly assist in off-chain retrieval of remainingSupply of each item and facilitate better decision-making regarding the management of items as a non-finite supply items.

There is one instance of this issue:

File: GameItems.sol

    function remainingSupply(uint256 tokenId) public view returns (uint256) {
        return allGameItemAttributes[tokenId].itemsRemaining;
    }

Recommendation

Update the setting the GameItems::remainingSupply() to return itemsRemaining amount along with the item's finiteSupply status.

File: GameItems.sol

    function remainingSupply(uint256 tokenId) public view returns (uint256, bool) {
+       return (allGameItemAttributes[tokenId].itemsRemaining, allGameItemAttributes[tokenId].finiteSupply);
    }

</br>

---