RabbitHole Quest Protocol contest - omis's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L100-L104 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L75-L79

Vulnerability details

Impact

The withdrawFee for the Erc20Quest contract can be called after the quest ended which it allows to send the protocol fee to the protocolFeeRecipient however it doesn't check if it was already called. If the contract's token balance is multiple of the protocol fee it's possible for anyone to call witdrawFee and grief all the people who didn't claim yet because when they will try the contract's token balance will be insufficient.

Proof of Concept

Assume the quest ended, the contract has 100 tokens, the protocol fee is 10, unclaimed tokens 90 (for simplicity we assume all the receipts were minted and all contract's token balance should go in rewards). A user calls withdrawFee 10 times and the unclaimed receipts won't be able to be claimed.

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

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

Recommended Mitigation Steps

Validate that withdrawFee can be called only once.

Lines of code

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

Vulnerability details

Impact

Erc1155Quest's withdrawRemainingTokens function allows the owner to claim all the remaining tokens, the unclaimed tickets will become worthless since they won't be claimable. Note this functionality for the Erc20Quest correctly take in consideration the unclaimed ticket remaining.

Proof of Concept

The quest ended and there are 100 unclaimed tickets corresponding to 100 ERC1155 token rewards. The owner calls withdrawRemainingTokens and withdraw all the remaining tokens, the unclaimed tickets can't be claim anymore.

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

Recommended Mitigation Steps

In withdrawRemainingTokens the correct amount of tokens to withdraw should be contract's ERC1155 tokens balance - unclaimed tokens (questFactoryContract.getNumberMinted(questId) - redeemedTokens).

Lines of code

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

Vulnerability details

Impact

The withdrawRemainingTokens function leaves tokens (for an amount equal to protocolFee) in the contract when withdraFee is called earlier due to non tracking if withdraFee was already called.

Proof of Concept

Assume balance = 150, unclaimedTokens = 0, protocolFee = 50. withdrawFee is called and balance becomes 100. When calling withdrawRemainingTokens nonClaimableTokens will be 100 - 50 = 50. 50 tokens are lost in the contract.

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

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

Recommended Mitigation Steps

Keep track if withdrawFee is called and if yes don't subtracts protocolFee when computing nonClaimableTokens.

Lines of code

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

Vulnerability details

Impact

A user can claim the same receipt across different chains under certain conditions because the chainId is not included in the hashed data.

Proof of Concept

The hashed data contains only the msg.sender and questId_, if the following conditions hold true the same signature can be reused in different chains:

• claimSignerAddress is the same address
• It exists a quest with the same questId_ and there are tokens in the contract

    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 a result a user can complete a quest in a single chain and get the rewards in all of them.

Recommended Mitigation Steps

Use the EIP-712 standard and validate the chainId