RabbitHole Quest Protocol contest - carrotsmuggler's results

Lines of code

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

Vulnerability details

Impact

The withdrawFee() function, responsible for collecting the fees has two issues:

1. No access control
2. No checks on whether it has been already called

The function contains the modifier onlyAdminWithdrawAfterEnd, however, when we look into the said modifier we see it only checks for the endTime, and not the msg.sender as the name would imply

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

So any user can call this function once the endTime has passed, and they can do so multiple times, deducting the fees from the contract and essentially draining it before the actual users who completed the quests are able to redeem their receipts. This is classified as high severity since it denies the users awards, and drains the contract.

Proof of Concept

The following test snippet shows how the fee can be deducted twice without any issue by a non-admin (firstAddress). This can be repeated until the contract is drained.

it('should pay fee multiple times', async () => {
      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])
      await deployedQuestContract.connect(firstAddress).claim()
      await ethers.provider.send('evm_increaseTime', [100001])

      // Initial balances
      let initBal = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
      let protocolFee = await deployedQuestContract.protocolFee()

      // First fee deduction
      await deployedQuestContract.connect(firstAddress).withdrawFee()
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        initBal.sub(protocolFee)
      )

      // Second fee deduction
      await deployedQuestContract.connect(firstAddress).withdrawFee()
      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
        initBal.sub(protocolFee).sub(protocolFee)
      )
      await ethers.provider.send('evm_increaseTime', [-100001])
      await ethers.provider.send('evm_increaseTime', [-86400])
    })

Tools Used

Hardhat

Recommended Mitigation Steps

Prevent fee from being deducted multiple times.

// Declare a bool
bool feeDeducted;

function withdrawFee() public onlyAdminWithdrawAfterEnd {
        // Check bool
        require(!feeDeducted, "Fee already deducted");

        // Set bool
        feeDeducted = true;
   
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }

Also, consider changing the modifier onlyAdminWithdrawAfterEnd to also check msg.sender, or rename it to better represent its purpose since it is misleading in its current form.

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/RabbitHoleReceipt.sol#L98-L104

Vulnerability details

Impact

The function withdrawRemainingTokens() in ERC20Quest.sol as shown below shows the intent of saving tokens for users who have minted their receipt, but haven't claimed their tokens yet, when the owner tries to drain the contract.

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

This intent is also included in the natspec, setting an expectation that a user who has completed the quest in the stipulated time will be able to claim their reward even if the owner tries to disable the quest. However, this expectation can be broken.

Receipts can be minted by calling the function mintReceipt() in QuestFactory.sol. However this function never chacks for the ending timestamps of any of the quests. Thus, a malicious user can complete the quest after the endtime, generate a signature and claim the prize tokens, denying a legitimate user who hasn't claimed yet from a payout since the contract will not have enough tokens. This breaks the intent specified by the natspec and the code, and is classified as high severity as it leads to denying of rewards by a malicious user.

The RabbitHoleReceipt.sol contract also has a public mapping timestampForTokenId which keeps track of mint times, again showing intent to control access based on timestamps but this is never used.

Proof of Concept

POC isn't provided since it is trivially shown. The mintReceipt function is in a different contract (QuestFactory.sol) and has no information about the end timestamps of the quests, which are stored in ERC20Quest.sol.

Tools Used

Manual Review

Recommended Mitigation Steps

Incorporate the timestampForTokenId mapping to check for valid receipts in the claim function in Quest.sol. It is shown here in the if statement checking claimed tokens.

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]) && rabbitHoleReceiptContract.timestampForTokenId(i) < endTime ) {
                redeemableTokenCount++;
            }
        }

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

        uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
        _setClaimed(tokens);
        _transferRewards(totalRedeemableRewards);
        redeemedTokens += redeemableTokenCount;

        emit Claimed(msg.sender, totalRedeemableRewards);
    }

Another option would be to include this check in the claim isClaimed() function and set all expired tokens as claimed, which can be useful for the front end.

Lines of code

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

Vulnerability details

Impact

The contract ERC20Quest.sol has two functions of interest here. The first is withdrawFee(), which is responsible for transferring out the fee amount from the contract once endTime has been passed, and the second is withdrawRemainingTokens() which recovers the remaining tokens in the contract which haven't been claimed yet.

Function withdrawRemainingTokens():

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

As evident from this excerpt, calling this recovery function subtracts the tokens which are already assigned to someone who completed the quest, and the fee, and returns the rest. However, there is no check for whether the fee has already been paid or not. The owner is expected to first call withdrawRemainingTokens(), and then call withdrawFee().

However, if the owner calls withdrawFee() before calling the function withdrawRemainingTokens(), the fee will be paid out by the first call, but the same fee amount will still be kept in the contract after the second function call, basically making it unrecoverable. Since there are no checks in place to prevent this, this is classified as a high severity since it is an easy mistake to make and leads to loss of funds of the owner.

Proof of Concept

This can be demonstrated with this test

describe('Funds stuck due to wrong order of function calls', async () => {
    it('should trap funds', async () => {
      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])
      await deployedQuestContract.connect(firstAddress).claim()

      await ethers.provider.send('evm_increaseTime', [100001])
      await deployedQuestContract.withdrawFee()
      await deployedQuestContract.withdrawRemainingTokens(owner.address)

      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(200)
      expect(await deployedSampleErc20Contract.balanceOf(owner.address)).to.be.lessThan(
        totalRewardsPlusFee * 100 - 1 * 1000 - 200
      )
      await ethers.provider.send('evm_increaseTime', [-100001])
      await ethers.provider.send('evm_increaseTime', [-86400])
    })
  })

Even though the fee is paid, the contract still retains the fee amount. The owner receives less than the expected amount. This test is a modification of the test should transfer non-claimable rewards back to owner already present in ERC20Quest.spec.ts.

Tools Used

Hardhat

Recommended Mitigation Steps

Only allow fee to be withdrawn after the owner has withdrawn the funds.

// Declare a boolean to check if recovery happened
bool recoveryDone;

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);
   
        // Set recovery bool
        recoveryDone = true;
    }
function withdrawFee() public onlyAdminWithdrawAfterEnd {
        // Check recovery
        require(recoveryDone,"Recover tokens before withdrawing Fees");
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }

1. ERC1155Quest.sol: withdrawRemainingTokens() does not leave tokens for users who completed the challenge but hasn't claimed their reward yet.

ERC20Quest.sol implements this function in a way where users can still claim their rewards after the owner clears out the quest contract. Suggest implementing the same logic for ERC1155Quest as well.

2. ERC1155Quest.sol has no recovery/destination address option

ERC721 and ERC1155 transfer contracts generally have a recovery address option allowing the msg.sender to send the ERC721/1155 token to a different address. This is recommended since not all smart contract wallets have the required callback functions implemented, and thus might lead to stuck tokens/unexpected reverts. Advised to add recovery address option, or take a destination address as an input.

3. QuestFactory.sol: mintReceipt() has no recovery/destination address option

ERC721 and ERC1155 transfer contracts generally have a recovery address option allowing the msg.sender to send the ERC721/1155 token to a different address. This is recommended since not all smart contract wallets have the required callback functions implemented, and thus might lead to stuck tokens/unexpected reverts. Advised to add recovery address option, or take a destination address as an input.

4. Quest.sol: start() should only be callable once

Currently, start() function can be called multiple times, and unpauses the contract as well. General practice is to have a start() function which is callable once, and use pause/unpause to control the contract instead of the start function.

5. Quest.sol: onlyAdminWithdrawAfterEnd() modifier does not check for admin

The modifier name says onlyAdmin, but the modifier itself doesn't check for admin. Consider renaming the modifier.

6. Quest.sol: Long list of minted receipts can cause out of gas error.

Consider limiting maximum number of claims in a single transaction.

7. QuestFactory.sol: Missing address validations when creating ERC1155Quests

ERC1155Quest creator does not validate the passed ERC1155 address. It is easy to validate ERC1155 address since they support ERC165 so a simple supportInterface call is advised.

8. QuestFactory.sol: totalparticipants_ should be required to be more than 0

9. Missing events

Every change of storage state in a contract should be accompanied by an event. This is missing in a few places as listed below

• Quest.sol
  • start() -pause() -unpause()