RabbitHole Quest Protocol contest - m9800's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/9ef32907788dde6c42990ee5dde8f53caeaba474/contracts/QuestFactory.sol#L219 https://github.com/rabbitholegg/quest-protocol/blob/9ef32907788dde6c42990ee5dde8f53caeaba474/contracts/Erc20Quest.sol#L81 https://github.com/rabbitholegg/quest-protocol/blob/9ef32907788dde6c42990ee5dde8f53caeaba474/contracts/Erc20Quest.sol#L58

Vulnerability details

Impact

A user can mint a receipt even when the quest end time has passed. The problem with this is that the quest contract may not have sufficient funds to pay the receipt rewards.

Proof of Concept

When a quest is started funds are checked:

    function start() public override {
        if (IERC20(rewardToken).balanceOf(address(this)) < maxTotalRewards() + maxProtocolReward())
            revert TotalAmountExceedsBalance();
        super.start();
    }

When the quest is finished, the owner can withdraw the remaining tokens in the contract but every receipt minted should still be able to claim rewards (it is stated in the comments of the function @dev Every receipt minted should still be able to claim rewards... ):

   /// @notice Function that allows the owner to withdraw the remaining tokens in the contract
    /// @dev Every receipt minted should still be able to claim rewards (and cannot be withdrawn). This function can only be called after the quest end time
    /// @param to_ The address to send the remaining tokens to
    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);
    }

The problem is that receiptRedeemers() is the count of minted receipts at the time the function is being called. So if a user mints a receipt (mintReceipt() in QuestFactory) after this function has been called, he won´t be able to claim his rewards.

Tools Used

Manual review

Recommended Mitigation Steps

Don´t let users mint receipts once the quest has finished.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/9ef32907788dde6c42990ee5dde8f53caeaba474/contracts/QuestFactory.sol#L210

Vulnerability details

Impact

The signature used to mint a receipt in QuestFactory is not taking the chain ID into account. A user can use the same signature in other chains where the contract is deployed.

Proof of Concept

In the documentation of the Quest protocol, it is stated that it´s a multichain protocol. So a signature must take the chain into account to avoid being used again in other chains.

    function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public { cadenas
        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_);
    }
}

We can see that the chain id is not being taken into account.

    function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) {
        bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_));
        return ECDSAUpgradeable.recover(messageDigest, signature_);
    }

Tools Used

Manual review

Recommended Mitigation Steps

See EIP 155.