RabbitHole Quest Protocol contest - Aymen0909'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 the RabbitHoleReceipt and RabbitHoleTickets contracts the minterAddress should be the only account allowed to mint a new token, but due to an error in the onlyMinter modifier all the users are able to mint new tokens without permission, the impact of this issue is that any user can mint a new token and use it to claim a reward without even completing the quest or having a signed hash message.

Proof of Concept

This issue occurs because of an error in the onlyMinter modifier in both contracts :

File: RabbitHoleReceipt.sol Line 58-61

File: RabbitHoleTickets.sol Line 47-50

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

As you can see from the code above the modifier does not contain a require/revert statement which will revert in case non minter tries to call the function, but instead the modifier just compare the value of msg.sender and the value of minterAddress and then continues to executing the function regardless of the result of the comparison, and thus any address that call the mint functions and mint new tokens without permission.

Tools Used

Manual review

Recommended Mitigation Steps

Add a require/revert in the onlyMinter modifier for both contracts as follows :

modifier onlyMinter() {
    require(msg.sender == minterAddress, "only minter");
    _;
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54-L63

Vulnerability details

Impact

After completing their tasks users can mint a new receipt token which they can later claim reward with it using the claim function, this function can not be called when the Quest contract is paused so the users can't claim when quest contract is paused.

After the end of the quest the owner is able to withdraw back the remaining tokens, in the case of Erc20Quest the owner is able to withdraw only the reward tokens for the non-participants and the participants rewards are kept in the contract until they get claimed but for the Erc1155Quest the owner is able to withdraw all tokens deposited when starting the quest regardless of the already minted receipt.

Thus for an Erc1155Quest type quest the scenario where users can not claim their reward is the following :

• the owner start the Erc1155Quest by calling the start() function and then he set the contract to paused.
• The users can complete the tasks and mint a receipt token by calling the mintReceipt function.
• users can not claim reward for their receipt token because the Quest contract is paused.
• the owner wait until the end of the quest and then calls the withdrawRemainingTokens function to get back all the tokens previously deposited at the start.
• in the end the users did complete their tasks but they will never receive the reward for it.

Proof of Concept

This issue occurs because withdrawRemainingTokens function of the Erc1155Quest contract allow the owner to withdraw all the tokens deposited at the start :

File: Erc1155Quest.sol Line 54-63

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);
    IERC1155(rewardToken).safeTransferFrom(
        address(this),
        to_,
        rewardAmountInWeiOrTokenId,
        IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
        '0x00'
    );
}

On the contrary to the withdrawRemainingTokens function of the Erc20Quest which account for the already minted tokens by calling questFactoryContract.getNumberMinted(questId) and thus allow the owner to only withdraw reward correspanding to non-minted tokens, the withdrawRemainingTokens function of the Erc1155Quest allow the owner to withdraw all the reward tokens regardless if they correspand to a minted receipt.

This will allow the owner to follow the scenario mentioned before to stop users for claiming their rewards.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid this issue i recommend to modify the withdrawRemainingTokens function of the Erc1155Quest and make work similarly to the one in Erc20Quest.

QA Report

Summary

Findings

1- royaltyFee should have a maximum value of 10000 :

In both RabbitHoleReceipt and RabbitHoleTickets contracts the royaltyFee is set in basis point to calculate the fee taken from the sale price, the maximum fee should be 10000 (the denominator in the royaltyInfo function) but the setRoyaltyFee function doesn't check the new fee which could be greater than 10000 and in that case funds greater than the actual sale price will be transferred to the fee recepient.

Risk : Low

Proof of Concept

Issue occurs in the instance below :

File: RabbitHoleReceipt.sol Line 90-93

/** @audit
    owner can set royaltyFee to any value
*/
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
    royaltyFee = royaltyFee_;
    emit RoyaltyFeeSet(royaltyFee_);
}

File: RabbitHoleTickets.sol Line 66-69

/** @audit
    owner can set royaltyFee to any value
*/
function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
    royaltyFee = royaltyFee_;
    emit RoyaltyFeeSet(royaltyFee_);
}

As you can see the owner can set any value for royaltyFee variables even type(uint256).max.

Mitigation

To avoid this issue i recommend to add a check in the setRoyaltyFee function to ensure that the royaltyFee is always less than 10000, the setRoyaltyFee function should be modified as follow :

function setRoyaltyFee(uint256 royaltyFee_) public onlyOwner {
    if (royaltyFee_ > 10_000) revert RoyaltyFeeTooHigh();
    royaltyFee = royaltyFee_;
    emit RoyaltyFeeSet(royaltyFee_);
}

2- Front-runable initialize function :

The initialize function is used in the QuestFactory contracts to initialize important contract parameters (ownership & access control), there is the issue that any attacker can initialize the contract before the legitimate deployer and even if the developers when deploying call immediately the initialize function, malicious agents can trace the protocol deployment transactions and insert their own transaction between them and by doing so they front run the developers call and gain the ownership of the contract and set the wrong parameters.

The impact of this issue is :

• In the best case developers notice the problem and have to redeploy the contract and thus costing more gas.

• In the worst case the protocol continue to work with the wrong owner and state parameters which could lead to the loss of user funds.

Risk : Low

Proof of Concept

Instances include:

File: QuestFactory.sol Line 37-50

function initialize(
    address claimSignerAddress_,
    address rabbitholeReceiptContract_,
    address protocolFeeRecipient_
) public

Mitigation

It's recommended to use the constructor to initialize non-proxied contracts.

For initializing proxy (upgradable) contracts deploy contracts using a factory contract that immediately calls initialize after deployment or make sure to call it immediately after deployment and verify that the transaction succeeded.

3- Setter functions should emit an event :

The setter functions which are used to change critical protocol parameters (allow reward tokens, fees,...) should emit an event to allow all users and dapps to detect those changes and act correspondly.

Risk : NON CRITICAL

Proof of Concept

The are many setter function which don't emit and event :

File: QuestFactory.sol Line 179

function setRewardAllowlistAddress(address rewardAddress_, bool allowed_) 

File: QuestFactory.sol Line 186

function setQuestFee(uint256 questFee_) public

File: Quest.sol Line 50

function start() public virtual onlyOwner

File: Quest.sol Line 57

function pause() public onlyOwner onlyStarted

File: Quest.sol Line 63

function unPause() public onlyOwner onlyStarted

Mitigation

Emit an event in the setter functions aforementioned.

4- pause() and unPause() functions can be refactored into a single function :

Risk : NON CRITICAL

The pause() and unPause() functions from the Quest contract can be refactored into a single function setPause which can help reduce contract code size :

function setPause(bool _pause) public onlyOwner onlyStarted {
    if(isPaused != _pause){
        isPaused = _pause;
    }
}

5- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: RabbitHoleReceipt.sol Line 181

returns (address receiver, uint256 royaltyAmount)

File: RabbitHoleTickets.sol Line 112

returns (address receiver, uint256 royaltyAmount)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

6- Use scientific notation :

When using multiples of 10 you shouldn't use decimal literals or exponentiation (e.g. 1000000, 10**18) but use scientific notation for better readability.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: QuestFactory.sol Line 187

if (questFee_ > 10_000)

File: Erc20Quest.sol Line 53

return (maxTotalRewards() * questFee) / 10_000;

File: Erc20Quest.sol Line 97

return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;

File: RabbitHoleReceipt.sol Line 184

uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;

File: RabbitHoleTickets.sol Line 113

uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;

Mitigation

Use scientific notation for the instances aforementioned.

Gas Optimizations

Summary

Findings

1- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, 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.

There are 2 instances of this issue :

File: contracts/BondNFT.sol

30      mapping(uint => string) public questIdForTokenId;
34      mapping(uint => uint) public timestampForTokenId;

These mappings could be refactored into the following struct and mapping for example :

struct Receipt {
    uint256 questId;
    uint256 timestamp;
}
    
mapping(uint => Receipt) private _receiptForTokenId;

2- Variables inside struct should be packed to save gas :

As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings.

There is 1 instance of this issue:

File: QuestFactory.sol Line 19-24

struct Quest {
    mapping(address => bool) addressMinted;
    address questAddress;
    uint totalParticipants;
    uint numberMinted;
}

As the totalParticipants and numberMinted parameters values can't really overflow 2^128 their values should be packed together and the struct should be modified as follow to save gas :

struct Quest {
    mapping(address => bool) addressMinted;
    address questAddress;
    uint128 totalParticipants;
    uint128 numberMinted;
}

3- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There is 1 instance of this issue:

File: QuestFactory.sol Line 226

quests[questId_].numberMinted++;

The above operation cannot overflow due to the check :

File: QuestFactory.sol Line 220

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

This check ensures that we always have numberMinted <= totalParticipants so the value of numberMinted can not overflow and the operation should be marked as unchecked.

4- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 30 instances of this issue:

File: Quest.sol Line 115

redeemedTokens += redeemableTokenCount;

5- Don't compare boolean expressions to boolean literals :

The following formulas should be used : if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)

File: QuestFactory.sol

73       if (rewardAllowlist[rewardTokenAddress_] == false) revert RewardNotAllowed();
221      if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();

6- require() strings longer than 32 bytes cost extra gas :

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas.

There is 1 instance of this issue :

File: RabbitHoleReceipt.sol Line 161

require(_exists(tokenId_), 'ERC721URIStorage: URI query for nonexistent token');

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

There are 4 instances of this issue:

File: Quest.sol

70      for (uint i = 0; i < tokenIds_.length; i++)
104     for (uint i = 0; i < tokens.length; i++)

File: RabbitHoleReceipt.sol

117     for (uint i = 0; i < msgSenderBalance; i++)
128     for (uint i = 0; i < msgSenderBalance; i++)