Unbounded loop when claiming the NFT reward

[code423n4]

Lines of code

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

Vulnerability details

Impact

Unbounded loop when claiming the NFT reward.

Proof of Concept

The code contains two unbounded loop that can consume all gas and revert the tranasction when claming the reward

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

The first one is:

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

iterate over the nft owned by msg.sender given the questId,

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

        uint[] memory filteredTokens = new uint[](foundTokens);
        uint filterTokensIndexTracker = 0;

        for (uint i = 0; i < msgSenderBalance; i++) {
            if (tokenIdsForQuest[i] > 0) {
                filteredTokens[filterTokensIndexTracker] = tokenIdsForQuest[i];
                filterTokensIndexTracker++;
            }
        }
        return filteredTokens;
    }

the owner coulld have 10 NFT but purchase 100 NFT, the loop needs to run 110 times.

The second unbounded loop is when we mark all NFT as used, the same amout of loop needs to run again.

_setClaimed(tokens);

Which can be very gas costly.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol set upper limit for the generated and claimable NFT number

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-sponsor]

waynehoover marked the issue as disagree with severity

[waynehoover]

Same comment as #403 (comment)

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked issue #552 as primary and marked this issue as a duplicate of 552

[kirk-baird]

Discussion about severity can be seen in #403