RabbitHole Quest Protocol contest - cccz's results

Lines of code

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

Vulnerability details

Impact

After Erc20Quest ends, anyone can call Erc20Quest.withdrawFee to send protocolFee() to protocolFeeRecipient. However, Erc20Quest.withdrawFee can be called multiple times by anyone, thus depleting the reward tokens in the contract and preventing users from receiving their rewards after the end.

    function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }
...
    modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

Consider that when Erc20Quest ends, the contract has 1000 reward tokens, 900 of which are rewards and 100 are protocolFee. Since Erc20Quest.withdrawFee can be called multiple times by anyone, a malicious user can call withdrawFee 10 times to deplete the reward tokens in the contract, thus preventing the user from claiming the rewards

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider only allowing Erc20Quest.withdrawFee to be called once

Lines of code

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

Vulnerability details

Impact

Erc20Quest.withdrawRemainingTokens will be called by the owner after the end of Erc20Quest and will leave unclaimedTokens (mintReceipt has been called but not claim) and protocolFee in the contract, the rewards of users who did not call mintReceipt are not left in the contract.

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

However, mintReceipt can still be called after this point, and the user can claim the remaining rewards in the contract, thus preventing other users from claiming rewards

    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();
        if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

        quests[questId_].addressMinted[msg.sender] = true;
        quests[questId_].numberMinted++;
        emit ReceiptMinted(msg.sender, questId_);
        rabbitholeReceiptContract.mint(msg.sender, questId_);
    }

Consider the following scenario, where alice and bob have both completed Erc20Quest and received signatures. alice calls mintReceipt to mint an NFT for himself. After the Erc20Quest ends, the owner calls withdrawRemainingTokens to leave alice's reward in the contract (since bob did not call mintReceipt, his reward will not be left) and protocolFee At this point bob calls mintReceipt and claim, he will receive the reward left for alice in the contract, and alice will not receive the reward due to insufficient contract balance (or take out protocolFee as reward)

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219-L229 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87

Tools Used

None

Recommended Mitigation Steps

Add endTime to the Quest structure and set it in createQuest and check it in mintReceipt

    struct Quest {
        mapping(address => bool) addressMinted;
        address questAddress;
        uint totalParticipants;
        uint numberMinted;
+      uint256 endTime;
    }
...
            quests[questId_].questAddress = address(newQuest);
            quests[questId_].totalParticipants = totalParticipants_;
+          quests[questId_].endTime = endTime_;
...
    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 (quests[questId_].endTime <= block.timestamp ) revert AlreadyEnded();

Lines of code

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

Vulnerability details

Impact

After Erc20Quest ends, withdrawFee collects protocolFee based on the number of users who have called mintReceipt, and anyone can call withdrawFee. This allows a malicious user to call withdrawFee after Erc20Quest ends and then call mintReceipt and claim to collect the reward, which results in less protocolFee. For example, in an Erc20Quest where the reward per user is 100, the questFee is 2%. alice/bob has the signature, and alice calls mintReceipt, at which point protocolFee is 2. At the end of the Erc20Quest, bob calls withdrawFee and then calls mintReceipt and claim to collect the reward, so that bob's 100 reward tokens do not need to be charged with protocolFee

    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();
        if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

        quests[questId_].addressMinted[msg.sender] = true;
        quests[questId_].numberMinted++;
        emit ReceiptMinted(msg.sender, questId_);
        rabbitholeReceiptContract.mint(msg.sender, questId_);
    }

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider disabling calling mintReceipt after Erc20Quest ends

struct Quest {
    mapping(address => bool) addressMinted;
    address questAddress;
    uint totalParticipants;
    uint numberMinted;
+      uint256 endTime;
}
...
        quests[questId_].questAddress = address(newQuest);
        quests[questId_].totalParticipants = totalParticipants_;
+          quests[questId_].endTime = endTime_;
...
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 (quests[questId_].endTime <= block.timestamp ) revert AlreadyEnded();

Lines of code

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

Vulnerability details

Impact

Erc20Quest.withdrawRemainingTokens leaves rewards in the contract for users who have called mintReceipt but have not called claim. However, Erc1155Quest.withdrawRemainingTokens disregards these users and simply takes all the rewards left in the contract.

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

This results in users in Erc1155Quest who have called mintReceipt but not claim losing their rewards.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider leaving the rewards of users who have called mintReceipt but not claim in Erc1155Quest.withdrawRemainingTokens.

Lines of code

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

Vulnerability details

Impact

After Erc20Quest ends, withdrawRemainingTokens can be called by the owner and withdrawFee can be called by anyone. Bur even if withdrawFee has been called, withdrawRemainingTokens still leaves protocolFee amount of reward tokens in the contract. This allows a malicious user to call Erc20Quest.withdrawFee to front-run Erc20Quest.withdrawRemainingTokens, thereby locking the protocolFee amount of reward tokens in the contract

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

NOTE: due to another vulnerability, Erc20Quest.withdrawFee can be called multiple times and the reward tokens are not actually locked in the contract, but this allows protocolFeeRecipient to receive double protocolFee

Consider the following scenario, where an Erc20Quest ends with 1000 reward tokens left in the contract and the protocolFee is 100. The owner intends to call Erc20Quest.withdrawRemainingTokens to take 900 reward tokens and leave 100 reward tokens as protocolFee The malicious user calls Erc20Quest.withdrawFee to front-run Erc20Quest.withdrawRemainingTokens and send 100 reward tokens to protocolFeeRecipient When the owner calls Erc20Quest.withdrawRemainingTokens, there are 900 reward tokens left in the contract. As a result, Erc20Quest.withdrawRemainingTokens takes 800 reward tokens, leaving 100 reward tokens in the contract.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Change to

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

By resetting the questFee , we can avoid withdrawRemainingTokens from leaving protocolFee again, and we can avoid collecting protocolFee multiple times

Lines of code

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

Vulnerability details

Impact

In the mintReceipt function, the contract will mint an NFT for the user if the user provides the correct signature. Here the signature contains msg.semder and questId_.

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

However, considering that this is a multi-chain project, since the signature does not contain the chainId, a malicious user can replay the signature on other chains to get the reward. Consider that the project exists on ethereum and polygon and uses the same claimSignerAddress. User A completes a quest with questId_ == 1 on ethereum and gets a signature. User A calls QuestFactory.mintReceipt on ethereum to mint an NFT for himself. Meanwhile, User A can call QuestFactory.mintReceipt on polygon to mint an NFT for himself without completing any quest.

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider including the chainId in the signature