RabbitHole Quest Protocol contest - Breeje's results

Lines of code

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

Vulnerability details

Impact

There is an Edge Case not covered where some users got their issue hashes in between the Start and end Time for quest but they minted a RabbitHole Receipt after the endTime. In this Situation, User despite owning a Receipt Token, might not be able to claim Reward.

Proof of Concept

Consider the Following Situation:

1. For an Erc20Quest, Let's take a situation where totalParticipants are set to 100 and rewardAmountInWeiOrTokenId is set to 1 ether.
2. Let's Consider 50 Participants minted Receipt and Claimed Rewards but 5 more Participants got their issue hashes in between the Quest Start and the Quest End Time. As the Max limit was not reached their hashes got issued.
3. After End Time, the Owner withdraws the unclaimed Tokens of 50 Participants as the Total minted Receipts are 50 till that time.
4. Next those 5 Users uses the mintReceipt method after the End Time when owner has already withdrawn all the Unclaimed Tokens back. As there is only max limit check, User will successfully mint the Receipt.
5. In this situation, Despite owning a Receipt token, Users won't get any reward.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Add an extra field named endTime in Quest Structure.
2. Add its value in quests[questId_].endTime while Creating a new quest.
3. Add a Require parameter in mintReceipt() method such that if the end time of any quest is passed, User should not be able to mint Receipt for that Quest.

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/QuestFactory.sol#L219-L230

Vulnerability details

Impact

Protocol might not get any Fees for Mints done after End Time for a Quest whose hashes has been Issued in between start and End time.

Proof of Concept

File: QuestFactory.sol

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

Link to Code

As there are no checks on quest lifecycle, User can mint the Receipt at any time of their convenience. As number of hashes issued are tracked off chain, getting the Hash confirms its Eligibility to mint the Receipt.

So Consider the Following Situation:

1. User got the hash issued.
2. Once the End time is gone, Quest Owner withdraws all the Remaining Tokens. It also includes the Protocol fees of no. of users who didn't minted the Receipt.
3. Now few User whose hashes got issued earlier mints the token. They will successfully mint the token as max limit would not have reached.
4. But in this Case, Protocol losses on Protocol Fees which are already withdrawn by Quest owner.

Tools Used

Manual Review

Recommended Mitigation Steps

2 Ways to Mitigate the Issue depending on which functionality you consider more important:

1. Add an extra field in Quest Structure to track the End Time for the Quest and add a require condition in mintReceipt method such that no one is allowed to mint the Token after endTime.
2. Don't allow Owner to withdraw any amount from the Protocol Fees. Use maxProtocolReward instead of protocolFee to cover the Fees for Future mints.

File: Erc20Quest.sol

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

Link to Code

QA Report

[L-1] USE OF FLOATING PRAGMA

Impact: swcregistry

All the 10 Smart Contracts uses Floating Pragma Solidity Version.

[L-2] ABI.ENCODEPACKED() SHOULD NOT BE USED WITH DYNAMIC TYPES WHEN PASSING THE RESULT TO A HASH FUNCTION SUCH AS KECCAK256()

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456).

Instances (3):

File: QuestFactory.sol

72:     if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20'))) {

105:    if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155'))) {

222:    if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();

Link to Code

[L-3] SINGLE-STEP PROCESS FOR CRITICAL OWNERSHIP TRANSFER/RENOUNCE IS RISKY

Given that Quest, QuestFactory, Erc20Quest, Erc1155Quest, RabbitHoleReceipt and RabbitHoleTickets are derived from Ownable or OwnableUpgradable, the ownership management of these contract defaults to Ownable’s transferOwnership() and renounceOwnership() methods which are not overridden here. Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes.

Recommendation

It is recommended to use Ownable2Step to mitigate this risk. If not, It is highly recommended to atleast override renounceOwnership() method and disable that option forever.

[L-4] POOR IMPLEMENTATION OF OOPS CONCEPT IN Quest CONTRACT

_calculateRewards, _transferRewards and withdrawRemainingTokens methods in Quest contract uses either revert or contains the empty block.

Instance:

File: Quest.sol

122:      function _calculateRewards(uint256 redeemableTokenCount_) internal virtual returns (uint256) {
123:          revert MustImplementInChild();
124:      }

129:      function _transferRewards(uint256 amount_) internal virtual {
130:          revert MustImplementInChild();
131:      }

150:      function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}

Link to Code

As Quest contract is inherited by Erc20Quest and Erc1155Quest, this methods are overriden in those contracts.

According to Solidity Docs,

Contract need to be marked as abstract when at least one of their functions is not implemented.

Given that the Quest contract is not implementing withdrawRemainingTokens method and same goes for _calculateRewards and _transferRewards too, so it should be marked as abstract contract.

Checkout this reference.

[L-5] MISSING CHECKS FOR address(0) WHEN ASSIGNING VALUES TO ADDRESS STATE VARIABLES

Instances (6):

File: QuestFactory.sol

37:   function initialize(
          address claimSignerAddress_,
          address rabbitholeReceiptContract_,
          address protocolFeeRecipient_
      ) public initializer {
          __Ownable_init();
          __AccessControl_init();
          grantDefaultAdminAndCreateQuestRole(msg.sender);
          claimSignerAddress = claimSignerAddress_;
          rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_);
          setProtocolFeeRecipient(protocolFeeRecipient_);
          setQuestFee(2_000);
          questIdCount = 1;
50:   }

160:  claimSignerAddress = claimSignerAddress_;

Link to Code

File: Quest.sol

    constructor(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountInWeiOrTokenId_,
        string memory questId_,
        address receiptContractAddress_
    ) {
        if (endTime_ <= block.timestamp) revert EndTimeInPast();
        if (startTime_ <= block.timestamp) revert StartTimeInPast();
        if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();
        endTime = endTime_;
        startTime = startTime_;
        rewardToken = rewardTokenAddress_;
        totalParticipants = totalParticipants_;
        rewardAmountInWeiOrTokenId = rewardAmountInWeiOrTokenId_;
        questId = questId_;
        rabbitHoleReceiptContract = RabbitHoleReceipt(receiptContractAddress_);
        redeemedTokens = 0;
    }

Link to Code

[L-6] ADMIN WON'T HAVE ACCESS TO GRANT OR REVOKE ANY NEWER ROLE IN QuestFactory.sol

In QuestFactory.sol contract, grantDefaultAdminAndCreateQuestRole method is used at the time of initialization to grant role of DEFAULT_ADMIN_ROLE and CREATE_QUEST_ROLE.

File: QuestFactory.sol

152:    function grantDefaultAdminAndCreateQuestRole(address account_) internal {
153:        _grantRole(DEFAULT_ADMIN_ROLE, account_);
154:        _grantRole(CREATE_QUEST_ROLE, account_);
155:    }

Link to Code

What this internally does is call _grantRole method in AccessControlUpgradable which sets the _account as "member".

File: AccessControlUpgradable.sol

    function _grantRole(bytes32 role, address account) internal virtual {
        if (!hasRole(role, account)) {
            _roles[role].members[account] = true;
            emit RoleGranted(role, account, _msgSender());
        }
    }

Owner can still add or remove from CREATE_QUEST_ROLE because of changeCreateQuestRole method, But owner won't be able to grant role or revoke role for any other Role as those methods has onlyRole(getRoleAdmin(role)) modifier.

Recommended Mitigation Step

It is recommended to use _setRoleAdmin call which will enable DEFAULT_ADMIN_ROLE to create more roles, Grant them or revoke them in Future.

[L-7] __ERC721Enumerable_init() NOT CALLED IN RabbitHoleReceipt

*Instance (1): *

File: contracts/RabbitHoleReceipt.sol

      function initialize(
          address receiptRenderer_,
          address royaltyRecipient_,
          address minterAddress_,
          uint royaltyFee_
      ) public initializer {
          __ERC721_init('RabbitHoleReceipt', 'RHR');
          __ERC721URIStorage_init();
          __Ownable_init();
          royaltyRecipient = royaltyRecipient_;
          minterAddress = minterAddress_;
          royaltyFee = royaltyFee_;
          ReceiptRendererContract = ReceiptRenderer(receiptRenderer_);
      }

Link to Code

[L-8] NO CHECK ON royaltyFee TO BE LESS THAN OR EQUAL TO 10_000

It is recommended to add a require check on royaltyFee to be below or equal to 10_000 before setting it.

Instances (2):

File: RabbitHoleTickets.sol 

    function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
        royaltyFee = royaltyFee_;
        emit RoyaltyFeeSet(royaltyFee_);
    }

Link to Code

File: RabbitHoleReceipt.sol

    function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
        royaltyFee = royaltyFee_;
        emit RoyaltyFeeSet(royaltyFee_);
    }

Link to Code

[NC-1] UNUSED PARAMETER SHOULD BE REMOVED

Instances (3):

File: Quest.sol

122:      function _calculateRewards(uint256 redeemableTokenCount_) internal virtual returns (uint256) {

129:      function _transferRewards(uint256 amount_) internal virtual {

Link to Code

File: RabbitHoleTickets.sol

110:      uint256 tokenId_,

Link to Code

Gas Optimizations

[GAS-1] EMPTY BLOCKS SHOULD BE REMOVED OR EMIT SOMETHING

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the contract is meant to be extended, the contract should be abstract and the function signatures be added without any default implementation.

Instance (1):

File: Quest.sol

150:    function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}

Link to Code

[GAS-2] X += Y COSTS MORE GAS THAN X = X + Y FOR STATE VARIABLES

Instances (1):

File: Quest.sol

115:    redeemedTokens += redeemableTokenCount;

Link to Code

[GAS-3] NOT USING THE NAMED RETURN VARIABLES WHEN A FUNCTION RETURNS, WASTES DEPLOYMENT GAS

Instance (2):

File: RabbitHoleReceipt.sol

181:    ) external view override returns (address receiver, uint256 royaltyAmount) {

Link to Code

File: RabbitHoleTickets.sol

112:    ) external view override returns (address receiver, uint256 royaltyAmount) {

Link to Code

[GAS-4] UNNECESSARY BOOLEAN COMPARISONS COSTS MORE GAS

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using directValue instead of directValue == true and !directValue instead of directValue == false here:

Instances (3):

File: Quest.sol

136:    return claimedList[tokenId_] == true;

Link to Code

File: QuestFactory.sol

73:     if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();

221:    if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();

Link to Code