Users might be unable to claim receipt because of unbounded loop

[code423n4]

Lines of code

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

Vulnerability details

Impact

When users claim from any Quest contract, it will call to function getOwnedTokenIdsOfQuest() of RabbitHoleReceipt contract to get all token ids for that quest owned by caller. Function getOwnedTokenIdsOfQuest() will loop through all receipt owned by caller and filter token for that quest

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;

    // @audit unbounded loop
    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;
}

For each quest, normally each user can only mint 1 receipt, therefore only have 1 token per quest, but users can purchase more receipt in open market and have more than 2 tokens per quest. In addition, RabbitHoleReceipt contract hold receipts for all quests.

So if attacker somehow can create a quest where value of each receipt is neglectible or zero, he can transferred all these spam receipts to victim, making the claim call of victim more expensive in term of gas cost. Technically, it is even possible to DOS victim claim when the gas cost is break the block gas limit.

Proof of Concept

Function getOwnedTokenIdsOfQuest() is used in Quest.claim() function

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

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

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

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using a data structure that can query all token for a quest quickly. For example, a mapping of owner to questId to array.

[c4-judge]

kirk-baird marked the issue as duplicate of #135

[c4-judge]

kirk-baird marked the issue as satisfactory