RabbitHole Quest Protocol contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

The user may not able to claim the eligible amount after the end timestamp if the admin only calls withdrawFee multiple times

Proof of Concept

In the current implementation, the user is expects to claim the reward after the start timestamp

function claim() public virtual onlyQuestActive {

the onlyQuestActive is coded below:

/// @notice Checks if quest has started both at the function level and at the start time
modifier onlyQuestActive() {
	if (!hasStarted) revert NotStarted();
	if (block.timestamp < startTime) revert ClaimWindowNotStarted();
	_;
}

Basically after the startTime the user can the claim the reward.

After the endTime, the admin can claim the fee as well

/// @notice Sends the protocol fee to the protocolFeeRecipient
/// @dev Only callable when the quest is ended
function withdrawFee() public onlyAdminWithdrawAfterEnd {
	IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
}

which calls:

/// @notice Function that calculates the protocol fee
function protocolFee() public view returns (uint256) {
	return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
}

and

/// @notice Call the QuestFactory contract to get the amount of receipts that have been minted
/// @return The amount of receipts that have been minted for the given quest
function receiptRedeemers() public view returns (uint256) {
	return questFactoryContract.getNumberMinted(questId);
}

note the onlyAdminWithdrawAfterEnd modifier:

/// @notice Prevents reward withdrawal until the Quest has ended
modifier onlyAdminWithdrawAfterEnd() {
	if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
	_;
}

The issue is that the owner can call the withdrawFee multiple fee intentionally or even unintentionally drain the token from the ERC20Quest, and the NFT owner cannot fully claim the reward they are eligible for.

Tools Used

Manual Review

Recommended Mitigation Steps

Only let the admin withdrawal the fee onces and the withdrawable fee to 0 after the initial withdraw.

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

Lines of code

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

Vulnerability details

Impact

Buyer on secondary NFT market can lose fund if they buy a NFT that is already used to claim the reward

Proof of Concept

Let us look closely into the Quest.sol#claim function

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

After the NFT is used to claim, the _setClaimed(token) is called to mark the NFT as used to prevent double claiming.

/// @notice Marks token ids as claimed
/// @param tokenIds_ The token ids to mark as claimed
function _setClaimed(uint256[] memory tokenIds_) private {
	for (uint i = 0; i < tokenIds_.length; i++) {
		claimedList[tokenIds_[i]] = true;
	}
}

The NFT is also tradeable in the secondary marketplace, I woud like to make a reasonable assumption that user wants to buy the NFT because they can use the NFT to claim the reward, which means after the reward is claimed, the NFT lose value.

Consider the case below:

1. User A has 1 NFT, has he can use the NFT to claim 1 ETH reward.
2. User A place a sell order in opensea and sell the NFT for 0.9 ETH.
3. User B see the sell order and find it a good trae, he wants to buy the NFT.
4. User B submit a buy order, User A at the same time submit the claimReward transaction.
5. User A's transaction executed first, reward goes to User A, then User B transaction executed, NFT ownership goes to User B, but user B find out that the he cannot claim the reward becasue the reward is already claimed by User A.

User A can intentionally front-run User B's buy transaction by monitoring the mempool in polygon using the service

https://www.blocknative.com/blog/polygon-mempool

Or it could be just two user submit transaction at the same and User A's claim transaction happens to execute first.

Tools Used

Manual Review

Recommended Mitigation Steps

Disable NFT transfer and trade once the NFT is used to claim the reward.

Lines of code

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

Vulnerability details

Impact

Signature schema in QuestFactory is too simple

Proof of Concept

In the current implementation of the QueryFactory, the signature schema is too Simple

/// @dev recover the signer from a hash and signature
/// @param hash_ The hash of the message
/// @param signature_ The signature of the hash
function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) {
	bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_));
	return ECDSAUpgradeable.recover(messageDigest, signature_);
}

/// @dev mint a RabbitHole Receipt. Note: this contract must be set as Minter on the receipt contract
/// @param questId_ The id of the quest
/// @param hash_ The hash of the message
/// @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 (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_);
}

Only the questId and msg.sender is included, the chain id is missing, the nonce is missing, the timestamp is missing as well, meaning the signature does not expire.

Chain id is missing -> potential cross-chain transaction replay. Timestamp is missing -> the signature does not expire. for example, the a transaction mintReceipt with msg.sender signature is issued, this transaction is pending, however, the owner realize that he should change the claimSignerAddress, he issue another transaciton trying to change the claimSignerAddress,

Because the the signature does not expire, after a period of time, the mintReceipt landed first before the change claimSignerAddress transaction is landed and because old claimSIgnerAddress is used, the NFT still mint to old claimSignerAddress.

If the transaction has deadline check, by the time the mintReceipt landed, the signature expires and revert, which is revert, then the change old claimSignerAddress transaction comes in and the claimSignerAddress is updated.

The nonce incremented nonce is missing -> transaction may be vulnerable to replay if the smart contract is upgraded, the QuestFactory is certainly a upgradeable contract.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol make the signature schema more sophicated and including nonce and timesetamp and chain id.