RabbitHole Quest Protocol contest - rvierdiiev's results

Lines of code

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

Vulnerability details

Impact

Erc20Quest.withdrawFee can be called by anyone multiple times in order to sent more tokens to fee recepient and make Erc20Quest insolvent.

Proof of Concept

When Erc20Quest is started, then owner tops it up with amount that is enough to cover maximum amount of redeems and maximum fee for that.

Once quest is finished anyone can call Erc20Quest.withdrawFee function in order to send protocol fee amount. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L96-L104

    function protocolFee() public view returns (uint256) {
        return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
    }


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

This function just sends amount, that depends on how many tokens where minted for the current quest. Even that it uses onlyAdminWithdrawAfterEnd modifier doesn't restrict anyone else from calling it because modifier doesn't check if caller is owner.

Pls, note, that there is no any check that fee was already sent and this function can be called as many times as contract still has enough funds to send to protocolFeeRecipient. That means that by calling this function more than 1 time, fee recepient will receive more fees, then he should and contract can become insolvent(depends on amount of minted receipts for the quest).

Why i think this is high severity here is because if this function was only callable by admin, then he should be malicious to do that. But in this case anyone who is interested can make contract insolvent and receipt holders will not be able to redeem them.

Tools Used

VsCode

Recommended Mitigation Steps

Create boolean variable that is set, once fee is paid.

Lines of code

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

Vulnerability details

Impact

QuestFactory.mintReceipt should not be able to mint token for quest that is not started yet

Proof of Concept

QuestFactory.mintReceipt function is called by users in order to mint new receipt token for them for the specific quest. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219-L229

    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_);
    }

As you can see this function do some validations, but it's not checking that quest has started.

There are 2 problems that i see here. 1.Quest is toped up by owner, but isn't started yet. So it has tokens inside. If owner changed his mind he can just wait till finish of quest and call withdrawRemainingTokens function which will send all amount to him. But in case if QuestFactory will mint any tokens before quest is started, then owner will not be able to withdraw all amount, even that he didn't start the quest. 2.Quest is not toped up and not started. Factory mints receipts for this quest for any reason. But this receipts are not backed with redeemable amount inside Quest contract. If owner will not start it, then noone will receive nothing.

I understand that it will be another service that allows users to start quest and will generate hashes for them that can be changed to receipt, but i believe that the check should be present on chain that will prevent any actions in case if smth will happen and users will be able to receive hashes before quest is actually started.

Tools Used

VsCode

Recommended Mitigation Steps

Add check that auction is started in order to mint receipts.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219-L229 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87

Vulnerability details

Impact

Late caller of QuestFactory.mintReceipt can lose his redeem amount.

Proof of Concept

When user participate in quest he receives signed message from indexer service that then can be changed to receipt by using QuestFactory.mintReceipt function. This function will increment amount of receipts minted for that quest. Currently there is no any time restrictions when user can call this function.

Each quest has start and end time. Once end time has passed then owner can call withdrawRemainingTokens function. Let's check how it works inside Erc20Quest contract. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81-L87

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


        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }


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

As you can see this function will check in Factory how many tokens were minted for this quest in order to leave that amount inside contract and not transfer it to owner.

So here is problem scenario. 1.Quest is started. User participated and received signed message that he can change for receipt token. 2.For some reasons he doesn't call QuestFactory.mintReceipt for a long time, so quest has finished. 3.Owner wants to withdraw remaining tokens and calls Erc20Quest.withdrawRemainingTokens. Because user didn't call QuestFactory.mintReceipt, amount of minted receipts was not increased for the quest, so owner withdraws more. 4.User calls QuestFactory.mintReceipt. 5.In case if all redeemers already redeemed their receipts for the rewards, then user will not be able to get funds. Otherwise he will do that, but someone else, who will be the last will not be able to redeem.

Why i believe this is high severity, because it's very likely that user will call QuestFactory.mintReceipt function not right after they received their signed message. So this increases the amount of people who will call function when quest is already finished. And because of that the amount of people who will receive nothing increases.

Tools Used

VsCode

Recommended Mitigation Steps

Think about some window period after the quest end, when users still can change their signed message to receipt. After that period revert when function is called. And also do not allow owner to withdraw funds till that window is finished.

Lines of code

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

Vulnerability details

Impact

Signed message can be reused on another chain to mint quest receipt.

Proof of Concept

When user participate in contest he receives signed message from off chain service, that allows him to mint receipt and claim rewards. Currently this signed message contains account and quest. Then signer is checked to be claimSignerAddress.

Because protocol is planned to be used on different chains, such check is not enough. In case if same claimSignerAddress will be used on different chains, then user will be able to reuse his signed message from one chain on another one without participating in contests.

Scenario. 1.On chainA quest with id 1 is started. User participated and received signed message that he exchanged for receipt. 2.On chainB quest with id 1 is started. User just used same signed message and exhanged it for receipt for free.

Tools Used

VsCode

Recommended Mitigation Steps

Add chainId field to the signed message to restrict replay on another chain.