RabbitHole Quest Protocol contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

A User who has not yet claimed his tickets , can do so even after quest has ended. This has 2 problems:

1. User might not be able to claim the tickets, as Owner would have already sweeped the balance using withdrawRemainingTokens function

2. If User claims ticket post quest end time, Owner would get lesser tokens via withdrawRemainingTokens function even though User has minted ticket post endTime and ideally he should not be allowed to claim funds

Proof of Concept

1. Assume quest Q has completed at timestamp T1 (endTime for Q is T1)
2. User A mint his tickets for quest Q post T1 timestamp

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

3. A ticket is minted to User A even though quest Q has already ended
4. This becomes a problem:

a. If before Step 2 and post endTime, owner has already called withdrawRemainingTokens function then all remaining balance would be sent to specified address. This becomes problem since now contract will not have funds for User A after User A mints the ticket

b. If after Step 3, owner calls withdrawRemainingTokens function then owner will be refunded amount deducting User A claim amount as well (even though User A minted post endTime)

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

Recommended Mitigation Steps

Do not allow mintReceipt function for a completed quest

Lines of code

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

Vulnerability details

Impact

The ERC20 Quest ensures that unclaimed funds will be locked from Admin withdrawal even after endTime. This ensures User to claim there funds by redeeming ticket at any moment

The same check is missing in case of ERC1155 Quest. Admin will be able to withdraw token which user missed to claim within endTime which is wrong

Proof of Concept

1. Assume X users were minted ERC1155 tickets

function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
        _mint(to_, id_, amount_, data_);
    }

2. Lets say X-10 users have claimed there ticket and got required funds using the claim function

3. Now endTime has reached

4. Admin calls the withdrawRemainingTokens function to withdraw the excess tokens

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

5. Ideally 10 users are still remaining from claiming their tickets, so Owner should be allowed to only withdraw balance-10 amount

6. But seems like this check is missing and Admin can withdraw full balance

IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );

Recommended Mitigation Steps

Calculate the total number of participant of the ERC1155 quest by the endTime, lets say this number is X Owner should now only be allowed to retrieve totalParticipants-X tokens

User will not be able to claim tickets

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

Impact: Changing the claimSignerAddress will invalidate all past user tickets.

Proof of Concept:

1. Assume claimSignerAddress was initially address A
2. This is used in signature verification so once user tries to mint receipt, it checks whether signer is indeed claimSignerAddress

function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
       ...
        if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
        if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();

        ...
    }

3. This becomes problem if claimSignerAddress changes as then mintReceipt will fail on existing issued signatures due to unmatching claimSignerAddress

Recommended Mitigation Steps: Do not allow changing claimSignerAddress

Initialize can be frontrun

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

Impact: The intialize function can currently be front run by attacker and set all initialization parameter. This will force product team to redeploy, wasting gas

Steps:

1. Observe that initialize function can be called by anyone

function initialize(
        address claimSignerAddress_,
        address rabbitholeReceiptContract_,
        address protocolFeeRecipient_
    ) public initializer {
        __Ownable_init();
        __AccessControl_init();
        grantDefaultAdminAndCreateQuestRole(msg.sender);
        claimSignerAddress = claimSignerAddress_;
        rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_);
        setProtocolFeeRecipient(protocolFeeRecipient_);
        setQuestFee(2_000);
        questIdCount = 1;
    }

Recommendation: Use a batch transaction to perform deployment and initialization together

Fee recipient can be zero address

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

Impact: It was observed that Fee recipient can be mistakenly set as 0 address. This will lead to all fees getting lost in zero address

Steps:

1. Owner mistakenly set protocolFeeRecipient to address 0 using setProtocolFeeRecipient function
2. After quest ends, withdrawFee was called by owner
3. All collected fee is sent to protocolFeeRecipient which is address 0
4. Admin calls withdrawFee function which sends all funds to protocolFeeRecipient (0 address)

Recommendation: Do not allow 0 address protocolFeeRecipient