RabbitHole Quest Protocol contest - mookimgo's results

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/RabbitHoleTickets.sol#L48

Vulnerability details

Impact

The onlyMinter modifier actually do nothing, leads to no check of mint process.

Attacker can steal all reward token (erc20 and erc1155) by minting RabbitHoleReceipt as he wish, to many accounts he controlled. And then steal the reward tokens by simply calling claim of Quest.

Similarly RabbitHoleTickets's onlyMinter is also vulnerable.

Proof of Concept

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

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

This equal does not check whether it's true or false, which actually means do nothing.

This can be demonstrated by this simple contract test:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;

contract TEST{
    address public minterAddress;
    modifier onlyMinter() {
        msg.sender == minterAddress;
        _;
    }
    constructor(){
        minterAddress = msg.sender;
    }
    function test() onlyMinter external{

    }
}

The address (not minterAddress) can successfully call test function.

Anyone can call mint function, so the attacker can create many RabbitHoleReceipt NFTs, and call claim of Quest to steal all reward tokens.

Tools Used

Human code reading

Recommended Mitigation Steps

change to:

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

git diff provided below:

diff --git a/contracts/RabbitHoleReceipt.sol b/contracts/RabbitHoleReceipt.sol
index 085b617..e561378 100644
--- a/contracts/RabbitHoleReceipt.sol
+++ b/contracts/RabbitHoleReceipt.sol
@@ -56,7 +56,7 @@ contract RabbitHoleReceipt is
     }

     modifier onlyMinter() {
-        msg.sender == minterAddress;
+        require(msg.sender == minterAddress, "not minter");
         _;
     }

diff --git a/test/RabbitHoleReceipt.spec.ts b/test/RabbitHoleReceipt.spec.ts
index 1ab2e31..bbac00c 100644
--- a/test/RabbitHoleReceipt.spec.ts
+++ b/test/RabbitHoleReceipt.spec.ts
@@ -51,7 +51,10 @@ describe('RabbitholeReceipt Contract', async () => {

       expect(await RHReceipt.balanceOf(firstAddress.address)).to.eq(1)
       expect(await RHReceipt.questIdForTokenId(1)).to.eq('def456')
-    })
+    });
+    it('mint should revert for non-minter', async () => {
+      await expect(RHReceipt.connect(firstAddress).mint(firstAddress.address, 'def456')).to.be.revertedWith("not minter");
+    });
   })

   // todo, this needs a quest created
@@ -77,9 +80,9 @@ 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')
+      await RHReceipt.connect(minterAddress).mint(contractOwner.address, 'abc123')
+      await RHReceipt.connect(minterAddress).mint(contractOwner.address, 'def456')
+      await RHReceipt.connect(minterAddress).mint(contractOwner.address, 'eeeeee')

       let tokenIds = await RHReceipt.getOwnedTokenIdsOfQuest('abc123', contractOwner.address)

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L99 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L117

Vulnerability details

Impact

Attacker can make a specific user unable to claim (or pay a lot of gas fee) by sending him a lot of claimed RabbitHoleReceipt. This will make RabbitHoleReceipt's getOwnedTokenIdsOfQuest function iterate through a lot of useless tokens, causing a lot of gas, incurring huge fee to the victim user. Even worse, if the required gas is bigger than block gaslimit, the victim can not claim his reward.

Proof of Concept

    /// @dev get the token ids for a quest owned by an address
    /// @param questId_ the quest id
    /// @param claimingAddress_ the address claiming to own the tokens
    function getOwnedTokenIdsOfQuest(
        string memory questId_,
        address claimingAddress_
    ) public view returns (uint[] memory) {
        uint msgSenderBalance = balanceOf(claimingAddress_);
        uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
        uint foundTokens = 0;

        for (uint i = 0; i < msgSenderBalance; i++) {
            uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
            if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
                tokenIdsForQuest[i] = tokenId;
                foundTokens++;
            }
...

This code will iterate all tokens hold by the msg.sender, including useless receipts that have been claimed. As long as the msgSenderBalance is big enough, it will cause a huge amount of gas for user, which can be achieved by malicious attacker sending a lot of RabbitHoleReceipt to the victim.

Tools Used

Human code reading

Recommended Mitigation Steps

Change the claim function to add a tokenIds list variable, to specify which token ids to claim.

Do not use loop to iterate user balance NFTs.