RabbitHole Quest Protocol contest - cryptonue's results

Lines of code

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

Vulnerability details

Impact

Remaining (unclaimed) rewardToken can be drained through withdrawFee()

Proof of Concept

the withdrawFee() function is public function with a onlyAdminWithdrawAfterEnd. By name of the modifier it suppose to be onlyAdmin, but in the implementation:

File: Erc20Quest.sol
102:     function withdrawFee() public onlyAdminWithdrawAfterEnd {
103:         IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
104:     }

it doesn't really limit the onlyAdmin part, so anyone can call this function, after the claim period ends.

File: Quest.sol
76:     modifier onlyAdminWithdrawAfterEnd() {
77:         if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
78:         _;
79:     }

What really the problem here is, the withdrawFee() function doesn't check if the protocol fee already withdrawn, so it can be called multiple times.

This will drain the rewardToken of unclaimed rewards, any owner's excess token which supposed to be claimed withdrawRemainingTokens will be drained.

If withdrawFee() is correctly limited by onlyAdmin modifier, this issue might not be happen (as the control is in admin hand), so the admin can just call the function after owner call withdrawRemainingTokens(). But the withdrawFee() can be called by anyone, therefore the draining of remaining token will happen. (Ignoring the fact that the destination transfer of it will be the protocolFeeRecipient). The effect is the owner can't reclaim of remaining token, so, this is a HIGH issue.

Tools Used

Manual analysis

Recommended Mitigation Steps

There should be a variable to store how many redeemer when the withdrawFee() is being called, and compare it every time when withdrawFee() is being called again.

    uint256 redeemersCounter = 0;

    function withdrawFee() public onlyAdminWithdrawAfterEnd {
        uint256 diff = receiptRedeemers() - redeemersCounter;
        require(diff > 0, "Nothing to withdraw");

        redeemersCounter = receiptRedeemers();

        uint256 withdrawableFee = (diff * rewardAmountInWeiOrTokenId * questFee) / 10_000;

        IERC20(rewardToken).safeTransfer(protocolFeeRecipient, withdrawableFee);
    }

[L] Ownable two step transfer

The protocol use openzeppelin ownable contract import {OwnableUpgradeable} from '@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol';. This contract doesn't have a two step transfer pattern.

Recommend considering implementing a two step process where the owner or admin nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

[L] Missing onlyAdmin part (or wrong modifier name)

File: Quest.sol
76:     modifier onlyAdminWithdrawAfterEnd() {
77:         if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
78:         _;
79:     }

The onlyAdmin part is not defined in the modifier, it only check the time. Either the modifier name is wrong, might be onlyWithdrawAfterEnd since the only usage is on withdrawRemainingTokens() function which already include the onlyOwner, or indeed the modifier is missing the onlyAdmin part/implementation.

File: Quest.sol
150:     function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}

[L] Inexistent Sanitization of Input Address

Zero address check. The linked address argument affects a sensitive contract variable yet remains unsanitized. We advise it to be sanitized against the zero-address (address(0)) to prevent misconfiguration of the contract.

File: QuestFactory.sol
37:     function initialize(
38:         address claimSignerAddress_,
39:         address rabbitholeReceiptContract_,
40:         address protocolFeeRecipient_
41:     ) public initializer {

File: Quest.sol
40:         rewardToken = rewardTokenAddress_;

[L] Inexistent Event Emissions

The linked functions adjust sensitive contract variables yet do not emit an event for them. We advise an event to be coded and correspondingly emitted to ensure off-chain processes can properly react to these changes.

File: RabbitHoleReceipt.sol
63:     /// @dev set the receipt renderer contract
64:     /// @param receiptRenderer_ the address of the receipt renderer contract
65:     function setReceiptRenderer(address receiptRenderer_) public onlyOwner {
66:         ReceiptRendererContract = ReceiptRenderer(receiptRenderer_);
67:     }
68:
69:     /// @dev set the royalty recipient
70:     /// @param royaltyRecipient_ the address of the royalty recipient
71:     function setRoyaltyRecipient(address royaltyRecipient_) public onlyOwner {
72:         royaltyRecipient = royaltyRecipient_;
73:     }
74:
75:     /// @dev set the quest factory contract
76:     /// @param questFactory_ the address of the quest factory contract
77:     function setQuestFactory(address questFactory_) public onlyOwner {
78:         QuestFactoryContract = IQuestFactory(questFactory_);
79:     }

More on QuestFactory.sol

[N] Typos

• remave

[N] Refactor

This two function can be packed into setPause(bool status) function, further more add a check require to prevent same state changes.

File: Quest.sol
57:     function pause() public onlyOwner onlyStarted {
58:         isPaused = true;
59:     }
60:
61:     /// @notice Unpauses the Quest
62:     /// @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)
63:     function unPause() public onlyOwner onlyStarted {
64:         isPaused = false;
65:     }

[N] Standarize usage of uint vs uint256

There are some interchangeable type being use in the project, uint and uint256, even thought it's the same meaning, but it's better to keep in standard practice, only use one.

Save gas by create a compare function for string comparation

Seems kind of expensive for strings that have a different length, which could be most of the time.

https://fravoll.github.io/solidity-patterns/string_equality_comparison.html

    function compare(string memory str1, string memory str2) public pure returns (bool) {
        return (bytes(str1).length == bytes(str2).length) && keccak256(abi.encodePacked(str1)) == keccak256(abi.encodePacked(str2));
    }

In createQuest we can rewrite

        if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc20')))
        if (keccak256(abi.encodePacked(contractType_)) == keccak256(abi.encodePacked('erc1155')))

with

        if (compare(contractType_, 'erc20'))
        if (compare(contractType_, 'erc1155'))

Returning the boolean value directly

File: Quest.sol
135:     function isClaimed(uint256 tokenId_) public view returns (bool) {
136:         return claimedList[tokenId_] == true;
137:     }

the claimList itself is a boolean, just return the claimedList[tokenId_] rather than comparing it with true. comparing with a true is unnecessary, removing it will save some gas and will return the same output. furthermore, if needed just use the claimedList public variable directly to save some more gas.

Save gas by preventing same action or action which doesn't change anything

for example:

File: QuestFactory.sol
179:     function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
180:         rewardAllowlist[rewardAddress_] = allowed_;
181:     }

we can add check, like require(rewardAllowlist[rewardAddress_] != allowed_, "No changes");

    function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) public onlyOwner {
        require(rewardAllowlist[rewardAddress_] != allowed_, "No changes");
        rewardAllowlist[rewardAddress_] = allowed_;
    }

Integer overflow/underflow

Prior to Solidity 0.8.0 version, interger overflow and underflow checks were performed by using the SafeMath library. From Solidity 0.8.0 upward, the compiler does that check for us.

This extra check cost gas. If we know that the mathematical operations we will perform inside the contract will not overflow or underflow, we can tell the compiler not to check for overflow or underflow in the operation.

If we know that our mathematics will be safe we could safe a little gas by using unchecked.

File: QuestFactory.sol
101:             ++questIdCount;

File: Quest.sol
70:         for (uint i = 0; i < tokenIds_.length; i++) {
71:             claimedList[tokenIds_[i]] = true;
72:         }

the gas-saving pattern for the for-loops by using unchecked:

for (uint256 i; i < tokenIds_.length; ) {
  claimedList[tokenIds_[i]] = true;
  unchecked {
    ++i;
  }
}

Unnecessary loop

File: RabbitHoleReceipt.sol
125:         uint[] memory filteredTokens = new uint[](foundTokens);
126:         uint filterTokensIndexTracker = 0;
127:
128:         for (uint i = 0; i < msgSenderBalance; i++) {
129:             if (tokenIdsForQuest[i] > 0) {
130:                 filteredTokens[filterTokensIndexTracker] = tokenIdsForQuest[i];
131:                 filterTokensIndexTracker++;
132:             }
133:         }

the if (tokenIdsForQuest[i] > 0) will always return true because tokenIdsForQuest[i] = tokenId;, so it is unnecssary to have the filteredTokens. Removing it will save gas.

Flatten internal function which being use only once

The grantDefaultAdminAndCreateQuestRole is only being used once, thus it's better to flatten this function.

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