RabbitHole Quest Protocol contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A protocol fee collector can withdraw MANY TIMES of protocol fee.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

A protocol fee collector can withdraw MANY TIMES of protocol fee because:

1. The collector calls the following function to collect protocol fee. There is no check whether he/she has already collected the protocol fee before. As a result, she/he can call it again and again until there is no sufficient balance left to cover another round collection of protocolFee().

function withdrawFee() public onlyAdminWithdrawAfterEnd {
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    }

2. It is possible that the host noticed this, and then rush to run withdrawRemainingTokens() to mitigate it, unfortunately, withdrawRemainingTokens() has the same problem: it does not check whether the protocol collector has collected the protocol fee or not, and leaves the amount of protocolFee() in the contract after the call.

3. As this point, the protocol collector can make another call to withdrawFee() to collect the last batch of protocolFee().

Tools Used

remix

Recommended Mitigation Steps

We can introduce a state boolean variable isFeeCollected to check whether protocol fee has been paid or not and reimplement withdrawFee() and withdrawRemainingTokens() as follows:

function withdrawFee() public onlyAdminWithdrawAfterEnd {
        if(isFeeCollected) revert protocolFeeHasBeenCollected();

        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
        isFeeCollected = true;
}


function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint remainingFee =  isFeeCollected ? 0: protocolFee();

        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - remainingFee - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Winners of ERC1155Quest might not be able to claim rewards after the quest end if the host calls the function withdrawRemainingTokens() right after the quest end.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Winners of ERC1155Quest might not be able to claim rewards after the quest end if the host calls the function withdrawRemainingTokens() right after the quest end. This is because withdrawRemainingTokens() will withdraw all remaining tokens in the contract, including those that have not yet been claimed:

function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );
    }

withdrawRemainingTokens() should have left the unclaimed tokens in the contract so that winners can claim them. Otherwise they lost fund.

Tools Used

Remix

Recommended Mitigation Steps

Similar to its counterpart for ERC20Quest, leave the unclaimed tokens in the contract for the winners to claim. If necessary, implement another function called withdrawUnclaimedTokens() that will be called after some claimDeadline expires.

QA1. The NatSpec here is wrong, this is not ERC165, it is about ERC2981

@dev See {IERC165-royaltyInfo}

The correction is

@dev See {IERC2981Upgradeable-royaltyInfo}

QA2. When admins/owner set important address parameters, it is important to do a zero address check and also X != address(this) check to ensure not losing funding due to mistakes. Timelock might also be introduced to ensure further security.

1. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L71-L73
2. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L77-L79
3. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L83-L86
4. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L65-L67

QA3. The withdrawFee() has the risk of sending protocol fee to a dead address:

https://github.com/rabbitholegg/quest-protocol/blob/9ef32907788dde6c42990ee5dde8f53caeaba474/contracts/Erc20Quest.sol#L102-L104

The losing of fund can happen in the following way:

1. A dead address is set as the protocol recipient by the owner of QuestFactory by mistake or by a compromised owner on purpose.

2. The malicious user calls withdrawFee() and then protocol fee is sent to the dead address.

3. Nobody can retrieve the protocol fee from the dead address.

Mitigation: add a modifier so that only the protocol recipient can call withdrawFee(). In this way, no fund will be sent to a dead address.

QA4. The following modifier name is not consistent with its implementation logic. Better change it to onlyAfterQuestEnd().

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

 modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

QA5. Signature replay attack is possible for mintReceipt() such that a malicious user can mint receipts for free. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219

Analysis: This is because the signature only signs for (winnerAddress, questId), so the signature can be reused for another contract and another blockchain to mint free receipts.

For example, if a quest was very successful on Ethererum and then it is deployed on a new blockchain Polygon, one can reuse the signature signed on the Ethereum to get free receipt on Polygon by calling mintReceipt with the same signature.

Mitigation: To prevent signature replay attack, include nonce, contract address and blockchain ID in the hash as well.

QA6. royaltyFee should never be greater than 100% of the salePrice (represented as 10_000). Otherwise, the seller gets nothing from a sale or causing the selling to fail.

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L66-L69

The mitigation is to add such a check:

 function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
        if(royaltyFee > 10_000) revert royaltyFeeTooLarge();   // audit: add this check 

        royaltyFee = royaltyFee_;
        emit RoyaltyFeeSet(royaltyFee_);
    }

QA7. If a sponsor sends the wrong ERC20/ERC1155 to the quest contract (e.g. due to miscommunication), for example, instead of sending USDC, the sponsor sends USDT as reward tokens, then they will be locked in the contract forever since there is no way to withdraw arbitrary ERC20/ERC1155 tokens.

Mitigation: introduce generic withdrawERC20(address tokenAddress) and withdraw1155(address tokenAddress)so that the host can withdraw arbitrary ERC20/ERC1155 tokens.

QA8. The mint() function of the RabbitHoleReceipt violates the separation of duty rule. The minter of one quest can mint tokens for another quest. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98-L104 For example, Alice is the minter for quest A, so she has the role of minter, then she can also call mint(to, B) to mint receipts for quest B, as a result, she can claim rewards from quest B that has nothing to do with quest A.

Mitigation: each quest should has its own minter. A minter of quest A should be not allowed to mint receipts for quest B.

QA9. QuestIdCount is always the total number of Quests + 1. This is confusing.

This is because it is initialized to 1 in the constructor: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L49

 questIdCount = 1;

and it increases by 1 each time a new quest is launched: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L101 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L132

 ++questIdCount;

Mitigation: don't bother to initialize it to 1 in the constructor, so it will be zero initially.

QA10. It is important to declare a uint _gap[50] state variable for the following upgradable implementation contracts so that when they are upgraded with the introduction of new state variables, other inheriting contracts will not be disturbed.

1. QuestFactory
2. RabbitHoleReceipt
3. RabbitHoleTickets

G1. There is no need for the isClaimed() function: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L104-L108

We can save gas by check claimedList directly:

uint len = tokens.length;
for (uint i; i < len; unchecked{i++}) {
            if (!claimedList) {
                redeemableTokenCount++;
            }
}

G2. onlyOwner is used in both child method and parent method, eliminating one can save gas:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L150

onlyOwner is used for both withdrawRemainingTokens() and its super method which is also called in withdrawRemainingTokens():

 function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

One modifier onlyOwner can be eliminated to save gas here.

G3: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L185 To save gas here, just needs to assign royaltyRecipient to receiver. There is no need for the stack variable royaltyPayment and the return statement. Further gas is saved by checking whether royaltyFee !=0.

function royaltyInfo(
        uint256 tokenId_,
        uint256 salePrice_
    ) external view override returns (address receiver, uint256 royaltyAmount) {
        require(_exists(tokenId_), 'Nonexistent token');
 
        if(royaltyFee !=0) royaltyAmount = (salePrice_ * royaltyFee) / 10_000;
        receiver = royaltyRecipient;
    }

G4. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L109-L135 Much gas can be saved for getOwnedTokenIdsOfQuest() by doing two things: 1) Using only ONE for-loop to calculate the final filteredTokens; 2) Revising the length of array filteredTokens directly using assembly.

 function getOwnedTokenIdsOfQuest(
        string memory questId_,
        address claimingAddress_
    ) public view returns (uint[] memory  filteredTokens) {
        uint msgSenderBalance = balanceOf(claimingAddress_);
         filteredTokens = new uint[](msgSenderBalance);
        uint foundTokens = 0;

        for (uint i; i < msgSenderBalance;) {
            uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);

            if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
                filteredTokens[i] = tokenId;
                foundTokens++;
            }

            unchecked{++i;}
        }
   
       // @save the size of the array
       assembly{
               mstore(filteredTokens, foundTokens)
      }
}

G5. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L162 We can save gas here by not performing a zero address check for QuestFactoryContract. Instead, the zero address check should be performed by less called functions: the constructor and setQuestFactory(). In this way, we know for sure that QuestFactoryContract != addresss(0) when tokenURI() is called.

G6. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96-L118 The claim() function calls _setClaimed(tokens) to iterate each token in tokens and blindly set it to true regardless of its truth value. This is a waste of gas. We can leverage the for-loop earlier (used to calculate redeemableTokenCount) to save gas since we can avoid some blind writes here and another for-loop.

uint256 redeemableTokenCount = 0;
        for (uint i = 0; i < tokens.length; i++) {
            if (!claimedList[tokens[i]]) {
                redeemableTokenCount++;
                claimedList[tokens[i]] == true;   // @audit: only those false need to be changed to true
            }
        }

G7. Use uint for questId can save much gas. Currently, string is used for questId.

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

Suggestion: using string for questId will waste much gas especially when it is used in mappings. Use uint instead and use OpenZeppelin's counter.sol for generating questId. Introduce questTitle to store the title information. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Counters.sol

G8. It can be gas saving to eliminate the following checks (or later checks) since they will be checked again in later code. a) https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Lendgine.sol#L87