RabbitHole Quest Protocol contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

The onlyMinter check in RabbitHoleReceipt is moot, allowing anyone to pass this check to mint.

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

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

Proof of Concept

diff --git a/test/RabbitHoleReceipt.spec.ts b/test/RabbitHoleReceipt.spec.ts
index 1ab2e31..19e9531 100644
--- a/test/RabbitHoleReceipt.spec.ts
+++ b/test/RabbitHoleReceipt.spec.ts
@@ -75,11 +75,12 @@ describe('RabbitholeReceipt Contract', async () => {
     })
   })
 
-  describe('getOwnedTokenIdsOfQuest', () => {
-    it('returns the correct tokenIds', async () => {
-      await RHReceipt.mint(contractOwner.address, 'abc123')
-      await RHReceipt.mint(contractOwner.address, 'def456')
-      await RHReceipt.mint(contractOwner.address, 'eeeeee')
+  describe.only('getOwnedTokenIdsOfQuest', () => {
+    it.only('only minter can mint', async () => {
+      await RHReceipt.connect(firstAddress).mint(contractOwner.address, 'abc123')
+      await RHReceipt.connect(firstAddress).mint(contractOwner.address, 'def456')
+      await RHReceipt.connect(firstAddress).mint(contractOwner.address, 'eeeeee')
+      expect(await RHReceipt.minterAddress()).eq(firstAddress.address)
 
       let tokenIds = await RHReceipt.getOwnedTokenIdsOfQuest('abc123', contractOwner.address)

firstAddress is not a minter, but is able to mint; the above test failed with

  1) RabbitholeReceipt Contract
       getOwnedTokenIdsOfQuest
         returns the correct tokenIds:

      AssertionError: expected '0x3C44CdDdB6a900fa2b585dd299e03d12FA4…' to equal '0x90F79bf6EB2c4f870365E785982E1f101E9…'
      + expected - actual

      -0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
      +0x90F79bf6EB2c4f870365E785982E1f101E93b906

Recommended Mitigation Steps

    modifier onlyMinter() {
        require(msg.sender == minterAddress, "NOT_MINTER");
        _;
    }

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

The onlyMinter check in RabbitHoleTickets is moot, allowing anyone to pass this check to mint.

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

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

Same as the other issue regarding RabbitHoleReceipt but its a different contract without inheritance.

Proof of Concept

diff --git a/test/RabbitHoleTickets.spec.ts b/test/RabbitHoleTickets.spec.ts
index e64e510..f95ede1 100644
--- a/test/RabbitHoleTickets.spec.ts
+++ b/test/RabbitHoleTickets.spec.ts
@@ -33,10 +33,11 @@ describe('RabbitholeTickets Contract', async () => {
     })
   })
 
-  describe('mint', () => {
-    it('mints 5 tokens', async () => {
-      await RHTickets.connect(minterAddress).mint(firstAddress.address, 1, 5, "0x");
-
+  describe.only('mint', () => {
+    it.only('mints 5 tokens', async () => {
+      await RHTickets.connect(firstAddress).mint(firstAddress.address, 1, 5, "0x");
+      expect(await RHTickets.minterAddress()).eq(firstAddress.address)

firstAddress is not a minter, but is able to mint; the above test failed with

1) RabbitholeTickets Contract
       mint
         mints 5 tokens:

      AssertionError: expected '0x3C44CdDdB6a900fa2b585dd299e03d12FA4…' to equal '0x90F79bf6EB2c4f870365E785982E1f101E9…'
      + expected - actual

      -0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
      +0x90F79bf6EB2c4f870365E785982E1f101E93b906

Recommended Mitigation Steps

    modifier onlyMinter() {
        require(msg.sender == minterAddress, "NOT_MINTER");
        _;
    }

Lines of code

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

Vulnerability details

Impact

Admin can drain all token in ERC20Quest by calling withdrawFee multiple times and receive more token than it is entitled to.

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

    function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }

Proof of Concept

diff --git a/test/Erc20Quest.spec.ts b/test/Erc20Quest.spec.ts
index d0626ee..729544a 100644
--- a/test/Erc20Quest.spec.ts
+++ b/test/Erc20Quest.spec.ts
@@ -363,6 +363,8 @@ describe('Erc20Quest', async () => {
 
       await ethers.provider.send('evm_increaseTime', [100001])
       await deployedQuestContract.withdrawFee()
+      // Admin steal more fee by calling withdrawFee again
+      await deployedQuestContract.withdrawFee()
 
       expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
         totalRewardsPlusFee * 100 - 1 * 1000 - 200

The test will fail because the quest contract have less than expected balance (since fee is drawn twice)

Recommended Mitigation Steps

Only allow the admin to call withdrawFee once.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L104-L108

Vulnerability details

Impact

The for loop in Quest.claim is unbounded which might run over block gas limit when there is a lot of reward to claim. The call to claim will revert and user will lose their NFT although they were rewarded.

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L104-L108

        for (uint i = 0; i < tokens.length; i++) {
            if (!isClaimed(tokens[i])) {
                redeemableTokenCount++;
            }
        }

Proof of Concept

quest-protocol/test/Erc20Quest.spec.ts

    it.only('should only transfer the correct amount of rewards', async () => {
      const nRewards = 2000
      for (let i = 0; i < nRewards; i++) {
        await deployedRabbitholeReceiptContract.mint(owner.address, questId)
      }
      await deployedQuestContract.start()

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

      expect(await deployedSampleErc20Contract.balanceOf(owner.address)).to.equal(0)

      const totalTokens = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, owner.address)
      expect(totalTokens.length).to.equal(nRewards)

      expect(await deployedQuestContract.isClaimed(1)).to.equal(false)

      await deployedQuestContract.claim()
      expect(await deployedSampleErc20Contract.balanceOf(owner.address)).to.equal(rewardAmount * nRewards)
      await ethers.provider.send('evm_increaseTime', [-86400])
    })

Recommended Mitigation Steps

Allow user to claim specific reward token id or bound the maximum number of reward per user

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54

Vulnerability details

Impact

Erc1155Quest allow owner to withdraw all balance from the Quest contract anytime, unlike in Erc20Quest where the owner's ability to withdraw token is significantly restricted (owner can only withdraw protocol fee, and remaining token less claimable portion). Although this is an privileged function, the way that its implementation deviated from the other Quest contract Erc20Quest made this Med RIsk since one may expect proper restriction in terms of admin withdrawing reward token when using ERC1155 token as ERC20 token.

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );
    }

Recommended Mitigation Steps

Implement checks as appeared in Erc20Quest

Struct packing

Some struct can be packed tighter to save storage slot, for example:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L19-L24

    struct Quest {
        mapping(address => bool) addressMinted;
        address questAddress;
        uint totalParticipants;
        uint numberMinted;
    }

can be packed into

    struct Quest {
        mapping(address => bool) addressMinted;
        address questAddress;
        uint48 totalParticipants;
        uint48 numberMinted;
    }

To save 2 storage slot, while still allow 2^48 participant and reward to be minted which should be reasonably enough.

Deploy with proxy

Quest contracts are deployed directly from the factory, which can be expensive. Consider deploying proxy to a shared Quest implementation to reduce deployment gas cost. Although this will increase each call to the contract slightly.

Redundant hash parameter

hash_ is redundant since it can be calculated with other input

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219-L222

    function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
        if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint();
        if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
        if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();

Inline recoverSigner into hash calculation

Instead of abi.encodePacked twice, and wasting gas copying the data into memory twice,

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L210-L213

    function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) {
        bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_));
        return ECDSAUpgradeable.recover(messageDigest, signature_);
    }

hash it here altogether, the actual message signed would need to be slightly changed (instead of signing a typed hash, sign the typed message)

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L222

        if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();