Quest: Potential out-of-gas in claim

[code423n4]

Lines of code

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

Vulnerability details

Note: I can't seem to edit my submitted QA report, so I am filing this as a Med, but expect it to be downgraded!

Quest#claim claims all rewards for all owned receipt tokens owned by the caller. It's possible in certain situations for this to exceed the block gas limit, preventing the end user from claiming rewards:

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

Proof of concept

• Calling claim() costs about ~125,000 gas, based on the gas report.
• The current block gas limit is 30 million gas.
• claim could exceed the block gas limit of 30 million if a user owns more than 240 tokens and calls claim().

This is probably not very likely, but not out of the realm of possibility, especially since receipts are intended to trade on the secondary market. The likelihood depends in part on the value of the underlying rewards and how many tokens will be issued per quest.

There is a potential workaround: end users can transfer tokens to another wallet to claim less than their full balance at once.

Suggestion:

Add a separate claim(uint256[] tokenIds) function that allows the user to claim a specific set of tokens by ID. In the worst case, a user can call this function multiple times to process their tokens in batches.

However, be careful to ensure this implementation handles duplicate token IDs in the input array: the existing logic in claim() can't be directly applied to a user-provided array of tokens, since it sets tokens claimed after counting them up.

[c4-judge]

kirk-baird marked the issue as duplicate of #135

[c4-judge]

kirk-baird marked the issue as satisfactory