RabbitHole Quest Protocol contest - RaymondFam'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/RabbitHoleTickets.sol#L47-L50

Vulnerability details

Impact

In RabbitHoleReceipt.sol and RabbitHoleTickets.sol, the modifier onlyMinter() is missing a critical custom error if msg.sender != minterAddress. This will lead to serious consequences where the public function, mint() and mintBatch() can be called by anyone at will.

Proof of Concept

File: RabbitHoleReceipt.sol#L58-L61 File: RabbitHoleTickets.sol#L47-L50

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

File: RabbitHoleReceipt.sol#L98-L104

    function mint(address to_, string memory questId_) public onlyMinter {
        _tokenIds.increment();
        uint newTokenID = _tokenIds.current();
        questIdForTokenId[newTokenID] = questId_;
        timestampForTokenId[newTokenID] = block.timestamp;
        _safeMint(to_, newTokenID);
    }

File: RabbitHoleTickets.sol#L83-L99

    function mint(address to_, uint256 id_, uint256 amount_, bytes memory data_) public onlyMinter {
        _mint(to_, id_, amount_, data_);
    }

    function mintBatch(
        address to_,
        uint256[] memory ids_,
        uint256[] memory amounts_,
        bytes memory data_
    ) public onlyMinter {
        _mintBatch(to_, ids_, amounts_, data_);
    }

As can be seen from the function visibility above, mint() and mintBatch() are supposedly onlyMinter() guarded where the caller must be set as Minter on the receipt contract or the allowed minter, i.e. msg.sender == minterAddress. But given the modifier logic, a non-minter will simply make msg.sender == minterAddress false, allowing the rest of the code logic of mint() and mintBatch() proceed uninterrupted.

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider having the affected modifier refactored as follows:

+    error InvalidCaller();

    modifier onlyMinter() {
-        msg.sender == minterAddress;
+        if (msg.sender != minterAddress) revert InvalidCaller();
        _;
    }

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L60 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L114 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L41-L43

Vulnerability details

Impact

Unlike Erc20Quest.sol, owner of Erc1155Quest.sol is going to withdraw the remaining tokens from the contract when block.timestamp == endTime without deducting the unclaimedTokens. As a result, users will be denied of service when attempting to call the inherited claim() from Quest.sol.

Proof of Concept

As can be seen from the code block below, when the Quest time has ended, withdrawRemainingTokens() is going to withdraw the remaining tokens from the contract on line 60:

File: Erc1155Quest.sol#L52-L63

    /// @dev Withdraws the remaining tokens from the contract. Only able to be called by owner
    /// @param to_ The address to send the remaining tokens to
    function withdrawRemainingTokens(address to_) public override onlyOwner {
        super.withdrawRemainingTokens(to_);
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
60:            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
            '0x00'
        );
    }

When a user tries to call claim() below, line 114 is going to internally invoke _transferRewards():

File: Quest.sol#L94-L118

    /// @notice Allows user to claim the rewards entitled to them
    /// @dev User can claim based on the (unclaimed) number of tokens they own of the Quest
    function claim() public virtual onlyQuestActive {
        if (isPaused) revert QuestPaused();

        uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

        if (tokens.length == 0) revert NoTokensToClaim();

        uint256 redeemableTokenCount = 0;
        for (uint i = 0; i < tokens.length; i++) {
            if (!isClaimed(tokens[i])) {
                redeemableTokenCount++;
            }
        }

        if (redeemableTokenCount == 0) revert AlreadyClaimed();

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

        emit Claimed(msg.sender, totalRedeemableRewards);
    }

safeTransferFrom() is going to revert on line 42 because the token balance of the contract is now zero. i.e. less than amount_:

File: Erc1155Quest.sol#L39-L43

    /// @dev Transfers the reward token `rewardAmountInWeiOrTokenId` to the msg.sender
    /// @param amount_ The amount of reward tokens to transfer
    function _transferRewards(uint256 amount_) internal override {
42:        IERC1155(rewardToken).safeTransferFrom(address(this), msg.sender, rewardAmountInWeiOrTokenId, amount_, '0x00');
    }

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider refactoring withdrawRemainingTokens() as follows:

(Note: The contract will have to separately import {QuestFactory} from './QuestFactory.sol' and initialize questFactoryContract.

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

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

+        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens)
+        uint256 nonClaimableTokens = IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId) - unclaimedTokens;
        IERC1155(rewardToken).safeTransferFrom(
            address(this),
            to_,
            rewardAmountInWeiOrTokenId,
-            IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
+            nonClaimableTokens,
            '0x00'
        );
    }

No storage gap for upgradeable contracts

Consider adding a storage gap at the end of an upgradeable contract, just in case it would entail some child contracts in the future. This would ensure no shifting down of storage in the inheritance chain.

+ uint256[50] private __gap;

Here are the contract instances entailed:

File: QuestFactory.sol File: RabbitHoleReceipt.sol

uint256 over uint

Across the codebase, there are numerous instances of uint, as opposed to uint256. In favor of explicitness, consider replacing all instances of uint with uint256.

Here are some of the instances entailed:

File: QuestFactory.sol

22:        uint totalParticipants;
23:        uint numberMinted;

31:    uint public questFee;
32:    uint public questIdCount;

193:    function getNumberMinted(string memory questId_) external view returns (uint) {

199:    function questInfo(string memory questId_) external view returns (address, uint, uint) {

Unspecific compiler version pragma

For some source-units the compiler version pragma is very unspecific, i.e. ^0.8.15. While this often makes sense for libraries to allow them to be included with multiple different versions of an application, it may be a security risk for the actual application implementation itself. A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up actually checking a different EVM compilation that is ultimately deployed on the blockchain.

Avoid floating pragmas where possible. It is highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.

Non-compliant contract layout with Solidity's Style Guide

According to Solidity's Style Guide below:

https://docs.soliditylang.org/en/v0.8.17/style-guide.html

In order to help readers identify which functions they can call, and find the constructor and fallback definitions more easily, functions should be grouped according to their visibility and ordered in the following manner:

constructor, receive function (if exists), fallback function (if exists), external, public, internal, private

And, within a grouping, place the view and pure functions last.

Additionally, inside each contract, library or interface, use the following order:

type declarations, state variables, events, modifiers, functions

Consider adhering to the above guidelines for all contract instances entailed.

Use a more recent version of solidity

The protocol adopts version 0.8.15 on writing contracts. For better security, it is best practice to use the latest Solidity version, 0.8.17.

Security fix list in the versions can be found in the link below:

https://github.com/ethereum/solidity/blob/develop/Changelog.md

Add a Constructor Initializer

As per Openzeppelin's recommendation:

https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/6

The guidelines are now to prevent front-running of initialize() on an implementation contract, by adding an empty constructor with the initializer modifier. Hence, the implementation contract gets initialized atomically upon deployment.

This feature is readily incorporated in the Solidity Wizard since the UUPS vulnerability discovery. You would just need to check UPGRADEABILITY to have the following constructor instance below more equipped (just like it has been done so on RabbitHoleReceipt.sol) as follows:

File: QuestFactory.sol#L34-L35

    /// @custom:oz-upgrades-unsafe-allow constructor
-    constructor() initializer {}
+    constructor() {
+        _disableInitializers();
+    }

Lack of events for critical operations

Critical operations not triggering events will make it difficult to review the correct behavior of the deployed contracts. Users and blockchain monitoring systems will not be able to detect suspicious behaviors at ease without events. Consider adding events where appropriate for all critical operations for better support of off-chain logging API.

Here are the setter instances with missing events entailed:

File: QuestFactory.sol#L157-L189

159:    function setClaimSignerAddress(address claimSignerAddress_) public onlyOwner {

165:    function setProtocolFeeRecipient(address protocolFeeRecipient_) public onlyOwner {

172:    function setRabbitHoleReceiptContract(address rabbitholeReceiptContract_) public onlyOwner {

179:    function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {

186:    function setQuestFee(uint256 questFee_) public onlyOwner {

Ownership can be renounced

Inherited Openzeppelin’s OwnableUpgradeable.sol implements renounceOwnership(). This can represent a certain risk if the ownership is renounced for any other reason than by design. Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

Here is the contract instance entailed:

File: QuestFactory.sol

Lines too long

Lines in source code are typically limited to 80 characters, but it’s reasonable to stretch beyond this limit when need be as monitor screens theses days are comparatively larger. Considering the files will most likely reside in GitHub that will have a scroll bar automatically kick in when the length is over 164 characters, all code lines and comments should be split when/before hitting this length. Keep line width to max 120 characters for better readability where possible.

Here are some of the instances entailed:

File: IQuestFactory.sol#L16 File: Erc20Quest.sol#L56-L57

Inadequate NatSpec

Solidity contracts can use a special form of comments, i.e., the Ethereum Natural Language Specification Format (NatSpec) to provide rich documentation for functions, return variables and more. Please visit the following link for further details:

https://docs.soliditylang.org/en/v0.8.16/natspec-format.html

For instance, it will be of added values to the users and developers if:

• at least a minimalist NatSpec is provided on IQuestFactory.sol
• @return is included in the function NatSpec of getOwnedTokenIdsOfQuest()

Open TODOs

Open TODOs can point to architecture or programming issues that still need to be resolved. Consider resolving them before deploying.

Here are some of the instances entailed:

File: IQuest.sol#L4

// TODO clean this whole thing up

Camel casing input parameter names

State variable names should be conventionally camel cased where possible.

Here are some of the instances entailed:

File: RabbitHoleReceipt.sol#L35-L36

    ReceiptRenderer public ReceiptRendererContract;
    IQuestFactory public QuestFactoryContract;

Missing boundary check on royaltyFee

A sanity boundary check should be implemented on royaltyFee when calling setRoyaltyFee() just as it has been done so in setQuestFee().

File: RabbitHoleReceipt.sol#L90-L93

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

Use delete to clear variables

delete a assigns the initial value for the type to a. i.e. for integers it is equivalent to a = 0, but it can also be used on arrays, where it assigns a dynamic array of length zero or a static array of the same length with all elements reset. For structs, it assigns a struct with all members reset. Similarly, it can also be used to set an address to zero address or a boolean to false. It has no effect on whole mappings though (as the keys of mappings may be arbitrary and are generally unknown). However, individual keys and what they map to can be deleted: If a is a mapping, then delete a[x] will delete the value stored at x.

The delete key better conveys the intention and is also more idiomatic.

For instance, the a = false instance below may be refactored as follows:

File: Quest.sol#L64

-        isPaused = false;
+        delete isPaused;

Typo errors

File: QuestFactory.sol#L176

-    /// @dev set or remave a contract address to be used as a reward
+    /// @dev set or remove a contract address to be used as a reward

questIdCount should be initialized to 0

questIdCount in QuestFactory.sol is initialized to 1 when there is no quest created. Although there is no accounting mess up entailed, it should be initialized to 0 to more accurately reflect the number of quests created when its public getter is accessed.

Consider removing it from initialize() since not assigning anything to questIdCount is as good as keeping its default value, i.e. 0:

File: QuestFactory.sol#L37-L50

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

Parameterized custom errors

Custom errors can be parameterized to better reflect their respective error messages if need be.

For instance, the custom error instance below may be refactored as follows:

File: IQuestFactory.sol#L5

-    error QuestIdUsed();
+    error QuestIdUsed(string memory questId_);

File: QuestFactory.sol#L70

-        if (quests[questId_].questAddress != address(0)) revert QuestIdUsed();
+        if (quests[questId_].questAddress != address(0)) revert QuestIdUsed(questId_);

Boundary check

questFee, an immutable variable in Erc20Quest.sol, should have a boundary check just as it has been adopted in setQuestFee() of QuestFactory.sol.

File: Erc20Quest.sol#L38

+    error QuestFeeTooHigh();

+        if (questFee_ > 10_000) revert QuestFeeTooHigh();
        questFee = questFee_;

Events associated with setter functions

Consider having events associated with setter functions emit both the new and old values instead of just the new value.

Here are some of the instances entailed:

File: RabbitHoleTickets.sol

68:        emit RoyaltyFeeSet(royaltyFee_);

75:        emit MinterAddressSet(minterAddress_);

Over distribution of on-chain actions

The design of the protocol seems to be overselling or over-distributing on-chain actions as can be seen from the if block below:

File: QuestFactory.sol#L220

        if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint();

It is not sure whether or not the users are going to be compensated via a new Erc20Quest contract. If not, the amount of on-chain actions to be completed should not exceed totalParticipants to avoid these specific DOS grievances.

Threshold on boundary checks

It is recommended adding a threshold when setting boundary limits to the two opposite ends. For instance, the third if block in the constructor() of Quest.sol could end up having endTime_ = startTime_ + 1 and still bypass the revert in this edge instance.

Consider having the if block refactored by adding a threshold of 7 days or any other suitable period as follows:

        if (endTime_ <= block.timestamp) revert EndTimeInPast();
        if (startTime_ <= block.timestamp) revert StartTimeInPast();
-        if (endTime_ <= startTime_) revert EndTimeLessThanOrEqualToStartTime();
+        if (endTime_ <= startTime_ + 7 days) revert EndTimeLessThanOrEqualToStartTime(); 

Erc1155Quest unincentivized to promote on-chain actions

Devoid of protocolFee and its associated protocol rewards, Erc1155Quest is a lot less likely to have all participants complete the on-chain actions due to zero incentives given to promote the campaign.

Consider implementing maxProtocolRewards(), protocolFee(), and withdrawFee() like it has been done so in Erc20Quest where deemed fit.

Use of named returns for local variables saves gas

You can have further advantages in term of gas cost by simply using named return values as temporary local variable.

For instance, the code block below may be refactored as follows:

File: QuestFactory.sol#L61-L103

    function createQuest(
        address rewardTokenAddress_,
        uint256 endTime_,
        uint256 startTime_,
        uint256 totalParticipants_,
        uint256 rewardAmountOrTokenId_,
        string memory contractType_,
        string memory questId_
-    ) public onlyRole(CREATE_QUEST_ROLE) returns (address) {
+    ) public onlyRole(CREATE_QUEST_ROLE) returns (address newQuestAddr) {

            [ ... ]

-            return address(newQuest);
+            newQuestAddr = address(newQuest);

Avoid comparing boolean expressions to boolean literals

Comparing a boolean value to a boolean literal incurs the ISZERO operation and costs more gas than using a boolean expression.

Here are some of the affected code lines that may be refactored as follows:

File: QuestFactory.sol#L73

-            if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();
+            if (!rewardAllowlist[rewardTokenAddress_]) revert RewardNotAllowed();

File: QuestFactory.sol#L221

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

Non-strict inequalities are cheaper than strict ones

In the EVM, there is no opcode for non-strict inequalities (>=, <=) and two operations are performed (> + = or < + =).

As an example, consider replacing <= with the strict counterpart < in the following inequality instance:

File: Quest.sol#L35-L37

-        if (endTime_ <= block.timestamp) revert EndTimeInPast();
// Rationale for adding 1 on the right side of the inequality:
// x <= 10; [10, 9, 8, ...]
// x < 10 + 1 is the same as x < 11; [10, 9, 8 ...]
+        if (endTime_ < block.timestamp + 1) revert EndTimeInPast();

Merging of two opposing functions into a toggling function

pause() and unpause() in Quest.sol are two opposing functions that can be merged into a toggling function to save gas on contract deployment.

File: Quest.sol#L57-L65

-    function pause() public onlyOwner onlyStarted {
-        isPaused = true;
-    }

-    function unPause() public onlyOwner onlyStarted {
-        isPaused = false;
-    }

+    function pauser(bool status_) public onlyOwner onlyStarted {
+        isPaused = status_;
+    }

Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier instance below may be refactored as follows:

File: Quest.sol#L88-L92

+    function _onlyQuestActive() private view {
+        if (!hasStarted) revert NotStarted();
+        if (block.timestamp < startTime) revert ClaimWindowNotStarted();
+    }

    modifier onlyQuestActive() {
-        if (!hasStarted) revert NotStarted();
-        if (block.timestamp < startTime) revert ClaimWindowNotStarted();
+        _onlyQuestActive();
        _;
    }

Use storage instead of memory for structs/arrays

A storage pointer is cheaper since copying a state struct in memory would incur as many SLOADs and MSTOREs as there are slots. In another words, this causes all fields of the struct/array to be read from storage, incurring a Gcoldsload (2100 gas) for each field of the struct/array, and then further incurring an additional MLOAD rather than a cheap stack read. As such, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables will be much cheaper, involving only Gcoldsload for all associated field reads. Read the whole struct/array into a memory variable only when it is being returned by the function, passed into a function that requires memory, or if the array/struct is being read from another memory array/struct.

Here are some of the instances entailed:

File: RabbitHoleReceipt.sol

114:        uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);

125:        uint[] memory filteredTokens = new uint[](foundTokens);

Redundant modifier visibility

withdrawRemainingTokens() in Erc20Quest.sol and Erc1155Quest.sol has super.withdrawRemainingTokens() execute in the first code line of the function logic. Since the parental function will have the same modifier onlyOwner run again, the child function may have the same modifier removed from its visibility (just as it has been done on start()) to save gas both on contract deployment and function calls:

File: Erc20Quest.sol#L81-L82 File: Erc1155Quest.sol#L54-L55

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

        [ ... ]

+= and -= cost more gas

+= and -= generally cost 22 more gas than writing out the assigned equation explicitly. The amount of gas wasted can be quite sizable when repeatedly operated in a loop.

For instance, the += instance below may be refactored as follows:

File: Quest.sol#L115

-        redeemedTokens += redeemableTokenCount;
+        redeemedTokens = redeemedTokens + redeemableTokenCount;

Function order affects gas consumption

The order of function will also have an impact on gas consumption. Because in smart contracts, there is a difference in the order of the functions. Each position will have an extra 22 gas. The order is dependent on method ID. So, if you rename the frequently accessed function to more early method ID, you can save gas cost. Please visit the following site for further information:

https://medium.com/joyso/solidity-how-does-function-name-affect-gas-consumption-in-smart-contract-47d270d8ac92

Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.15",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.