RabbitHole Quest Protocol contest - tnevler's results

Lines of code

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

Vulnerability details

Impact

1. If there is no check that msg.sender == admin in modifier onlyAdminWithdrawAfterEnd()

    modifier onlyAdminWithdrawAfterEnd() { 
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

2. And if there is no check that function withdrawFee() was called already then attacker will be able to call withdrawFee() function multiple times.

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

3. After that all tokens will be transferred from the balance of the contract to protocolFeeRecipient.
4. As result there will be no tokens on the contract's balance and users won't be able to get their reward tokens.

Tools Used

Manual Review

Recommended Mitigation Steps

Check that function withdrawFee() is called only once. It is not important who called it (admin or another user) because fee will be send to specified protocolFeeRecipient address.

Report

Low Risk

[L-1]: Critical changes should use two-step procedure

Context:

1. function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner { L159
2. function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner { L165
3. function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { L71
4. function setMinterAddress(address minterAddress_) public onlyOwner { L83
5. function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner { L60
6. function setMinterAddress(address minterAddress_) public onlyOwner { L73

Recommendation:

The best practice is to use two-step procedure for critical changes to make them less error-prone.

[L-2]: Owner can renounce ownership

Context:

import {OwnableUpgradeable} from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol'; L9

Description:

@openzeppelin/contracts/access/Ownable.sol' used in this project contract implements renounceOwnership() function. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

Recommendation:

You need to reimplement the function.

Non-Critical Issues

[N-1]: Function defines a named return variable but then it uses return statements

Context:

1. return (royaltyRecipient, royaltyPayment); L185
2. return (royaltyRecipient, royaltyPayment); L114

Recommendation:

Choose named return variable or return statement. It is unnecessary to use both.

[N-2]: Use of immutable instead of constant keccak expression

Context:

1. bytes32 public constant CREATE_QUEST_ROLE = keccak256('CREATE_QUEST_ROLE'); L17

Description:

According to official solidity documentation for a constant variable, the expression assigned to it is copied to all the places where it is accessed and also re-evaluated each time. It is recommended to use immutable instead.

[N-3]: No same value input check

Context:

1. claimSignerAddress = claimSignerAddress_; L160
2. protocolFeeRecipient = protocolFeeRecipient_; L167
3. rabbitholeReceiptContract = RabbitHoleReceipt(rabbitholeReceiptContract_); L173
4. rewardAllowlist[rewardAddress_] = allowed_; L180
5. questFee = questFee_; L188
6. ReceiptRendererContract = ReceiptRenderer(receiptRenderer_); L66
7. royaltyRecipient = royaltyRecipient_; L72
8. QuestFactoryContract = IQuestFactory(questFactory_); L78
9. minterAddress = minterAddress_; L84
10. royaltyFee = royaltyFee_; L91
11. TicketRendererContract = TicketRenderer(ticketRenderer_); L55
12. royaltyRecipient = royaltyRecipient_; L61
13. royaltyFee = royaltyFee_; L67
14. minterAddress = minterAddress_; L74

Recommendation:

Example how to fix require(_newOwner != owner, " Same address");

[N-4]: Wrong order of functions

Context:

1. using CountersUpgradeable for CountersUpgradeable.Counter; L26 (using for declaration can not go after event definition)
2. modifier onlyAdminWithdrawAfterEnd() { L76 (modifier definition can not go after private function)
3. function withdrawRemainingTokens(address to_) public override onlyOwner { L54 (public function can not go after internal function)

Description:

According to official solidity documentation functions should be grouped according to their visibility and ordered:

• constructor

• receive function (if exists)

• fallback function (if exists)

• external

• public

• internal

• private

Within a grouping, place the view and pure functions last.

Recommendation:

Put the functions in the correct order according to the documentation.

[N-5]: NatSpec is missing

Context:

1. function initialize( L37
2. function initialize( L43
3. constructor( L26
4. function initialize( L32
5. constructor( L17
6. constructor( L13
7. function generateDataURI( L40

[N-6]: Typos

Context:

1. /// @dev set or remave a contract address to be used as a reward L176 (Change remave to remove)

[N-7]: Line is too long

Context:

1. return ReceiptRendererContract.generateTokenURI(tokenId_, questId, totalParticipants, claimed, rewardAmount, rewardAddress); L172
2. /// @dev Only the owner of the Quest can call this function. Also requires that the Quest has started (not by date, but by calling the start function) L56
3. /// @dev Only the owner of the Quest can call this function. Also requires that the Quest has started (not by date, but by calling the start function) L62
4. /// @dev Function that gets the maximum amount of rewards that can be claimed by all users. It does not include the protocol fee L43
5. /// @notice Starts the quest by marking it ready to start at the contract level. Marking a quest ready to start does not mean that it is live. It also requires that the start time has passed L56
6. /// @dev Requires that the balance of the rewards in the contract is greater than or equal to the maximum amount of rewards that can be claimed by all users and the protocol L57
7. /// @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 L79

Description:

Maximum suggested line length is 120 characters.