RabbitHole Quest Protocol contest - libratus's results

Judge has assessed an item in Issue #670 as 2 risk. The relevant finding follows:

[L-03] DoS if address owns too many receipts With time it is viable for users to acquire thousands and tens of thousands of receipts. This may happen as a result of buying receipts for example, which was highlighted as a valid use-case. Moreover, receipts aren't burned when they are used for claiming a reward.

Calculations in getOwnedTokenIdsOfQuest require looping over all of user's tokens. This may lead to denial of service as EVM isn't suitable for big loops.

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

Recommendation: consider using ERC1155 for receipts which will allow tracking user receipts for each quest separately.

Lines of code

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

Vulnerability details

Impact

It should be possible for users to claim rewards after quest ends. However, for ERC1155 Quests owner can withdraw all tokens from the contract as soon as the quest ends by calling withdrawRemainingTokens. Then, if someone tries to claim a reward with a valid receipt, the execution will revert due to insufficient balance.

Proof of Concept

The following test reverts due to 'ERC1155: insufficient balance for transfer'. This test:

• mints a receipt for an address
• starts a quest
• rewinds time to end the quest
• initiates withdrawRemainingTokens by the owner
• claims the reward with the receipt minted in the beginning

diff --git a/test/Erc1155Quest.orig.ts b/test/Erc1155Quest.spec.ts
index 8897a21..4630b70 100644
--- a/test/Erc1155Quest.orig.ts
+++ b/test/Erc1155Quest.spec.ts
@@ -232,6 +232,16 @@ describe('Erc1155Quest', () => {
       await ethers.provider.send('evm_increaseTime', [-1000])
     })
 
+    it('should transfer after contest ends', async () => {
+      await deployedRabbitholeReceiptContract.mint(owner.address, questId)
+      await deployedQuestContract.start()
+      await ethers.provider.send('evm_increaseTime', [10000])
+      await deployedQuestContract.withdrawRemainingTokens(owner.address)
+      await deployedQuestContract.claim()
+      expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(1)
+      await ethers.provider.send('evm_increaseTime', [-10000])
+    })
+
     it('should only transfer the correct amount of rewards', async () => {
       await deployedRabbitholeReceiptContract.mint(owner.address, questId)
       await deployedQuestContract.start()

Tools Used

Manual Review

Recommended Mitigation Steps

Similar to ERC20 Quest, I recommend calculating how many receipts haven't been claimed yet and leaving corresponding amount of tokens on the contract.

    function receiptRedeemers() public view returns (uint256) {
        return questFactoryContract.getNumberMinted(questId);
    }

    /// @dev Withdraws the remaining tokens from the contract. Only able to be called by owner
    /// @param to_ The address to send the remaining tokens to
    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = receiptRedeemers() - redeemedTokens;

        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedTokens,
            '0x00'
        );
    }

Judge has assessed an item in Issue #670 as 2 risk. The relevant finding follows:

[L-04] Changing rabbitholeReceiptContract in QuestFactory will break currently running quests rabbitHoleReceiptContract must be the same in QuestFactory and Quest contracts for quests to function correctly. If there is a mismatch, then factory will mint receipts on a contract different from the one quests check when rewards are claimed.

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

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

QuestFactory has a function that allows changing the receipt contract. If that happens while quests are running, these quests will break.

Recommendation: don't allow changing receipt contract when there are active quests or document this scenario.

Lines of code

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

Vulnerability details

Impact

In the contest description it is mentioned that the protocol is meant to be multi-chain, meaning it can be deployed on multiple blockchains. This opens a vector of attack related to receipt minting.

Minting signature currently holds information about minter address and quest id. This means that if the address/quest pair is the same on multiple blockchains, signature will also be the same.

Consider a quest created for multi-chain dApp (like Uniswap) and deployed on multiple blockchains. User can perform required actions on one blockchain and then reuse the signature to mint receipts on all of them.

Proof of Concept

Consider two copies of the protocol with identical state deployed on two blockchains. If a signature is accepted on one blockchain, it will also work on the other. Nothing in the four checks in mintReceipt will prevent that

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

Tools Used

Manual Review

Recommended Mitigation Steps

The back-end should include chain id along with address and quest id in the signature hash. The hash then can be verified when minting receipts the following way

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

[L-01] Receipt SVG not rendering correctly

toString() isn't called on tokenId_ uint variable when rendering SVG for receipt.

            '<text x="70%" y="40%" class="base" dominant-baseline="middle" text-anchor="middle">RabbitHole Quest Receipt #',
            tokenId_,
            '</text>',

Resulting base64 encoded image will not render as it will contain invalid characters.

Code reference: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L108-L110

[L-02] Users can still mint receipts when quest is paused

Pausing functionality is unclear. After pausing a quest an owner has two options:

• Unpause and let users claim their rewards. In this case it's unclear why it was paused in the first place.
• Keep it paused until expiry. However, that doesn't prevent users from minting receipts. The owner won't be able to withdraw tokens allocated for rewards as it excludes the amount reserved for minted receipts: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L84-L85

[L-03] DoS if address owns too many receipts

With time it is viable for users to acquire thousands and tens of thousands of receipts. This may happen as a result of buying receipts for example, which was highlighted as a valid use-case. Moreover, receipts aren't burned when they are used for claiming a reward.

Calculations in getOwnedTokenIdsOfQuest require looping over all of user's tokens. This may lead to denial of service as EVM isn't suitable for big loops.

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

Recommendation: consider using ERC1155 for receipts which will allow tracking user receipts for each quest separately.

[L-04] Changing rabbitholeReceiptContract in QuestFactory will break currently running quests

rabbitHoleReceiptContract must be the same in QuestFactory and Quest contracts for quests to function correctly. If there is a mismatch, then factory will mint receipts on a contract different from the one quests check when rewards are claimed.

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

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

QuestFactory has a function that allows changing the receipt contract. If that happens while quests are running, these quests will break.

Recommendation: don't allow changing receipt contract when there are active quests or document this scenario.

[N-01] Make Quest contract abstract

Quest is a base class and should be made abstract as there is no intention to deploy it. This will also tidy up the code as empty function definitions can be changed to abstract: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L120-L131

[N-02] Remove "== false" and "== true"

Comparing to true or false is redundant and can be removed:

+++ b/contracts/Quest.sol
@@ -133,7 +133,7 @@ contract Quest is Ownable, IQuest {
     /// @notice Checks if a Receipt token id has been used to claim a reward
     /// @param tokenId_ The token id to check
     function isClaimed(uint256 tokenId_) public view returns (bool) {
-        return claimedList[tokenId_] == true;
+        return claimedList[tokenId_];
     }

+++ b/contracts/QuestFactory.sol
@@ -70,7 +70,7 @@ contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgrade
         if (quests[questId_].questAddress != address(0)) revert QuestIdUsed();
 
         if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) {
-            if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();
+            if (rewardAllowlist[rewardTokenAddress_]) revert RewardNotAllowed();
 
             Erc20Quest newQuest = new Erc20Quest(
                 rewardTokenAddress_,
@@ -218,7 +218,7 @@ contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgrade
     /// @param signature_ The signature of the hash
     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_].addressMinted[msg.sender]) revert AddressAlreadyMinted();
         if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
         if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

[N-03] isPaused check can be moved to the modifier

The following check can be moved into onlyQuestActive or a modifier of its own. This will make the code cleaner.

    function claim() public virtual onlyQuestActive {
        if (isPaused) revert QuestPaused();

Code reference:

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

[N-04] Index correct event properties

To make it easier for external apps to integrate with the contracts, make sure events have indexed properties.

• Account can be indexed:

event Claimed(address account, uint256 amount);

• contractType or startTime can be indexed instead of contractAddress

event QuestCreated(address indexed creator, address indexed contractAddress, string indexed questId, string contractType, address rewardTokenAddress, uint256 endTime, uint256 startTime, uint256 totalParticipants, uint256 rewardAmountOrTokenId);