RabbitHole Quest Protocol contest - arialblack14's results

QA REPORT

---

Summary of low risk issues

Total: 2 issues.

Summary of non-critical issues

Total: 3 issues.

---

Low Risk Issues

<a id=L1>[L-1]</a> Use of block.timestamp

Description

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommendation

Use oracle instead of block.timestamp

Instances (5):

File: 2023-01-rabbithole/quest-protocol/contracts/Quest.sol

35: if (endTime_ <= block.timestamp) revert EndTimeInPast();
36: if (startTime_ <= block.timestamp) revert StartTimeInPast();
77: if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
90: if (block.timestamp < startTime) revert ClaimWindowNotStarted();

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

102: timestampForTokenId[newTokenID] = block.timestamp;

<a id=L5>[L-2]</a> Lock pragmas to specific compiler version.

Description

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile locally.

Recommendation

Ethereum Smart Contract Best Practices - Lock pragmas to specific compiler version. solidity-specific/locking-pragmas

Instances (10):

File: 2023-01-rabbithole/quest-protocol/contracts/Erc1155Quest.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/Erc20Quest.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/Quest.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/QuestFactory.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleTickets.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/ReceiptRenderer.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/TicketRenderer.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/interfaces/IQuest.sol

2: pragma solidity ^0.8.15;

File: 2023-01-rabbithole/quest-protocol/contracts/interfaces/IQuestFactory.sol

2: pragma solidity ^0.8.15;

Non-critical Issues

<a id=NC1>[NC-1]</a> Constants should be defined rather than using magic numbers

Description

Even assembly can benefit from using readable constants instead of hex/numeric literals

Recommendation

You should declare the variable instead of magic number.

Instances (6):

File: 2023-01-rabbithole/quest-protocol/contracts/Erc20Quest.sol

53: return (maxTotalRewards() * questFee) / 10_000;
97: return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;

File: 2023-01-rabbithole/quest-protocol/contracts/QuestFactory.sol

187: if (questFee_ > 10_000) revert QuestFeeTooHigh();

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

129: if (tokenIdsForQuest[i] > 0) {
184: uint256 royaltyPayment = (salePrice_ * royaltyFee) / 10_000;

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleTickets.sol

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

<a id=NC2>[NC-2]</a> Use scientific notation (e.g.1e18) rather than large multiples of 10 (e.g. 1000000)

Description

Use scientific notation (e.g.1e18) rather than large multiples of 10, that could lead to errors.

Recommendation

Use scientific notation instead of large multiples of 10.

Instances (5):

File: 2023-01-rabbithole/quest-protocol/contracts/Erc20Quest.sol

53: return (maxTotalRewards() * questFee) / 10_000;
97: return (receiptRedeemers() * rewardAmountInWeiOrTokenId * questFee) / 10_000;

File: 2023-01-rabbithole/quest-protocol/contracts/QuestFactory.sol

187: if (questFee_ > 10_000) revert QuestFeeTooHigh();

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

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

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleTickets.sol

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

<a id=NC3>[NC-3]</a> Lines are too long.

Description

Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

Recommendation

Reduce number of characters per line to improve readability.

Instances (3):

File: 2023-01-rabbithole/quest-protocol/contracts/Erc20Quest.sol

56: /// @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
57: /// @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

File: 2023-01-rabbithole/quest-protocol/contracts/interfaces/IQuestFactory.sol

16: event QuestCreated(address indexed creator, address indexed contractAddress, string indexed questId, string contractType, address rewardTokenAddress, uint256 endTime, uint256 startTime, uint256 totalParticipants, uint256 rewardAmountOrTokenId);

GAS OPTIMIZATION REPORT

---

Summary of optimizations

Total: 5 issues. Note: The findings for issue with id 1 are different from the c4udit output.

---

Gas Optimizations

<a id=G4>[G-1]</a> ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, for example when used in for and while loops

Description

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. Example for loop:

for (uint i = 0; i < length; i++) {
// do something that doesn't change the value of i
}

In this example, the for loop post condition, i.e., i++ involves checked arithmetic, which is not required. This is because the value of i is always strictly less than length <= 2**256 - 1. Therefore, the theoretical maximum value of i to enter the for-loop body is 2**256 - 2. This means that the i++ in the for loop can never overflow. Regardless, the overflow checks are performed by the compiler.

Unfortunately, the Solidity optimizer is not smart enough to detect this and remove the checks. You should manually do this by:

for (uint i = 0; i < length; i = unchecked_inc(i)) {
	// do something that doesn't change the value of i
}

function unchecked_inc(uint i) returns (uint) {
	unchecked {
		return i + 1;
	}
}

Or just:

for (uint i = 0; i < length;) {
	// do something that doesn't change the value of i
	unchecked { i++; 	}
}

Note that it’s important that the call to unchecked_inc is inlined. This is only possible for solidity versions starting from 0.8.2.

Gas savings: roughly speaking this can save 30-40 gas per loop iteration. For lengthy loops, this can be significant! (This is only relevant if you are using the default solidity checked arithmetic.)

Recommendation

Use the unchecked keyword

Instances (4):

File: 2023-01-rabbithole/quest-protocol/contracts/Quest.sol

70: for (uint i = 0; i < tokenIds_.length; i++) {

104: for (uint i = 0; i < tokens.length; i++) {

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

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

<a id=G6>[G-2]</a> Using storage instead of memory for structs/arrays saves gas.

Description

When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct(src)

Recommendation

Use storage for struct/array

Instances (2):

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

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

<a id=G7>[G-3]</a> Empty blocks should be removed or emit something.

Description

The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting. If the block is an empty if-statement block to avoid doing subsequent checks in the else-if/else conditions, the else-if/else conditions should be nested under the negation of the if-statement, because they involve different classes of checks, which may lead to the introduction of errors when the code is later modified solidity if(x){}else if(y){...}else{...} => if(!x){if(y){...}else{...}}

Recommendation

Empty blocks should be removed or emit something (The code should be refactored such that they no longer exist, or the block should do something useful, such as emitting an event or reverting.

Instances (1):

File: 2023-01-rabbithole/quest-protocol/contracts/Quest.sol

150: function withdrawRemainingTokens(address to_) public virtual onlyOwner onlyAdminWithdrawAfterEnd {}

<a id=G12>[G-3]</a> Internal functions only called once can be inlined to save gas.

Description

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls. Check this out: link Note: To sum up, when there is an internal function that is only called once, there is no point for that function to exist.

Recommendation

Remove the function and make it inline if it is a simple function.

Instances (4):

File: 2023-01-rabbithole/quest-protocol/contracts/Erc1155Quest.sol

41: function _transferRewards(uint256 amount_) internal override {

File: 2023-01-rabbithole/quest-protocol/contracts/Erc20Quest.sol

66: function _transferRewards(uint256 amount_) internal override {

File: 2023-01-rabbithole/quest-protocol/contracts/Quest.sol

129: function _transferRewards(uint256 amount_) internal virtual {

File: 2023-01-rabbithole/quest-protocol/contracts/QuestFactory.sol

152: function grantDefaultAdminAndCreateQuestRole(address account_) internal {

<a id=G13>[G-4]</a> >= costs less gas than >.

Description

The compiler uses opcodes GT and ISZERO for solidity code that uses >, but only requires LT for >=, which saves 3 gas

Recommendation

Use >= instead if appropriate.

Instances (3):

File: 2023-01-rabbithole/quest-protocol/contracts/QuestFactory.sol

187: if (questFee_ > 10_000) revert QuestFeeTooHigh();

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

File: 2023-01-rabbithole/quest-protocol/contracts/RabbitHoleReceipt.sol

129: if (tokenIdsForQuest[i] > 0) {

<a id=G15>[G-5]</a> <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Description

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

Recommendation

Use <x> = <x> + <y> instead.

Instances (1):

File: 2023-01-rabbithole/quest-protocol/contracts/Quest.sol

115: redeemedTokens += redeemableTokenCount;