AI Arena - pynschon's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/token/ERC721/ERC721.sol#L167-L170

Vulnerability details

FighterFarm tokens must not be transferred, while staked. To enforce this the protocol has implemented checks on FighterFarm::safeTransferFrom before allowing transfer.

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

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

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

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

However since FighterFarm is an ERC721 contract witch also allows you transfer tokens using 721::safeTransferFrom with the data argument which isn't overridden by FighterFarm and isn't protected by any checks.

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

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/token/ERC721/ERC721.sol#L167-L170

Impact

• Users can transfer their staked NFTs, and that is not the desired behavior specified by the protocol.

Proof of Concept

 function TransferringFighterWhileStaked() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true); // Token is now staked
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        _fighterFarmContract.safeTransferFrom(
            _ownerAddress,
            _DELEGATED_ADDRESS,
            0,
            "" // data param
        );
        // Token has been transfer to new owner
        assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
    }

Tools Used

Foundry

Recommended mitigation steps

• Override 721::safeTransferFrom with the data argument and implement checks to verify if the NFT isn't staked, then allow transfer.

Assessed type

ERC721

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/token/ERC721/ERC721.sol#L167-L170

Vulnerability details

FighterFarm states that a user can only own 10 NTF fighters MAX_FIGHTERS_ALLOWED = 10. To enforce this the protocol has implemented checks on FighterFarm::safeTransferFrom before allowing transfer, and when minting as well, so users cannot mint more than the max allowed.

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

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

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

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

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

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

However since FighterFarm is an ERC721 contract witch also allows you transfer tokens using ERC721::safeTransferFrom with the data argument which isn't overridden by FighterFarm and isn't protected by any checks.

    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

• Users can own more than the max NFT fighters allowed, which isn't allowed by the protocol.

Proof of Concept

1. User mints MAX_FIGHTERS_ALLOWED.
2. User mints MAX_FIGHTERS_ALLOWED again using another account that they own.
3. Transfer form the other account to the current one, the NFTs using function safeTransferFrom(address from,address to,uint256 tokenId, bytes memory data) since there are no checks.

The NFT balance of the user will be more than MAX_FIGHTERS_ALLOWED

   function UserCanOwnMoreThanMaxAllowed() public {
        uint8 amount = 10;
        uint256 maxFightersAllowed = 10;
        mintAmountFighterTokens(address(this), amount);
        assertEq(_fighterFarmContract.balanceOf(address(this)), amount);

        vm.prank(_other);
        mintAmountFighterTokens(_other, amount);
        assertEq(_fighterFarmContract.balanceOf(_other), amount);

        for (uint256 i = 10; i < 20; i++) {
            vm.prank(_other);
            _fighterFarmContract.safeTransferFrom(
                _other,
                address(this),
                i,
                "" // data param
            );
        }

        assertEq(_fighterFarmContract.balanceOf(address(this)), 20);
        assertTrue(
            _fighterFarmContract.balanceOf(address(this)) > maxFightersAllowed
        );
    }

Tools Used

Foundry

Recommended Mitigation Steps

• Override ERC721::safeTransferFrom with the data argument and implement checks to verify if the NFT isn't staked, then allow transfer.

Assessed type

Other

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/token/ERC1155/ERC1155.sol#L114-L126

Vulnerability details

Game items have of the attribute of transferability, if it's set to true, users can transfer them otherwise they cannot. To enforce this the protocol has implemented checks on safeTransferFrom before allowing transfer.

File: src/GameItems.sol
    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);
    }

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

However GameItems is an ERC1155 contract witch also allows you transfer tokens using ERC1155::safeBatchTransferFrom which isn't overridden by GameItems and isn't protected by any checks.

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

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/token/ERC1155/ERC1155.sol#L114-L126

Impact

• Game mechanics can be cheated, which lead to an unfair game play.

Proof of Concept

 function TransferNonTransferableUsingBatchTransfer() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.createGameItem(
            "Battery",
            "https://ipfs.io/ipfs/",
            true,
            false, // transferable
            10_000,
            1 * 10 ** 18,
            10
        );

        _gameItemsContract.mint(0, 1);
        uint256[f] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;

        _gameItemsContract.safeBatchTransferFrom(
            _ownerAddress,
            address(_receiver),
            ids,
            amounts,
            ""
        );
        assertEq(_gameItemsContract.balanceOf(address(_receiver), 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used

Foundry

Recommended mitigation steps

• Override ERC1155::safeBatchTransferFrom and implement checks to verify if the game item is transferable, then allow transfer.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L509-L510 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L377-L390 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L95

Vulnerability details

FighterFarm::reRoll allows users to generate new random traits for their fighter, but when rerolling the function uses the previous dendroidBool which is derived from bool dendroidBool = fighterType == 1; but this only set when the user first mints and not when rerolling. In the case of when a user first mints a Fighter NFT with (fighterType 1), then decide to reroll the fighter using (fighterType 0) the NFT will not get random traits.

    bool dendroidBool = fighterType == 1;

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

      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 // Previously stored dendroidBool.
            );
            _tokenURIs[tokenId] = "";
        }

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

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

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L95

Impact

• Users will not get random traits when rerolling if they switch form (fighterType 1) to (fighterType 0).

Proof of Concept

 function rerollDoesNotGenerateRandomAttributes() public {
        _fundUserWith4kNeuronByTreasury(address(this));
        mintAmountFighterTypeOneTokens(address(this), 1);
        assertEq(_fighterFarmContract.balanceOf(address(this)), 1);

        (, uint256[6] memory attributes, , , , , ) = _fighterFarmContract
            .getAllFighterInfo(
                0 // tokenId
            );

        for (uint256 i; i < attributes.length; i++) {
            assertEq(99, attributes[i]);
        }

        _neuronContract.addSpender(address(_fighterFarmContract));

        _fighterFarmContract.reRoll(
            0, // tokenId
            0 // fighterType
        );

        for (uint256 i; i < attributes.length; i++) {
            assertEq(99, attributes[i]); // physicalAttributes did not change
        }
    }

Tools Used

Foundry

Recommended Mitigation Steps

• Update dendroidBool when rerolling, so it would take into consideration the updated fighterType

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L104-L111 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L499-L501 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L462-L474

Vulnerability details

To create the element for the fighter _createFighterBase function uses dna % numElements[generation[fighterType]]. The numElements is only set in the constructor numElements[0] = 3; and there is no setter function to modify it. When the fighterType generation is incremented the numElements for that generation will be 0, which leads to modulo by 0. Consequently, every time a user tries to mint a fighter NFT with (customAttributes[0] == 100) the modulo by zero will cause the transaction to revert.

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

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

        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }

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

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

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

Impact

• FighterFarm will be DoS'd and users cannot mint any fighter NFTs.

Proof of Concept

 function ClaimFightersFailModByZero() public {
     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";

        //Increment generation for fighter type 0
        _fighterFarmContract.incrementGeneration(0);

        // Reverts because of modulo by 0
        vm.expectRevert();
        _fighterFarmContract.claimFighters(
            numToMint,
            claimSignature,
            claimModelHashes,
            claimModelTypes
        );
    }

Tools Used

Foundry

Recommended mitigation steps

• Set numElements to a non-zero value when incrementing fighter generation, to prevent modulo by 0.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L93-L96 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L101-L104 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L109-L112

Vulnerability details

Neuron improperly uses AccessControl::_setupRole to grant roles, _setupRole should only be called from the constructor when setting up the initial roles for the system. When a role is granted to an address it can either be revoke by an address that has the adminRole for that role or if the user renounces the role, but since no address has been assigned the adminRole for any of the roles (MINTER_ROLE, SPENDER_ROLE, STAKER_ROLE), there is no way of revoking granted roles.

    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L93-L96

   function addStaker(address newStakerAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(STAKER_ROLE, newStakerAddress);
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L101-L104

    function addSpender(address newSpenderAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(SPENDER_ROLE, newSpenderAddress);
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L109-L112

     * [WARNING]
     * ====
     * This function should only be called from the constructor when setting
     * up the initial roles for the system.
     *
     * Using this function in any other way is effectively circumventing the admin
     * system imposed by {AccessControl}.
     * ====
     *
     * NOTE: This function is deprecated in favor of {_grantRole}.
     */
    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/access/AccessControl.sol#L186-L208

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

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/bd325d56b4c62c9c5c1aff048c37c6bb18ac0290/contracts/access/AccessControl.sol#L160-L162

Impact

• If addresses with any or all the roles (MINTER_ROLE, SPENDER_ROLE,STAKER_ROLE) turn out to be malicious or compromised, there's no way of revoking their access.

Recommended Mitigation Steps

• Only use _setupRole in the constructor to set up adminRoles, then grant roles using grantRole which performs the required checks.

Assessed type

Access Control