RabbitHole Quest Protocol contest - ElKu'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/RabbitHoleReceipt.sol#L98-L104

Vulnerability details

Impact

The onlyMinter() modifier is not correctly coded, allowing anyone to call important functions such as mint(). This means a malicious user can mint himself many nft receipts and then claim the rewards, thus clearing the Quest contract balance.

Proof of Concept

This is the onlyMinter modifier. As you can see, there is no if statement or require statement on this. Which means anybody can circumvent this modifier and call the supposedly restricted functions.

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

Once a Quest has started a malicious user will call the mint function in RabbitHoleReceipt contract to mint NFT receipts for himself. Right after he can call the claim() function from Quest contract to claim the reward tokens. Doing this in a repeated way, he can clear the Quest contract balance.

Note that the claim() function doesnt check whether the quests[questId_].addressMinted[msg.sender] is true or not. This makes sense, because it allows the receipts to be traded in secondary markets. But with this particular vulnerability, it allows for a malicious user to reap profit.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

Correct the modifier as follows:

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

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L47-L50 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L83-L99

Vulnerability details

Impact

The onlyMinter() modifier is not correctly coded, allowing anyone to call important functions such as mint() and mintBatch(). This means a malicious user can mint himself many ERC1155 reward tokens directly.

Proof of Concept

This is the onlyMinter modifier. As you can see, there is no if statement or require statement on this. Which means anybody can circumvent this modifier and call the supposedly restricted functions.

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

Once the RabbitHoleTickets contract is deployed the malicious user can mint as many tokens as he wants for himself. Causing unimaginable loss to users and the protocol.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

Correct the modifier as follows:

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

Lines of code

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

Vulnerability details

Impact

There is no access control on withdrawFee() function in Erc20Quest contract. Even though the protocol fees are sent to the designated protocolFeeRecipient, this function can be called multiple times by anyone, draining the Quest contract. This is more like a DoS attack, causing loss of time and effort for everyone involved and affected.

Proof of Concept

The attack path works like this:

1. Malicious user sees that the Quest has ended. He has a bone to pick with the Quest owner and decides to make his life difficult.
2. He calls the withdrawFee() function repeatedly which sends the contract balance to the protocolFeeRecipient set by the factory.
3. The users who completed the Quest and who were late to claim their rewards are not able to withdraw their funds now. The Quest owner is not able to withdraw his unclaimable funds as well.
4. The situation can be resolved by requesting the RabbitHole team to send the money back to the Quest contract, but it does leave a bad taste in everyone's mouth.
5. The attacker sits in his cozy chair with his evil laugh ( Muhahaha ), seeing everything being played out right in front of him.

Tools Used

Hardhat, VS code, Manual analysis

Recommended Mitigation Steps

I suggest, once the protocol fees are withdrawn, it should be marked as such with a state variable. That is, if you want to stop the evil attacker from having the last laugh.

Lines of code

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

Vulnerability details

Impact

withdrawRemainingTokens() function allows the withdrawal of the whole Erc1155Quest contract balance. This means any users who haven't yet claimed their rewards will be unable to do so resulting in a fund loss.

Proof of Concept

withdrawRemainingTokens function looks like this:

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

You can see that the amount withdrawn is given by IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) which is the contract balance for that particular tokenId.

If some users haven't claimed their rewards yet, they will be unable to do so after the withdrawRemainingTokens is called.

In the Erc20Quest contract the withdrawRemainingTokens function calculates the nonClaimableTokens which is the allowed amount of tokens to withdraw. A similar logic should be applied, but is missing in the 1155 contract.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

I suggest, the nonClaimableTokens to be calculated and adjusted when calculating how much tokens to withdraw after Quest ends. The logic from Erc20Quest contract can be used here as well.

Lines of code

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

Vulnerability details

Impact

withdrawFee() is used for sending the protocol fees to protocolFeeRecipient. And withdrawRemainingTokens() is used for transferring nonClaimableTokens back to the Quest admin. In the withdrawRemainingTokens function, when calculating the amount of tokens to withdraw, the protocolFee() is deducted from it. But if the withdrawFee was already called there is no need to deduct it. Thus Quest admin loses tokens equal to protocolFee.

Proof of Concept

The unfortunate event happens as such:

1. Quest has ended. Users and Admins are happy.
2. Someone calls the withdrawFee() function and the protocol fees are sent to the address set by the factory contract. So the balance of the contract is less now.
3. The Quest admin wakes up late and tries to withdraw the tokens which are not supposed to be claimed by anyone. The exact amount of tokens sent to him are calculated as follows:

uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;

4. Here the formula deducts the protocolFee() once again, which is already deducted from the contract balance.
5. Which means the Quest admin will loose tokens worth of protocolFee.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

I suggest, once the protocol fees are withdrawn, it should be marked as such with a state variable. And then use this state variable to decide whether the protocolFee should be deducted from the contract balance.

Lines of code

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

Vulnerability details

Impact

Once the user has received an ERC-721 RabbitHole Receipt token, he has two options - either to call the claim() function and get the reward tokens, or sell it to another user who is interested. Here, a malicious user can cheat the system and do both. He does this by selling his receipt nft first and in the same block, front running the sell transaction with a call to claim() function.

Proof of Concept

The attack path works like this:

1. User completes an on-chain task and mints himself an ERC-721 RabbitHole Receipt token.
2. He creates a sell order for the same.
3. Another user checks that the receipt is not claimed yet and initiates the buy transaction.
4. Noticing the buy transaction in the mempool, the original malicious user calls the claim() function with a high gas fee (and front runs) and claims his reward tokens.
5. Right after, the buy transaction also goes through in the same block and the user was able to sell his already claimed(and hence useless) receipt.

Note that this works only when the buyer of the NFT manually checks if the receipt was not claimed and then initiates the buy transaction. If he uses a contract which does the check and buy in a single transaction, this attack path wont work.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

The claim() function doesn't ask the user to send his receipt NFT to the contract before sending the reward tokens. It only checks if the user currently owns it and its not been claimed yet.

I suggest the claim() function should receive the NFT receipt before sending the reward tokens to avoid this kind of attack.