RabbitHole Quest Protocol contest - sakshamguruji's results

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98-L103

Vulnerability details

Impact

All the checks here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L220-L223 can be circumvented and multiple receipts can be minted for a single address , more than max allowed receipts can be minted and invalid hashes/signatures can be passed.

Proof of Concept

The mintReceipt function here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L228 calls the mint function in the RabbitHoleReceipt contract . The mint function in the RabbitHoleReceipt here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98 instead of being an internal function is declared as a public function , which means it can be called directly without interacting with the mintReceipt function in the QuestFactory. Even though the function is protected with the onlyMinter role , this possess the risk of letting the checks being circumvented and letting the minter(highly centralised) to mint multiple receipts for an address ( by keep calling the mint function with the same values) , minting more than the max allowed .

To mint multiple receipts or more than allowed receipts we interact with the mint function in the RabbitHoleReceipt contract as - 1.) Call the function with to_ address with an address the minter wishes to give receipt to , and the questId_ 2.) Simply call the function again with the same to_ and questId_ to mint the same person another receipt. 3.) To mint more than allowed simply keep minting as there are no checks in this function.

Tools Used

Manual analysis , VSCode

Recommended Mitigation Steps

Make the mint function internal

QUEST SHOULD NOT BE PAUSED IF IT HAS NOT STARTED YET

Description:

In the function pause here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L57 there should be a check something like if(!hasStarted) revert because we should not be able to pause a quest that has not been started yet.

DECLARE A VARIABLE FOR MAXIMUM FEE

Description:

Instead of using 10_000 a global constant variable , something like MAX_QUEST_FEE should be declared and used to increase code readabiluty. https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L187

questFee SHOULD HAVE A MINIMUM CHECK TOO

Description:

The setter for questFee here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L186 should have a minimum check too , something like require(questFee > MIN_FEE) or should not be 0 .

USE OF BYTES.CONCAT() INSTEAD OF ABI.ENCODEPACKED(,)

Description:

Rather than using abi.encodePacked for appending bytes, since version 0.8.4, bytes.concat() is enabled

Since version 0.8.4 for appending bytes, bytes.concat() can be used instead of abi.encodePacked(,).

Affected Instances: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L72 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L105 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L211 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L222

VARIABLES SHOULD BE RENAMED

Description:

The variable unclaimedTokens here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L84 and the variable nonClaimableTokens here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L85 should be renamed to something like unclaimedTokensValue and nonClaimableTokensValue as these variables depict the amount these non claimable tokens are worth and not the amount of tokens.

LACK OF ZERO ADDRESS/AMOUNT CHECKS

Description:

There lacks zero address check at some places throughout the codebase , it can be problematic if these checks are missed in functions like transfer(transferring to a zero address i.e. loss of funds) , mint or setting up privileged roles.

Affected instances: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L179 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L46 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L84 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L91 (fee should not be 0) https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L98 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L149 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L150 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L35 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L66 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L73 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L83 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L93 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L38 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L54

MISSING COMMENTS/INFORMATION ABOUT STORAGE VARIABLES

Description:

Even though all the functions have been well documented the storage variables lacks information , there should be proper comments describing the purpose of each storage variable , for example here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L30-L36 there is no information about these variables.

ALWAYS CHECK IF OPENZEPPELIN CONTRACTS ARE UP TO DATE

Description:

The openzeppelin version used is 4.8.0 whereas the latest version is 4.8.1 , it should always be ensured that latest version is used to avoid errors/bugs in the previous versions.

TYPO

Description:

Change remave to remove here https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L176