RabbitHole Quest Protocol contest - hansfriese's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L58-L61 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

In RabbitHoleReceipt.sol and RabbitHoleTicket.sol, receipts and tickets can be minted by anyone.

Proof of Concept

The first line inside the modifier can be passed without reverting for any callers.

    modifier onlyMinter() {
        msg.sender == minterAddress;
        _;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

We should modify like the below.

    modifier onlyMinter() {
        require(msg.sender == minterAddress, 'Not minter');
        _;
    }

Lines of code

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

Vulnerability details

Impact

withdrawFee can be called several times, so attackers can use this to drain Erc20Quest's balance.

Proof of Concept

When the admin calls withdrawRemainingTokens, protocolFee + unclaimedTokens left in the Erc20Quest contract. If unclaimedTokens >= protocolFee, the attacker can call withdrawFee to drain balance, and it can prevent user's claim because of the low balance.

Tools Used

Manual Review

Recommended Mitigation Steps

withdrawFee should be called only once.

Lines of code

https://github.com/rabbitholegg/quest-protocol/tree/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L228 https://github.com/rabbitholegg/quest-protocol/tree/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L99

Vulnerability details

Impact

Users can't claim if they mint receipts after the admin changes receiptContract, and they can't get rewards.

Proof of Concept

In the implementation of QuestFactory.mintReceipt, user will get factory's receipt. But when the user claim, quest's receipt is counted. So if the user mints receipt after the admin changes receipt contract, he will get a new receipt. But he needs old receipts for claiming rewards. So he can't get reward.

Tools Used

Manual Review

Recommended Mitigation Steps

Get receipt contract from quest, and mint quest's receipt instead of factory's receipt.

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#L102-L104

Vulnerability details

Impact

After the end time, the protocol fee might be increased if a new receipt is minted and the fee would be locked inside the contract forever.

Proof of Concept

The main reason for this issue is that users can mint a new receipt after the end time.

With the below scenario, fees might be locked inside the contract.

1. Erc20Quest contract was created with totalParticipants = 10.
2. Before the end time, 10 signatures have been published properly but only 9 users minted the receipt using QuestFactory.mintReceipt() and claimed the rewards. We might assume the 10th user was an attacker.
3. After the end time, receiptRedeemers() = 9, redeemedTokens = 9 and the admin called Erc20Quest.withdrawRemainingTokens() to withdraw remaining tokens. Then the contract will contain the fees of 9 redeemers only.
4. After that, the 10th user minted the receipt using QuestFactory.mintReceipt() and Erc20Quest.receiptRedeemers() will be 10 now.
5. But Erc20Quest.withdrawFee() will try to withdraw the fees of 10 redeemers and it will revert because of insufficient balance.

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

Tools Used

Manual Review

Recommended Mitigation Steps

We should prevent to mint a new receipt after the end time in QuestFactory.mintReceipt().

For that, we can add a new element endTime to the Quest struct and check the time while minting a receipt.

    struct Quest {
        mapping(address => bool) addressMinted;
        address questAddress;
        uint totalParticipants;
        uint numberMinted;
        uint endTime; //+++++++++++++++++
    }

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

        if (block.timestamp >= quests[questId_].endTime) revert InvalidTime(); //+++++++++++++++++

        quests[questId_].addressMinted[msg.sender] = true;
        quests[questId_].numberMinted++;
        emit ReceiptMinted(msg.sender, questId_);
        rabbitholeReceiptContract.mint(msg.sender, questId_);
    }

Lines of code

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

Vulnerability details

Impact

An attacker can run Erc20Quest.withdrawFee before withdrawRemainingTokens, and as a result, withdrawRemainingTokens can revert.

Proof of Concept

In Erc20Quest.withdrawRemainingTokens, the admin left protocolFee() + unclaimedTokens in the contract. It means that it assumes withdrawFee() will be called after withdrawRemainingTokens().

But in fact, anyone can call withdrawFee after the quest's end time. So if an attacker calls withdrawFee before the admin calls withdrawRemainingTokens, withdrawRemainingTokens will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

withdrawFee should be called only once, and in the implementation of withdrawRemainingTokens, if withdrawFee is called before, no need to leave protocolFee().