RabbitHole Quest Protocol contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

By default, the protocol receives a 20% cut per quest. After the quest ends, the admin is allowed to withdraw the protocol's share. But, there are no checks stopping them from withdrawing the fees multiple times. That allows them to drain any remaining funds inside the contract.

Proof of Concept

The withdrawFee() function implements no checks on whether it was already called. Thus you can call it multiple times to drain the contract:

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

The protocol's fees are 20% of the actual claimed user rewards:

    function protocolFee() public view returns (uint256) {
        // receiptRedeemers = number of minted receipts for this quest
        return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
    }

The following scenario is possible:

• Quest is funded with 120,000 tokens (100,000 max tokens for user rewards & 20,000 max tokens for the protocol fee)
• half of the user rewards are claimed; protocol is rewarded 50,000 * 0.2 = 10,000 tokens
• 70,000 tokens remain after the contest has ended.
• Admin can now call withdrawFee() to pull 10,000 tokens from the contract. By calling it 6 more times, they can also withdraw the remaining 60,000 tokens.

After a contest has ended, the quest owner is supposed to withdraw the remaining tokens using withdrawRemainingTokens(). To rug the quest owner, the admin has to withdraw the fees before them.

Tools Used

none

Recommended Mitigation Steps

Add a check to withdrawFee() so that it can only be called once.

Lines of code

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

Vulnerability details

Impact

The QuestFactory contract allows users to mint receipts for quests that have already ended. That can cause issues with the reward distribution because the quest contract might not have enough funds to cover all the existing receipts. A receipt holder might not be able to claim their reward.

Proof of Concept

The mintReceipt() function doesn't verify that the quest hasn't ended yet:

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

After a quest has ended, the owner of the quest is able to withdraw any remaining funds that are not reserved for existing receipts:

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

The combination of both can cause the following issue:

1. Alice starts a quest and funds it with 10e18 tokens for a maximum of 8 recipients (2 tokens are reserved for the protocol fee)
2. 7 people finish the quest and mint receipts
3. the quest ends At this point we have the following state:

• locked up funds for user rewards: 7e18
• protocol fee: 7e18 * 0.2 = 1.4e18
• remaining tokens: 1.6e18

The owner of the quest now calls withdrawRemainingTokens() to withdraw the 1.6e18 tokens. This leaves the contract with exactly 8.4e18 tokens that are supposed to cover the 7 receipt holders as well the protocol fees.

At this point, Bob calls mintReceipt() using the signature he has received at some point in the past. Since these signatures don't expire there's no deadline until which he has to submit it. This increases the number of receipts to 8. The 7e18 tokens reserved for the initial 7 receipt holders are now claimable by another user. This will lead to one of the receipt holders not being able to claim their reward since there are not enough tokens.

It also changes the protocol's fees. The protocol fees are calculated using the number of minted receipts. Since that value has increased, so do the protocol fees:

    function protocolFee() public view returns (uint256) {
        return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
    }

The admin is now able to withdraw 8 * 1e18 * 2_000 / 10_000 = 1.6e18

To summarize, minting receipts after a quest has ended breaks the reward distribution logic.

Tools Used

none

Recommended Mitigation Steps

It should be impossible to mint receipts for quests that have already ended.