Already claimed receipt could be sold on secondary markets but the receiver will not be able to claim the rewards

[code423n4]

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

[c4-judge]

kirk-baird marked the issue as duplicate of #201

[c4-judge]

kirk-baird marked the issue as satisfactory