RabbitHole Quest Protocol contest - Deivitto's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/32a9b58f040442af4c7f459fe409e40af0e54a78/contracts/RabbitHoleTickets.sol#L48 https://github.com/rabbitholegg/quest-protocol/blob/32a9b58f040442af4c7f459fe409e40af0e54a78/contracts/RabbitHoleTickets.sol#L47-L50 https://github.com/rabbitholegg/quest-protocol/blob/32a9b58f040442af4c7f459fe409e40af0e54a78/contracts/RabbitHoleTickets.sol#L92 https://github.com/rabbitholegg/quest-protocol/blob/32a9b58f040442af4c7f459fe409e40af0e54a78/contracts/RabbitHoleTickets.sol#L83 https://github.com/rabbitholegg/quest-protocol/blob/bd213e8629bb8587dd4bb35f3e9e8f8e42b40336/contracts/RabbitHoleReceipt.sol#L98

Vulnerability details

Minting can be called by anyone

Summary

Modifier is wrongly implemented, so every function that uses onlyMinter will be callable by anyone.

This affects:

• RabbitHoleTickets#mintBatch()

• RabbitHoleTickets#mint()

• RabbitHoleReceipt#mint()

Vulnerability Detail

Modifier has no if + revert / require statement within the condition, therefore, it will always pass the "checks"

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

This means one can mint all the tokens / NFTs he wants and claim rewards after that (Claim flow).

Impact

Minting NFTs even if it shouldn't be able. Callable as much as wanted, direct impact on the value of the protocol.

Code Snippet

https://github.com/rabbitholegg/quest-protocol/blob/32a9b58f040442af4c7f459fe409e40af0e54a78/contracts/RabbitHoleTickets.sol#L48

https://github.com/rabbitholegg/quest-protocol/blob/32a9b58f040442af4c7f459fe409e40af0e54a78/contracts/RabbitHoleTickets.sol#L47-L50

Tool used

Manual Review

Recommendation

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

Gas

Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key’s keccak256 hash (Gkeccak256 - 30 gas) and that calculation’s associated stack operations.

Both mapping being used in the same functions mostly consider making them a struct instead

• QuestFactory.sol#L20 mapping(address => bool) addressMinted;

• QuestFactory.sol#L30 mapping(address => bool) public rewardAllowlist;

• RabbitHoleReceipt.sol#L30 mapping(uint => string) public questIdForTokenId;

• RabbitHoleReceipt.sol#L34 mapping(uint => uint) public timestampForTokenId;

state variables can be packed into fewer storage slots

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables are also cheaper.

consider making this state var near one of the address types

• Quest.sol#L14 address public immutable rewardToken;

• Quest.sol#L19 bool public hasStarted;

• Quest.sol#L20 bool public isPaused;

<x> += <y> costs more gas than <x> = <x> + <y> for state variables

Using the addition operator instead of plus-equals saves gas

• Quest.sol#L115 redeemedTokens += redeemableTokenCount;

Not using the named return variables when a function returns, wastes deployment gas

• RabbitHoleReceipt.sol#L181 ) external view override returns (address receiver, uint256 royaltyAmount) {

• RabbitHoleTickets.sol#L112 ) external view override returns (address receiver, uint256 royaltyAmount) {

++i / i++ should be unchecked{++i} / unchecked{i++} when it is not possible for them to overflow, as is the case when used in for-loop and while-loops

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

• RabbitHoleReceipt.sol#L117 for (uint i = 0; i < msgSenderBalance; i++) {

• RabbitHoleReceipt.sol#L128 for (uint i = 0; i < msgSenderBalance; i++) {

• Quest.sol#L70 for (uint i = 0; i < tokenIds_.length; i++) {

• Quest.sol#L104 for (uint i = 0; i < tokens.length; i++) {

internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

• QuestFactory.sol#L152 function grantDefaultAdminAndCreateQuestRole(address account_) internal {

• RabbitHoleReceipt.sol#L139 function burn(uint256 tokenId) internal override(ERC721Upgradeable, ERC721URIStorageUpgradeable) {

• Quest.sol#L122 function calculateRewards(uint256 redeemableTokenCount) internal virtual returns (uint256) {

• Quest.sol#L129 function transferRewards(uint256 amount) internal virtual {

• Erc20Quest.sol#L66 function transferRewards(uint256 amount) internal override {

• Erc20Quest.sol#L74 function calculateRewards(uint256 redeemableTokenCount) internal view override returns (uint256) {

• Erc1155Quest.sol#L41 function transferRewards(uint256 amount) internal override {

• Erc1155Quest.sol#L48 function calculateRewards(uint256 redeemableTokenCount) internal pure override returns (uint256) {

• RabbitHoleReceipt.sol#L153 ) internal override(ERC721Upgradeable, ERC721EnumerableUpgradeable) {

Extra check on booleans

There are instances where a boolean variable / function is checked against a boolean unnecessarily wasting extra ~18 gas for instance

• Checks can be simplified from variable == false to !variable.

• Checks can be simplified from variable == true to variable.

• QuestFactory.sol#L221 if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();

• Quest.sol#L136 return claimedList[tokenId_] == true;

• QuestFactory.sol#L73 if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();

Use Custom Errors

NOTE: None of these findings where found by 4naly3er output - Gas

Source Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors. This would save both deployment and runtime cost.

• RabbitHoleReceipt.sol#L161 require(exists(tokenId), 'ERC721URIStorage: URI query for nonexistent token');

• RabbitHoleReceipt.sol#L162 require(QuestFactoryContract != IQuestFactory(address(0)), 'QuestFactory not set');

• RabbitHoleReceipt.sol#L182 require(exists(tokenId), 'Nonexistent token');