RabbitHole Quest Protocol contest - peakbolt's results

Lines of code

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

Vulnerability details

Impact

In Erc20Quest.sol, the withdrawFee() can be called multiple times by anyone to drain unclaimed rewards from the contract. This is due to missing tracking of previous protocol fee withdrawals and incorrect modifier.

Proof of Concept

After the end of the claim period, call the withdrawFee() multiple times to drain the contract of the unclaimed rewards. The function does not track or update the balance after transfering the protocol fee.

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

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

And there is no access control as the onlyAdminWithdrawAfterEnd modifier only ensure the withdrawFee() can be called after end of claim period.

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

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

Recommended Mitigation Steps

Track the amount of protocol fee withdrawn and add in access control if necessary.

Lines of code

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

Vulnerability details

In Erc20Quest.sol, the withdrawRemainingTokens() could withdraw more unclaimed tokens than calculated if new RabbitHoleReceipts are minted after that. This is because withdrawRemainingTokens() takes the number of RabbitHoleReceipt minted from questFactoryContract.getNumberMinted().

However, nothing stops users from minting new RabbitHoleReceipt after the claim period end. This will prevent some users from redeeming their rewards as there will be less token balance in the contract than required.

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

Proof of Concept

Suppose a scenario (with zero protocol fee to simplify it) - Where total participants = 100 and reward amount is 1 ETH, - Then max rewards will be 100 ETH (100 particiants *1ETH) - Erc20Quest contract balance will be = 100 ETH (assume min required starting balance).

After the end of claim period, we assume - RH receipts minted = 10 - RH receipts claimed = 9, leaving balance = 91 ETH (100ETH - 9 claims * 1 ETH) - Then withdrawRemainingTokens() will transfer out 91ETH - 1 unclaimed RH receipt (i.e. 1 ETH) = 90 ETH. - Remaining balance = 1 ETH for the 1 unclaimed receipt

However, a participant who has a legit signature could still mint a new RH receipt at this point. With the receipt, he can claim the remaining 1 ETH, taking away the unclaimed reward for the other participant.

Note: it doesn’t matter if off-chain signing is disabled after claim period. This is because participant could get a valid signature before claim period end and not mint it until the scenario above occurs.

Recommended Mitigation Steps

Prevent minting of new tickets after claim period end so that the amount of minted RH receipt will be fixed and unchanged.

Lines of code

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

Vulnerability details

Participants will not be able to claim their rewards after the quest owner calls Erc1155Quest.withdrawRemainingTokens(), which will withdraw all remaining tokens including unclaimed ones. This is different from Erc20Quest, which will retain any unclaimed rewards even after withdrawRemainingTokens() is called.

This behaviour is also different from the claim process documentation at https://user-images.githubusercontent.com/14314818/214354756-0af7e34d-746e-4429-8b55-8eb6d8bb1e31.png

Proof of Concept

Suppose the ERC1155 quest has ended and the quest owner calls withdrawRemainingTokens() to transfer out all the remaining tokens in the contract. Now, participants who still has unredeemed RH receipts will not be able to claim their rewards from the contract as the balance is zero.

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

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

Recommended Mitigation Steps

Rectify the withdrawRemainingTokens() to only withdraw non-claimable rewards (based on proportion of unminted RH receipts), while leaving the unclaimed rewards in the contract.

Lines of code

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

Vulnerability details

As the documentation mentioned that the protocol will be deployed on multiple chains, it is possible to create a quest with the exact same questId on a different chain and generate a valid signature for any user address.

Furthermore, there is no signature expiry, anyone could look for quests with unclaimed rewards and replay signature to mint RH receipt and claim the rewards.

Proof of Concept

As shown below, the hash used for the signature only contains the msg.sender and questId_, both of which could be duplicated on a separate chain, allowing generation of an exact same signature that can be replayed on another chain.

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

Recommended Mitigation Steps

Add in chainId, nonce and deadline timestamp for the signature hash generation.