RabbitHole Quest Protocol contest - StErMi's results

Lines of code

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

Vulnerability details

Impact

Erc1155Quest like the Erc20Quest have a method that allows the owner of the quest to withdraw all the remaining tokens once the quest has passed the endTime.

The main difference is that the Erc1155Quest contract does not check if some of those token have been already "locked" to some user that has minted a receipt.

Once the owner has called withdrawRemainingTokens those users will not be able to redeem their reward anymore.

Let's make an example

1. alice complete a quest and receive a receipt
2. alice decide to wait before claiming the reward
3. The quest end and the owner of it call withdrawRemainingTokens transferring all the tokens that the quest owns
4. alice will not be able anymore to call claim because now the quest has 0 balance of the reward token

Proof of Concept

Link to affected code

• Erc1155Quest.sol#L54-L63

Test code

import { expect } from 'chai'
import { ethers, upgrades } from 'hardhat'
import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
import {
  Erc1155Quest__factory,
  RabbitHoleReceipt__factory,
  SampleErc1155__factory,
  Erc1155Quest,
  SampleErc1155,
  RabbitHoleReceipt,
} from '../typechain-types'

describe('Erc1155Quest', () => {
  let deployedQuestContract: Erc1155Quest
  let deployedSampleErc1155Contract: SampleErc1155
  let deployedRabbitholeReceiptContract: RabbitHoleReceipt
  let expiryDate: number, startDate: number
  const mockAddress = '0x0000000000000000000000000000000000000000'
  const questId = 'asdf'
  const totalRewards = 10
  const rewardAmount = 1
  let owner: SignerWithAddress
  let firstAddress: SignerWithAddress
  let secondAddress: SignerWithAddress
  let thirdAddress: SignerWithAddress
  let fourthAddress: SignerWithAddress
  let questContract: Erc1155Quest__factory
  let sampleERC20Contract: SampleErc1155__factory
  let rabbitholeReceiptContract: RabbitHoleReceipt__factory

  beforeEach(async () => {
    const [local_owner, local_firstAddress, local_secondAddress, local_thirdAddress, local_fourthAddress] =
      await ethers.getSigners()
    questContract = await ethers.getContractFactory('Erc1155Quest')
    sampleERC20Contract = await ethers.getContractFactory('SampleErc1155')
    rabbitholeReceiptContract = await ethers.getContractFactory('RabbitHoleReceipt')

    owner = local_owner
    firstAddress = local_firstAddress
    secondAddress = local_secondAddress
    thirdAddress = local_thirdAddress
    fourthAddress = local_fourthAddress

    expiryDate = Math.floor(Date.now() / 1000) + 10000
    startDate = Math.floor(Date.now() / 1000) + 1000
    await deployRabbitholeReceiptContract()
    await deploySampleErc20Contract()
    await deployQuestContract()
    await transferRewardsToDistributor()
  })

  const deployRabbitholeReceiptContract = async () => {
    const ReceiptRenderer = await ethers.getContractFactory('ReceiptRenderer')
    const deployedReceiptRenderer = await ReceiptRenderer.deploy()
    await deployedReceiptRenderer.deployed()

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

  const deployQuestContract = async () => {
    deployedQuestContract = await questContract.deploy(
      deployedSampleErc1155Contract.address,
      expiryDate,
      startDate,
      totalRewards,
      rewardAmount,
      questId,
      deployedRabbitholeReceiptContract.address
    )
    await deployedQuestContract.deployed()
  }

  const deploySampleErc20Contract = async () => {
    deployedSampleErc1155Contract = await sampleERC20Contract.deploy()
    await deployedSampleErc1155Contract.deployed()
  }

  const transferRewardsToDistributor = async () => {
    await deployedSampleErc1155Contract.safeTransferFrom(
      owner.address,
      deployedQuestContract.address,
      rewardAmount,
      100,
      '0x00'
    )
  }

  describe('withdrawRemainingTokens()', async () => {
    it('should transfer all rewards back to owner', async () => {
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])

      // `firstAddress` receive a receipt for the ERC1155 quest
      await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)

      // The owner withdraw all the remaining token

      const beginningContractBalance = await deployedSampleErc1155Contract.balanceOf(
        deployedQuestContract.address,
        rewardAmount
      )
      const beginningOwnerBalance = await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)
      expect(beginningContractBalance.toString()).to.equal('100')
      expect(beginningOwnerBalance.toString()).to.equal('0')
      await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

      const endContactBalance = await deployedSampleErc1155Contract.balanceOf(
        deployedQuestContract.address,
        rewardAmount
      )
      expect(endContactBalance.toString()).to.equal('0')

      // The `firstAddress` user is not able to claim their receipt anymore
      await deployedQuestContract.connect(firstAddress).claim()
    })
  })
})

Tools Used

Manual review + Test

Recommended Mitigation Steps

The withdrawRemainingTokens function should check if there are some receipts minted that have not been claimed yet and transfer only the difference between the current balance and that amount.

Lines of code

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

Vulnerability details

Impact

Each user who completes a quest will receive a receipt. That specific receipt can be redeemed at any point to gain a reward token. A user could decide not to be interested in the reward and sell their receipt to another user in exchange for something. The second user will be able to claim the reward because now it owns the receipt.

The problem exists if the first user will claim the reward before transferring the receipt. The second user will not be able to claim the reward anymore.

Let's make an example

1. alice complete a quest and receive a receipt with tokenId = 1 by calling factory.mintReceipt(...)
2. alice decide to claim the reward by calling quest.claim(...)
3. After receiving the reward token alice decide to sell the receipt on secondary market
4. bob is interested in the reward and exchange the receipt for 1 ETH
5. bob at this point will not be able to receive the reward because it has been already claimed by alice

Proof of Concept

Link to affected code

• RabbitHoleReceipt.sol#L143-L155

Test code

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

describe('Erc20Quest', async () => {
  let deployedQuestContract: Erc20Quest
  let deployedSampleErc20Contract: SampleERC20
  let deployedRabbitholeReceiptContract: RabbitHoleReceipt
  let expiryDate: number, startDate: number
  const mockAddress = '0x0000000000000000000000000000000000000000'
  const mnemonic = 'announce room limb pattern dry unit scale effort smooth jazz weasel alcohol'
  const questId = 'asdf'
  const totalParticipants = 300
  const rewardAmount = 1000
  const questFee = 2000 // 20%
  const totalRewardsPlusFee = totalParticipants * rewardAmount + (totalParticipants * rewardAmount * questFee) / 10_000
  let owner: SignerWithAddress
  let firstAddress: SignerWithAddress
  let secondAddress: SignerWithAddress
  let thirdAddress: SignerWithAddress
  let fourthAddress: SignerWithAddress
  let questContract: Erc20Quest__factory
  let sampleERC20Contract: SampleERC20__factory
  let rabbitholeReceiptContract: RabbitHoleReceipt__factory
  const protocolFeeAddress = '0xE8B17e572c1Eea45fCE267F30aE38862CF03BC84'
  let deployedFactoryContract: QuestFactory
  let questFactoryContract: QuestFactory__factory
  let wallet: Wallet
  let messageHash: string
  let signature: string

  beforeEach(async () => {
    const [local_owner, local_firstAddress, local_secondAddress, local_thirdAddress, local_fourthAddress] =
      await ethers.getSigners()
    questContract = await ethers.getContractFactory('Erc20Quest')
    sampleERC20Contract = await ethers.getContractFactory('SampleERC20')
    rabbitholeReceiptContract = await ethers.getContractFactory('RabbitHoleReceipt')
    questFactoryContract = await ethers.getContractFactory('QuestFactory')
    wallet = Wallet.fromMnemonic(mnemonic)

    owner = local_owner
    firstAddress = local_firstAddress
    secondAddress = local_secondAddress
    thirdAddress = local_thirdAddress
    fourthAddress = local_fourthAddress

    expiryDate = Math.floor(Date.now() / 1000) + 100000
    startDate = Math.floor(Date.now() / 1000) + 1000
    await deployRabbitholeReceiptContract()
    await deploySampleErc20Contract()
    await deployFactoryContract()

    messageHash = utils.solidityKeccak256(['address', 'string'], [firstAddress.address.toLowerCase(), questId])
    signature = await wallet.signMessage(utils.arrayify(messageHash))
    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

    deployedQuestContract = await questContract.attach(contractAddress)
    await transferRewardsToDistributor()
  })

  const deployFactoryContract = async () => {
    deployedFactoryContract = (await upgrades.deployProxy(questFactoryContract, [
      wallet.address,
      deployedRabbitholeReceiptContract.address,
      protocolFeeAddress,
    ])) as QuestFactory
  }

  const deployRabbitholeReceiptContract = async () => {
    const ReceiptRenderer = await ethers.getContractFactory('ReceiptRenderer')
    const deployedReceiptRenderer = await ReceiptRenderer.deploy()
    await deployedReceiptRenderer.deployed()

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

  const deploySampleErc20Contract = async () => {
    deployedSampleErc20Contract = await sampleERC20Contract.deploy(
      'RewardToken',
      'RTC',
      totalRewardsPlusFee * 100,
      owner.address
    )
    await deployedSampleErc20Contract.deployed()
  }

  const transferRewardsToDistributor = async () => {
    const distributorContractAddress = deployedQuestContract.address
    await deployedSampleErc20Contract.functions.transfer(distributorContractAddress, totalRewardsPlusFee * 100)
  }

  describe('claim()', async () => {
    it('should only transfer the correct amount of rewards', async () => {
      // `owner` receive the receipt of the quest
      await deployedRabbitholeReceiptContract.mint(owner.address, questId)
      await deployedQuestContract.start()

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

      // `owner` claim the receipt
      await deployedQuestContract.claim()

      // `owner` has already reedemed the reward and sell the receipt to `secondAddress` user
      deployedRabbitholeReceiptContract.transferFrom(owner.address, secondAddress.address, 1)
      expect(await deployedRabbitholeReceiptContract.balanceOf(owner.address)).to.equal(0)
      expect(await deployedRabbitholeReceiptContract.balanceOf(secondAddress.address)).to.equal(1)

      // This will revert with the error
      // Error: VM Exception while processing transaction: reverted with custom error 'AlreadyClaimed()'
      // Because `owner` has already claimed the receipt
      await deployedQuestContract.connect(secondAddress).claim()

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

Tools Used

Manual review + Test

Recommended Mitigation Steps

There are two possible solutions to the problem

1. Once the user has claimed a receipt, it could be burned to prevent it to be transferred
2. The _beforeTokenTransfer hook of RabbitHoleReceipt could implement a check that prevents a token to be transferred if it has been already claimed