RabbitHole Quest Protocol contest - Tricko's results

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/Quest.sol#L96-L118 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L102-L104

Vulnerability details

Impact

When block.timestamp > endTime_ the Erc20Quest owner can call withdrawRemainingTokens to retrieve the balance in the contract that is not tied to fees or to unclaimed receipts. The contract logic takes into account unclaimed rewards by calculating the difference between the number of receipts minted in QuestFactory and how many were claimed in the Erc20Quest contract. But outstanding signatures-hash pairs (from users that did the required on-chain action and receive their signature-hash pair, but not yet used for minting receipts) are unaccounted.

If there are outstanding signatures-hash pairs (consider them as sigOustanding) and if they are used to obtain receipts after withdrawRemainingTokens was called, claiming them can lead to a potential loss of funds to either the RabbitHole protocol or others users.

Consider that a quest contract was started with exactly maxTotalRewards + maxProtocolReward in its balance, some time after endTime_ withdrawRemainingTokens is called, the ERC20 balance on the contract should be exactly protocolFee() + (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId. But if there are outstanding signature-hash pairs, those could still be used for minting receipt and claiming rewards, but because they are unaccounted on withdrawRemainingTokens logic, the remaining balance in the contract doesn't hold enough tokens for all liabilities (protocol fees + unclaimed rewards). Therefore there are two possible outcomes depending on the specific order of events, but on either of them permanent loss of funds is a possibility:

1. If withdrawRemainingTokens is called, but all the still unclaimed tokens + sigOustanding are able to be claimed before withdrawFee is called, then the remaining balance on the quest contract will be less than protocol fee, thus leading to permanent loss of funds to the protocol.

2. If withdrawRemainingTokens is called, followed by withdrawFee, then the remaining balance on the quest contract will be less than (receiptRedeemers() - redeemedTokens + sigOustanding) * rewardAmountInWeiOrTokenId, thus leading to permanent loss of funds to users, at least one of them (but possibly more, depending on sigOustanding) will not be able to claim and receive their rewards.

Proof of Concept

Consider the first scenario from above (small values were chosen to ease the explanation). New Erc20Quest with totalParticipants_ = 10, rewardAmountInWeiOrTokenId_ = 100 and questFee = 2000 (20%).

1. Quest is started (contract ERC20 balance = 1200)
2. 7 users do the required on-chain action and receive their signature-hash pair
3. 6 of those users mint their receipt and proceed to claim their rewards (contract ERC20 balance = 1200 - 6*100 = 600) Enough time has passed, so that block.timestamp > endTime_
4. quest owner calls withdrawRemainingTokens and withdraws 480 tokens (600 - 600*0.2) (contract ERC20 balance = 600 - 480 = 120)
5. the last user mint their receipt and proceed to claim his reward (contract ERC20 balance = 120 - 100 = 20)
6. withdrawFee is called but reverts, as there is not enough funds to pay the protocol fees

See the following POC

import { expect } from 'chai'
import { ethers, upgrades } from 'hardhat'
import { Wallet, utils } from 'ethers'
import {
  Erc20Quest__factory,
  RabbitHoleReceipt__factory,
  SampleERC20__factory,
  Erc20Quest,
  SampleERC20,
  RabbitHoleReceipt,
  QuestFactory,
  QuestFactory__factory,
} from '../typechain-types'

describe('POC', async () => {
    it('should fail to transfer protocol fees', async () => {

        //Setup
        const mnemonic = 'announce room limb pattern dry unit scale effort smooth jazz weasel alcohol'
        const questId = 'asdf'
        const totalParticipants = 10
        const rewardAmount = 100
        const questFee = 2000 // 20%
        const totalRewardsPlusFee = totalParticipants * rewardAmount + (totalParticipants * rewardAmount * questFee) / 10_000

        let accounts = await ethers.getSigners()
        let owner = accounts[0]
        const protocolFeeAddress = '0xE8B17e572c1Eea45fCE267F30aE38862CF03BC84'

        let questContract: Erc20Quest__factory = await ethers.getContractFactory('Erc20Quest')
        let sampleERC20Contract: SampleERC20__factory = await ethers.getContractFactory('SampleERC20')
        let rabbitholeReceiptContract: RabbitHoleReceipt__factory = await ethers.getContractFactory('RabbitHoleReceipt')
        let questFactoryContract: QuestFactory__factory = await ethers.getContractFactory('QuestFactory')
        let wallet: Wallet = Wallet.fromMnemonic(mnemonic)

        let expiryDate: number = Math.floor(Date.now() / 1000) + 100000
        let startDate: number = Math.floor(Date.now() / 1000) + 1000

        const ReceiptRenderer = await ethers.getContractFactory('ReceiptRenderer')
        const deployedReceiptRenderer = await ReceiptRenderer.deploy()
        await deployedReceiptRenderer.deployed()

        let deployedRabbitholeReceiptContract = (await upgrades.deployProxy(rabbitholeReceiptContract, [
        deployedReceiptRenderer.address,
        owner.address,
        owner.address,
        10,
        ])) as RabbitHoleReceipt

        let deployedSampleErc20Contract: SampleERC20  = await sampleERC20Contract.deploy(
        'RewardToken',
        'RTC',
        totalRewardsPlusFee,
        owner.address
        )
        await deployedSampleErc20Contract.deployed()

        let deployedFactoryContract = (await upgrades.deployProxy(questFactoryContract, [
            wallet.address,
            deployedRabbitholeReceiptContract.address,
            protocolFeeAddress,
        ])) as QuestFactory

        await deployedFactoryContract.setRewardAllowlistAddress(deployedSampleErc20Contract.address, true)
        const createQuestTx = await deployedFactoryContract.createQuest(
        deployedSampleErc20Contract.address,
        expiryDate,
        startDate,
        totalParticipants,
        rewardAmount,
        'erc20',
        questId
        )
        const waitedTx = await createQuestTx.wait()
        const event = waitedTx?.events?.find((event) => event.event === 'QuestCreated')
        const [_from, contractAddress, type] = event?.args
        let deployedQuestContract: Erc20Quest = await questContract.attach(contractAddress)
        const distributorContractAddress = deployedQuestContract.address
        await deployedSampleErc20Contract.functions.transfer(distributorContractAddress, totalRewardsPlusFee)

        // 1. Quest is started (contract ERC20 balance = 1200)
        await deployedQuestContract.start()
        await ethers.provider.send('evm_increaseTime', [86400])
        expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(1200)

        // 2. 7 users do the required on-chain action and receive their signature-hash pair
        let signatures: string[] = []
        let hashes: string[] = []
        for (let i = 1; i < 8; i++) {
            let hash = utils.solidityKeccak256(['address', 'string'], [accounts[i].address.toLowerCase(), questId])
            let sig = await wallet.signMessage(utils.arrayify(hash))
            hashes.push(hash)
            signatures.push(sig)
        }

        // 3. 6 of those users mint their receipt and proceed to claim their rewards (contract ERC20 balance = 600)
        for (let i = 1; i < 7; i++) {
            await deployedFactoryContract.connect(accounts[i]).mintReceipt(questId, hashes[i-1], signatures[i-1])
        }

        for (let i = 1; i < 7; i++) {
            await deployedQuestContract.connect(accounts[i]).claim()
        }
        expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(600)

        await ethers.provider.send('evm_increaseTime', [100001])
        
        // 4. quest owner calls `withdrawRemainingTokens` and withdraws 480 tokens (contract ERC20 balance = 120)
        await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)
        expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(120)

        // 5. the last user mint their receipt and proceed to claim his reward (contract ERC20 balance = 20)
        await deployedFactoryContract.connect(accounts[7]).mintReceipt(questId, hashes[6], signatures[6])
        await deployedQuestContract.connect(accounts[7]).claim()
        expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(20)

        // 6. `withdrawFee` is called but reverts, as there is not enough funds to pay the protocol fees
        await expect(deployedQuestContract.withdrawFee()).to.be.revertedWith("ERC20: transfer amount exceeds balance")
    })
})

Recommended Mitigation Steps

Please consider adding an endTime_ value to the hash to be signed and check block.timestamp > endTime_ in mintReceipt. Therefore making signature-hash pairs expire after endTime_. See the following patch below.

diff --git a/QuestFactory.sol.orig b/QuestFactory.sol
index cfba7f5..0871636 100644
--- a/QuestFactory.sol.orig
+++ b/QuestFactory.sol
@@ -216,10 +216,11 @@ contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgrade
     /// @param questId_ The id of the quest
     /// @param hash_ The hash of the message
     /// @param signature_ The signature of the hash
-    function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
+    function mintReceipt(string memory questId_, uint256 endTime_, 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 (block.timestamp > endTime_) revert SignatureExpired();
+        if (keccak256(abi.encodePacked(msg.sender, questId_, endTime_)) != hash_) revert InvalidHash();
         if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

         quests[questId_].addressMinted[msg.sender] = true;

Lines of code

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

Vulnerability details

Impact

After calling mintReceipt with a valid signature-hash pair, the user receives an ERC721 token as a receipt, which can be used later for claiming rewards. Users are free to sell and buy receipts from other users. The actual value of the receipt lies on its use to claim rewards, therefore a receipt which has already been claimed is worthless.

On non-custodial NFT marketplaces like Opensea, Superare and many others, the seller maintains ownership of the token until the sell occurs. Therefore a malicious seller can place an offer on any non-custodial NFT marketplaces for a valid unclaimed receipt, and then frontrun the sell transaction by calling claim. Therefore claiming the receipt before the sell is finalized.

This can harm the procotol credibility as user would be discouraged from buying receipts from others without using special services like Flashbots Protect to avoid being frontrunned.

Proof of Concept

Consider the following series of events

1. Eve place an offer for her quest receipt
2. Alice verifies that Eve's offer is a valid unclaimed token, by calling isClaimed
3. Alice send the buy confirmation transaction
4. Eve sees Alice transaction on the mempool and send her own transaction to claim her receipt with higher gas price.

If Eve's transaction manages to frontrun Alice's, then she was successful in selling a claimed token to Alice.

Recommended Mitigation Steps

Consider modifying the _beforeTokenTransfer to revert on transfers of claimed receipt tokens or consider burning the receipt token after they are claimed.

Lines of code

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

Vulnerability details

Impact

The contents of the message hashed, then signed to generate the signature-hash pair used for minting new receipts are only the user address and questId_. Without any other unique identifier, like chainId, the signature hash cannot uniquely identify the blockchain the transaction was intended for, therefore leading to potential replay attacks on other networks.

For the same chain, questId_ must be unique, because during quest creation questId_ are stored on the quests mapping and if there is another quest already created with the same questId_, then createQuest reverts.

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

But for different networks, collisions between questId_ are possible. Besides createQuest, nowhere in the code questId_ uniqueness is enforced. Nor it is said in the docs. Therefore if the RabbitHole Quest Protocol plans to be deployed on multiple chains, it is expected that collisions between questId_ on different chains are a possibility. Thus leading to possible signature replay attacks.

Proof of Concept

Consider that protocol X starts two quests with the same questId_ on chain A and chain B

1. Alice does the specified on-chain action (aka quest) on chain A, then later she obtains the hash-signature pair to mint her receipt.
2. Alice uses her hash-signature pair to mint her receipt on chain A.
3. Alice is able to reuse this same hash-signature pair on chain B Threfore Alice can bypass the on-chain quest and use her hash-signature pair to directly obtain another receipt on chain B.

Recommended Mitigation Steps

Consider adding chainId to the signed hash, making this replay attack across chain impossible.