RabbitHole Quest Protocol contest - carlitox477's results

Lines of code

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

Vulnerability details

DOS risk if enough tokens are minted in Quest.claim

Description

claim function can be summaraized in next steps:

1. Check that the quest is active
2. Check the contract is not paused
3. Get tokens corresponding to msg.sender for questId using rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest: DOS
4. Check that msg.sender owns at least one token
5. Count non claimed tokens
6. Check there is at least 1 unclaimed token
7. Calculate redeemable rewards: _calculateRewards(redeemableTokenCount);
8. Set all token to claimed state
9. Update redeemedTokens
10. Emit claim event

The problem with this functions relays in its dependency on RabbitHoleReceipt.getOwnedTokenIdsOfQuest. It's behaviour can be summarized in next steps:

1. Get queried balance (claimingAddress_)
2. Get claimingAddress_ owned tokens
3. Filter tokens corresponding to questId_
4. Return token of claimingAddress_ corresponding to questId_

If a user takes part in many quests and gets lot's of tokens, the claim function will eventually reach block gas limit. Therefore, it will be unable to claim their tokens.

If a user actively participates in multiple quests and accumulates a large number of tokens, the claim function may eventually reach the block gas limit. As a result, the user may be unable to successfully claim their earned tokens.

Impact

It can be argued that function ERC721.burn can address the potential DOS risk in the claim process. However, it is important to note the following limitations and drawbacks associated with this approach:

1. Utilizing ERC721.burn does not prevent the user from incurring network fees if a griefer, who has already claimed their rewards, sends their tokens to the user with the intent of causing a DOS and inducing loss of gas.
2. If the user has not claimed any rewards from their accumulated tokens, they will still be forced to burn at least some of their tokens, resulting in a loss of these assets.

POC

Griefing

1. Alice has took part in many quests, and want to recieve her rewards, so she call Quest.claim() function
2. Bob also has already claimed many rewards from many quest, and decide to frontrun alice an send her all his tokens to DOS her
3. Alice run out of gas, she lose transaction fees.

Lose of unclaimed rewards

1. Alice always takes part in many quest, but never claim her rewards. She trust RabbitHole protocol and is waiting to have much more rewards to claim in order to save some transaction fees
2. When Alice decide to call claim function she realize that she has run out of gas.

Then, Alice can only burn her some of her tokens to claim at least some rewards.

Code

Code sample

Mittigation steps

If a user can sent a token list by parameter to claim function, then this vector attack can be mitigated.

To do this add next function to RabbitHoleReceipt.sol:

function checkTokenCorrespondToQuest(uint tokenId, string memory questId_) external view returns(bool){
    return keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_));
}

Then modify Quest.claim:

// Quest.sol
-   function claim() public virtual onlyQuestActive {
+   function claim(uint[] memory tokens) public virtual onlyQuestActive {
        if (isPaused) revert QuestPaused();

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

        // require(tokens.length > 0)
        if (tokens.length == 0) revert NoTokensToClaim();

        uint256 redeemableTokenCount = 0;
        for (uint i = 0; i < tokens.length; i++) {
            // Check that the token correspond to this quest
            require(rabbitHoleReceiptContract.checkTokenCorrespondToQuest(tokens[i],questId))

-           if (!isClaimed(tokens[i])) {
+           if (!isClaimed(tokens[i]) && rabbitHoleReceiptContract.checkTokenCorrespondToQuest(tokens[i],questId)) {
                redeemableTokenCount++;
            }
        }

        if (redeemableTokenCount == 0) revert AlreadyClaimed();

        uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
        _setClaimed(tokens);
        _transferRewards(totalRedeemableRewards);
        redeemedTokens += redeemableTokenCount;

        emit Claimed(msg.sender, totalRedeemableRewards);
    }

[NC] Quest.constructor: endTime_ > block.timestamp check is redundant

Currently

// Quest constructor
    if (endTime_ <= block.timestamp) revert EndTimeInPast();
    if (startTime_ <= block.timestamp) revert StartTimeInPast();
    if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();

This means thats next checks are done:

1. endTime_ > block.timestamp
2. startTime_ > block.timestampp
3. endTime_ > startTime_

From 2 and 3: endTime_ > startTime_ > block.timestampp Then 1 can be omitted. Line if (endTime_ <= block.timestamp) revert EndTimeInPast(); should be removed, as well as EndTimeInPast() error declaration.}

[Non-critical] Quest.start, Quest.pause, Quest.unPause should emit events

Given that this calls are significant for multiple core contract functionalities, emiting events when this function are called informing the quest start and its pause/unpause would be advisable.

[Non-critical] Quest.pause, Quest.unPause, Quest.start can be called at any time

Quest.pause should check that isPaused = false, while Quest.unPause should check that isPaused = true in order to execute a meaningful transaction.

Quest.start should check that the quest has not already started.

[Low] Erc20Quest.withdrawFee allows anyone to call this function

Modifier onlyAdminWithdrawAfterEnd does not check the admin/owner, meaning anyone can call withdrawFee function.

Mittigation steps: Add onlyOwner modifier or check ownership in onlyAdminWithdrawAfterEnd modifier.

[Non-critical] RabbitHoleTickets.royaltyInfo(uint256,uint256) remove first parameter

tokenId_ is not used inside the function, then it can be removed

function royaltyInfo(
-   uint256 tokenId_,
    uint256 salePrice_
) external view override returns (address receiver, uint256 royaltyAmount) {
    uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;
    return (royaltyRecipient, royaltyPayment);
}

[Low] RabbitHoleTickets intitalizer and setters should check non zero address assignation

    function initialize(
        address ticketRenderer_,
        address royaltyRecipient_,
        address minterAddress_,
        uint royaltyFee_
    ) public initializer {
        __ERC1155_init('');
        __Ownable_init();
        __ERC1155Burnable_init();
+       require(ticketRenderer_ != address(0), ERROR_ADDRESS_ZERO_ASSIGNATION());
+       require(royaltyRecipient_ != address(0), ERROR_ADDRESS_ZERO_ASSIGNATION());
+       require(minterAddress_ != address(0), ERROR_ADDRESS_ZERO_ASSIGNATION());
        royaltyRecipient = royaltyRecipient_;
        minterAddress = minterAddress_;
        royaltyFee = royaltyFee_;
        TicketRendererContract = TicketRenderer(ticketRenderer_);
    }
//...

    function setTicketRenderer(address ticketRenderer_) public onlyOwner {
+       require(ticketRenderer_ != address(0), ERROR_ADDRESS_ZERO_ASSIGNATION());
        TicketRendererContract = TicketRenderer(ticketRenderer_);
    }
//...

    function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
+       require(royaltyRecipient_ != address(0), ERROR_ADDRESS_ZERO_ASSIGNATION());
        royaltyRecipient = royaltyRecipient_;
    }
//...
    function setMinterAddress(address minterAddress_) public onlyOwner {
+       require(minterAddress_ != address(0), ERROR_ADDRESS_ZERO_ASSIGNATION());
        minterAddress = minterAddress_;
        emit MinterAddressSet(minterAddress_);
    }

[Non-critical] RabbitHoleTickets setters should check if value are really changed

    function setTicketRenderer(address ticketRenderer_) public onlyOwner {
        require(ticketRenderer_ != address(TicketRendererContract));
        TicketRendererContract = TicketRenderer(ticketRenderer_);
    }
    //...

    function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
+       require(royaltyRecipient != royaltyRecipient_);
        royaltyRecipient = royaltyRecipient_;
    }

    function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
+       require(royaltyFee != royaltyFee_);
        royaltyFee = royaltyFee_;
        emit RoyaltyFeeSet(royaltyFee_);
    }
//...

    function setMinterAddress(address minterAddress_) public onlyOwner {
+       require(minterAddress_ != minterAddress);
        minterAddress = minterAddress_;
        emit MinterAddressSet(minterAddress_);
    }

[Low] Lack of event emission when setTicketRenderer and setRoyaltyRecipient are called

These seem important methods given they modifed state variables, it would be advisable to emit and event indicating the new values.

Quest.constructor: Remove endTime_ > block.timestamp check and EndTimeInPast error declaraion to save gas

Currently

// Quest constructor
    if (endTime_ <= block.timestamp) revert EndTimeInPast();
    if (startTime_ <= block.timestamp) revert StartTimeInPast();
    if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();

This means thats next checks are done:

1. endTime_ > block.timestamp
2. startTime_ > block.timestampp
3. endTime_ > startTime_

From 2 and 3: endTime_ > startTime_ > block.timestampp Then 1 can be omitted, saving gas. Line if (endTime_ <= block.timestamp) revert EndTimeInPast(); should be removed, as well as EndTimeInPast() error declaration.

Erc20Quest.maxTotalRewards and Erc20Quest.maxProtocolReward can be immutable in order to save gas

Given than totalParticipants, rewardAmountInWeiOrTokenId and questFee are immutable, and current function implementations, these values can calculated during construction to save gas each time they are used inside the contract. Then:

1. Remove Erc20Quest.maxTotalRewards and Erc20Quest.maxProtocolReward
2. Add immutable variables and modify constructor:

// Erc20Quest.sol
+   uint256 public immutable maxTotalRewards;
+   uint256 public immutable maxProtocolReward;
+   uint256 internal immutable sumMaxRewards;
    constructor(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountInWeiOrTokenId_,
        string memory questId_,
        address receiptContractAddress_,
        uint256 questFee_,
        address protocolFeeRecipient_
    )
        Quest(
            rewardTokenAddress_,
            endTime_,
            startTime_,
            totalParticipants_,
            rewardAmountInWeiOrTokenId_,
            questId_,
            receiptContractAddress_
        )
    {
        questFee = questFee_;
        protocolFeeRecipient = protocolFeeRecipient_;
        questFactoryContract = QuestFactory(msg.sender);
+       maxTotalRewards = totalParticipants_ * rewardAmountInWeiOrTokenId;
+       maxProtocolReward = (maxTotalRewards * questFee) / 10_000;
+       sumMaxRewards = maxTotalRewards + maxProtocolReward;
    }

3. Modify internal dependancies:

// Erc20Quest.sol
    function start() public override {
-       if (IERC20(rewardToken).balanceOf(address(this)) < maxTotalRewards() + maxProtocolReward())
+       if (IERC20(rewardToken).balanceOf(address(this)) < sumMaxRewards)
            revert TotalAmountExceedsBalance();
        super.start();
    }

This would also prevent any possible overflow and it consequences, given that constract creation will revert if this is the case.

Do not check if bool value is true

This can be avoided by just using the bool value:

// Quest.isClaimed
-   return claimedList[tokenId_] == true;
+   return claimedList[tokenId_];

// QuestFactory.mintReceipt
-   if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
+   if (quests[questId_].addressMinted[msg.sender]) revert AddressAlreadyMinted();

Make Function external instead of public

generateTokenURI is just used by external contracts, then it visibility should be change to external in ReceiptRender as well as in TicketRender to save gas

Then, next function are never used internally:

// Quest
-   function pause() public onlyOwner onlyStarted {
+   function pause() external onlyOwner onlyStarted {
//...

-   function unPause() public onlyOwner onlyStarted {
+   function unPause() external onlyOwner onlyStarted {
//...

-   function claim() public virtual onlyQuestActive {
+   function claim() external virtual onlyQuestActive {
//...

-   function getRewardAmount() public view returns (uint256) {
+   function getRewardAmount() external view returns (uint256) {
//...

-   function getRewardToken() public view returns (address) {
+   function getRewardToken() external view returns (address) {
//...

// Erc20Quest
-   function start() public override {
+   function start() external override {
//...

// RabbitHoleTickets.sol
    function mintBatch(
        address to_,
        uint256[] memory ids_,
        uint256[] memory amounts_,
        bytes memory data_
-   ) public onlyMinter {
+   ) external onlyMinter {
//...

-   function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
+   function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) external onlyMinter {
//...

-   function setMinterAddress(address minterAddress_) public onlyOwner {
+   function setMinterAddress(address minterAddress_) external onlyOwner {
//...

-   function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
+   function setRoyaltyFee(uint256 royaltyFee_) external onlyOwner {
//...

-   function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
+   function setRoyaltyRecipient(address royaltyRecipient_) external onlyOwner {
//...

-   function setTicketRenderer(address ticketRenderer_) public onlyOwner {
+   function setTicketRenderer(address ticketRenderer_) external onlyOwner {
//...

QuestFactory.createQuest can be refactor to save gas during deployment

Next lines are repeated twice in the function

quests[questId_].questAddress = address(newQuest);
quests[questId_].totalParticipants = totalParticipants_;
newQuest.transferOwnership(msg.sender);
++questIdCount;
return address(newQuest);

This can be avoid by refactoring the code:

    function createQuest(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountOrTokenId_,
        string memory contractType_,
        string memory questId_
    ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
        // require(quests[questId_].questAddress  != 0)
        if (quests[questId_].questAddress != address(0)) revert QuestIdUsed();

        if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) {
            // require(rewardAllowlist[rewardTokenAddress_])
            if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();

            Erc20Quest newQuest = new Erc20Quest(
                rewardTokenAddress_,
                endTime_,
                startTime_,
                totalParticipants_,
                rewardAmountOrTokenId_,
                questId_,
                address(rabbitholeReceiptContract),
                questFee,
                protocolFeeRecipient
            );

            emit QuestCreated(
                msg.sender,
                address(newQuest),
                questId_,
                contractType_,
                rewardTokenAddress_,
                endTime_,
                startTime_,
                totalParticipants_,
                rewardAmountOrTokenId_
            );
-           quests[questId_].questAddress = address(newQuest);
-           quests[questId_].totalParticipants = totalParticipants_;
-           newQuest.transferOwnership(msg.sender);
-           ++questIdCount;
-           return address(newQuest);
        }

-       if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155'))) {
+       else if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155'))) {
            // require(msg.sender == owner())
            if (msg.sender != owner()) revert OnlyOwnerCanCreate1155Quest();

            Erc1155Quest newQuest = new Erc1155Quest(
                rewardTokenAddress_,
                endTime_,
                startTime_,
                totalParticipants_,
                rewardAmountOrTokenId_,
                questId_,
                address(rabbitholeReceiptContract)
            );

            emit QuestCreated(
                msg.sender,
                address(newQuest),
                questId_,
                contractType_,
                rewardTokenAddress_,
                endTime_,
                startTime_,
                totalParticipants_,
                rewardAmountOrTokenId_
            );
-           quests[questId_].questAddress = address(newQuest);
-           quests[questId_].totalParticipants = totalParticipants_;
-           newQuest.transferOwnership(msg.sender);
-           ++questIdCount;
-           return address(newQuest);
        }
+       else{
+           revert QuestTypeInvalid();
+       }
-       revert QuestTypeInvalid();
+       quests[questId_].questAddress = address(newQuest);
+       quests[questId_].totalParticipants = totalParticipants_;
+       newQuest.transferOwnership(msg.sender);
+       unchecked{++questIdCount;}
+       return address(newQuest);
    }

Use storage pointer to save gas in for loops

    // Quest
    function _setClaimed(uint256[] memory tokenIds_) private {
+       mapping(address =>bool) storage _claimedList = claimedList;
        for (uint i = 0; i < tokenIds_.length; i++) {
-           claimedList[tokenIds_[i]] = true;
+           _claimedList[tokenIds_[i]] = true;
        }
    }

    // RabbitHoleReceipt.getOwnedTokenIdsOfQuest(string memory, address)
+   mapping(uint => string) _questIdForTokenId = questIdForTokenId;
    for (uint i = 0; i < msgSenderBalance; i++) {
        uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
-       if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
+       if (keccak256(bytes(_questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
            tokenIdsForQuest[i] = tokenId;
            foundTokens++;
        }
    }

In the case of claim function in Quest contract, function isClaimed is called inside the loop, which make use of claimedList state variable. In order to save gas using a storage pointer, the loop should be refactored to:

    // Quest.claim
    uint256 redeemableTokenCount = 0;
+   mapping(address =>bool) storage _claimedList = claimedList;
    for (uint i = 0; i < tokens.length; i++) {
-       if (!isClaimed(tokens[i])) {
+       if (!_claimedList(tokens[i])) {
            redeemableTokenCount++;
        }
    }

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

//Erc20Quest
    constructor(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountInWeiOrTokenId_,
        string memory questId_,
        address receiptContractAddress_,
        uint256 questFee_,
        address protocolFeeRecipient_
    )
    Quest(
        rewardTokenAddress_,
        endTime_,
        startTime_,
        totalParticipants_,
        rewardAmountInWeiOrTokenId_,
        questId_,
        receiptContractAddress_
-   )
+   ) payable
    //...
-   function start() public override {
+   function start() public payable override {
    //...

-   function withdrawRemainingTokens(address to_) public override onlyOwner {
+   function withdrawRemainingTokens(address to_) public payable override onlyOwner {
    //...

// Erc1155Quest
    constructor(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountInWeiOrTokenId_,
        string memory questId_,
        address receiptContractAddress_
    )
    Quest(
        rewardTokenAddress_,
        endTime_,
        startTime_,
        totalParticipants_,
        rewardAmountInWeiOrTokenId_,
        questId_,
        receiptContractAddress_
-   ){}
+   ) payable {}
    //...

-   function withdrawRemainingTokens(address to_) public override onlyOwner {
+   function withdrawRemainingTokens(address to_) public payable override onlyOwner
    //...

-   function start() public override {
+   function start() public payable override {
    //...

// Quest
    constructor(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountInWeiOrTokenId_,
        string memory questId_,
        address receiptContractAddress_
-   ) {
+   ) payable {
    //...

-   function start() public virtual onlyOwner {
+   function start() public virtual payable onlyOwner {
    //...

-   function pause() public onlyOwner onlyStarted {
+   function pause() public payable onlyOwner onlyStarted {
    //...

-   function unPause() public onlyOwner onlyStarted {
+   function unPause() public payable onlyOwner onlyStarted {

-   function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}
+   function withdrawRemainingTokens(address to_) public payable virtual onlyOwner onlyAdminWithdrawAfterEnd {}
    //...

// QuestFactory
-   constructor() initializer {}
+   constructor() payable initializer {}
    //...

    function initialize(
        address claimSignerAddress_,
        address rabbitholeReceiptContract_,
        address protocolFeeRecipient_
-   ) public initializer {
+   ) public payable initializer {
    //...

    function createQuest(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountOrTokenId_,
        string memory contractType_,
        string memory questId_
-   ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
+   ) public payable onlyRole(CREATE_QUEST_ROLE) returns (address) {
    //...

-   function changeCreateQuestRole(address account_, bool canCreateQuest_) public onlyOwner {
+   function changeCreateQuestRole(address account_, bool canCreateQuest_) public payable onlyOwner {
    //...

-   function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {
+   function setClaimSignerAddress(address claimSignerAddress_) public payable onlyOwner {
    //...

-   function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {
+   function setProtocolFeeRecipient(address protocolFeeRecipient_) public payable onlyOwner {
    //...

-   function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {
+   function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public payable onlyOwner {
    //...

-   function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
+   function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public payable onlyOwner {
    //...

-   function setQuestFee(uint256 questFee_) public onlyOwner {
+   function setQuestFee(uint256 questFee_) public payable onlyOwner { 
    //..