code423n4 commented 1 year ago

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.