RabbitHole Quest Protocol contest - bin2chen's results

Lines of code

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

Vulnerability details

Impact

Quest's rewardToken may be lost, or Quest may not work properly

Proof of Concept

When the quest expires, the following two actions are normally performed:

1. the owner will call Erc20Quest.withdrawRemainingTokens() to retrieve the remaining tokens, and the unclaimed token of mint Receipt and protocol fee will remain in the contract
2. Anyone can call Erc20Quest.withdrawFee() to transfer the protocol fee to protocolFeeRecipient

The implementation of Erc20Quest.withdrawFee() is as follows:

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

One problem with the current implementation is that it can be called repeatedly (as long as the time of quest is expires, any person can call it), which leads to the following possibilities:

1. Malicious users can front-run Erc20Quest.withdrawRemainingTokens() and then repeatedly call withdrawFee() multiple times to transfer all the balances
2. Even if Erc20Quest.withdrawRemainingTokens() is called first, there may still be unclaimed token of mint Receipt left, which anyone can also be called repeatedly to transfer all the remaining balance

The above two situations lead to: If the protocolFeeRecipient has been controlled by the hacker, then the transferred funds are lost If it is not controlled, it will also cause this Erc20Quest not to work properly, and the subsequent withdrawRemainingTokens() and claim() will fail, because there is no balance

Suggestion: add status, withdrawFee() can only be executed once

Tools Used

Recommended Mitigation Steps

+   bool public hasWithdrawFee;

    function withdrawFee() public onlyAdminWithdrawAfterEnd {
+       require(!hasWithdrawFee,"Repeated withdraw");
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
+       hasWithdrawFee = true;
    }

Lines of code

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

Vulnerability details

Impact

The malicious owner gets the claimSignerAddress signature, but does not call mintReceipt() first, waits until the Quest expires and executes withdrawRemainingTokens(), and then call mintReceipt(), so that the newly generated receipt call claim() will occupy the protocolFee

Proof of Concept

When the quest expires, the owner retrieves the remaining tokens by using withdrawRemainingTokens() nonClaimableTokens represents the number of tokens that can be retrieved, and is calculated as follows:

    function withdrawRemainingTokens(address to_) public override onlyOwner {
...
        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

    function receiptRedeemers() public view returns (uint256) {
        return questFactoryContract.getNumberMinted(questId);
    }

withdrawRemainingTokens() leaves unclaim token of the already minted receipt and protocolFee in the contract, waiting for the unclaimed user and protocolFeeRecipient to get them.

but QuestFactory.mintReceipt() does not restrict the Quest expired can not be minted receipt

So the malicious owner can perform the following steps, resulting in the new minted receipt call claim(), will occupy the protocolFee

1. Erc20Quest.withdrawRemainingTokens()
2. QuestFactory.mintReceipt(bob) -->not restrict time, If get the signature any time can call it
3. bob call Erc20Quest.claim() --->bob will take away the untaken protocolFee

The example is as follows: Suppose have Erc20Quest: totalParticipants = 100 rewardAmountInWeiOrTokenId = 1 questFee = 10%

1. When Erc20Quest.start(), the balance of Erc20Quest is: 100 + 10 = 110

2. In the process, there are 100 people get offline signature, note: only 99 people to execute mintReceipt() and claim(), one of bob did not to execute QuestFactory.mintReceipt() (it is also possible that the bob was front-run by withdrawRemainingTokens() when executing mintReceipt()) so: numberMinted = 99 redeemedTokens = 99

3. After quest expires, the owner executes withdrawRemainingTokens(), the balance of Erc20Quest becomes: 9.9 (the remaining protocolFee)

4. After that bob can still execute QuestFactory.mintReceipt() and claim() to take away 1 token numberMinted = 100 redeemedTokens = 100

5. Erc20Quest's balance will be: 9.9 - 1 = 8.9 (i.e. bob occupies protocolFee)

6. protocolFeeRecipient executing Erc20Quest.withdrawFee() will fail, because the balance left is 8.9, but protocolFee()=10

So it is recommended that: mintReceipt() should check whether the quest expires, If it expires, it can't mint receipt

Tools Used

Recommended Mitigation Steps

contract QuestFactory is Initializable, OwnableUpgradeable, AccessControlUpgradeable, IQuestFactory {

    function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
...
+       require(block.timestamp < Quest(quests[questId_].questAddress).endTime(),"Quest Ended, can't mint receipt");

Lines of code

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

Vulnerability details

Impact

After Erc1155Quest.withdrawRemainingTokens(), unclaim user can't call claim()

Proof of Concept

It is described in Erc20Quest.withdrawRemainingTokens() as follows.

Every receipt minted should still be able to claim rewards (and cannot be withdrawn)

I think this is reasonable because:

Note - users could claim a receipt, sell it on secondary (not claim reward)

So when the receipt has been mint even if the Quest end time is up, it is still possible to claim(), because the user has sold the receipt, and claim() should be allowed as long as it has not been claimed

But the current Erc1155Quest.withdrawRemainingTokens() implementation does not retain this mechanism, it will Transfer the entire balance，No reserved unclaim token, which will result in the sold receipts not being able to claim()

The balance is fully transferred, not leaving unclaimed token

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId), //***@audit Take all the balance
            '0x00'
        );
    }

Tools Used

Recommended Mitigation Steps

Implement a mechanism similar to that in Erc20Quest, which has mint Receipt but no claim() , must leave the corresponding funds in the contract

Lines of code

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

Vulnerability details

Impact

Erc20Quest.withdrawRemainingTokens() may leave additional token in the contract

Proof of Concept

After Quest expires, normally, the owner will take the remaining tokens and the protocol will take the protocolFee.

owner takes the remaining tokens via Erc20Quest.withdrawRemainingTokens(), and always leaves the protocolFee in the contract

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
        //****@audit subtract protocolFee()，so protocolFee will Stay in the contract
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }

anyone Call Erc20Quest.withdrawFee() to take the protocolFee and transfer it to the protocolFeeRecipient

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

But there is a problem here, the two methods do not limit the order, and withdrawRemainingTokens() does not check whether the protocolFee is taken or not, it always leaves the protocolFee() in the contract

This leads to a problem that if withdrawFee() is executed first, (this method can be called by anyone,Malicious users can front-run.) and the protocolFee has been transferred, then withdrawRemainingTokens() is executed afterwards, the protocolFee will still be left in the contract. This extra protocolFee will be locked in the contract.

So it is recommended that withdrawRemainingTokens() needs to check whether the protocolFee has been taken or not, and if it has been taken, the protocolFee does not need to be left in the contract.

Tools Used

Recommended Mitigation Steps

+   bool public hasWithdrawFee;

    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);

        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
+       uint remainingProtocolFee = 0;
+       if (!hasWithdrawFee) remainingProtocolFee = protocolFee();
+       uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - remainingProtocolFee - unclaimedTokens;
-       uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
    }



    function withdrawFee() public onlyAdminWithdrawAfterEnd {
+       require(!hasWithdrawFee,"Repeated withdraw"); //***@aduit Another problem
        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
+       hasWithdrawFee = true;
    }

Lines of code

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

Vulnerability details

Impact

If claimSignerAddress is the same address, the signature can be reused in other chains to steal the token

Proof of Concept

mintReceipt() is used to generate Receipt, which is a very important function for the protocol, and the method internally verifies the legitimacy of the msg.sender signature The signature is verified as follows:

    function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
....
        //****@audit hash = keccak256(abi.encodePacked(msg.sender, questId_))
        if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();


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

The above implementation does not add the chainId/address(this) to the signature, so the claimSignerAddress signature can be reused in multiple chains If claimSignerAddress is the same address

Tools Used

Recommended Mitigation Steps

use OpenZeppelin's EIP712.sol

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol

1.Erc1155Quest.getRewardAmount() Return error, the current implementation, each Receipt gets 1 Erc1155 It is recommended to override Quest.getRewardAmount() to return 1

contract Erc1155Quest is Quest, ERC1155Holder {
...
+   function getRewardAmount() public override view returns (uint256) {
+       return 1;
+   }

2.claim() does not follow the "Checks Effects Interactions" principle, resulting in external acquisition of redeemedTokens that may be inaccurate

    function claim() public virtual onlyQuestActive {
...

        uint256 totalRedeemableRewards = _calculateRewards(redeemableTokenCount);
        _setClaimed(tokens);
+       redeemedTokens += redeemableTokenCount;            
        _transferRewards(totalRedeemableRewards);
-       redeemedTokens += redeemableTokenCount;    

3.modifier "onlyAdminWithdrawAfterEnd" Naming problems, this does not limit must be admin suggested to modify the method named :onlyWithdrawAfterEnd