RabbitHole Quest Protocol contest - glcanvas's results

Lines of code

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

Vulnerability details

Impact

All funds might be stilled to protocolFeeRecipient address, and it might be impossible to return them back.

Proof of Concept

When contest finishes:

    /// @notice Prevents reward withdrawal until the Quest has ended
    modifier onlyAdminWithdrawAfterEnd() {
        if (block.timestamp < endTime) revert NoWithdrawDuringClaim();
        _;
    }

anyone can call withdrawFee().

This function withdrawFee() sends protocolFee() funds to protocolFeeRecipient address. But after transaction executed and funds transferred contract allows to call this function once again.

Possible attack, which can at least ruin the reputation.

1. Quest finished.
2. Attacker calls withdrawFee() as many times as possible.
3. Users can't withdraw their earned tokens.

Tools Used

Manual audit

Recommended Mitigation Steps

1. Forbid call withdrawFee() function twice.
2. Add reentrancy guard (due to using transfer function)

bool feeWithdrawn;

function withdrawFee() public onlyAdminWithdrawAfterEnd reentrancyGuard {
    require(!feeWithdrawn, "already withdrawn");
    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
    feeWithdrawn = true; 
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L96-L98 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L89-L93 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L100-L104

Vulnerability details

Impact

More than enough funds might be redeemed from ERC20Quest.

Proof of Concept

1. Quest finished.
2. Only one user withdrawn their funds.
3. Function withdrawFee has been executed.
4. Another one user withdrawn their funds.
5. Function withdrawFee has been executed. -- But here protocolFeeRecipient will receive fees for TWO users, Although it should has received only for one.

Tools Used

Manual audit

Recommended Mitigation Steps

As I mentioned early add possibility to call withdrawFee() only once.

And in protocolFee() instead of calculating fees only for redeemed users calculates fee over all users.

    function protocolFee() public view returns (uint256) {
        return (maxTotalRewards() * rewardAmountInWeiOrTokenId * questFee) / 10_000;
    }

---

But your idea may be to send commissions only for those users who participated. In this case, you need to allow withdrawFee() to be called as many times as you want, but at the same time maintain the number of commissions already withdrawn and the number of participants who have collected their awards. Like this:

uint256 receivedFeesFoUsers;


function withdrawFee() public onlyAdminWithdrawAfterEnd { // TODO DON'T FORGET ADD REENTRANCY GUARD
    uint256 newUsers = receiptRedeemers() - receivedFeesFoUsers;
    receivedFeesFoUsers = receiptRedeemers(); 
    uint256 fee = (newUsers * rewardAmountInWeiOrTokenId * questFee) / 10_000;
    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, fee);
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L78-L87 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L89-L93 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L95-L98 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L215-L229 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L94-L118

Vulnerability details

Impact

Users has possibility to claim after owner called withdrawRemainingTokens. It leads to to the misunderstanding on the part of the participants to the organizers.

Proof of Concept

Let's imagine totalParticipants = 100, mintedTokens = 10, redeemed tokens = 4. Let's consider withdrawRemainingTokens(address to_) function from ERC20Quest contract. This function must be called only by the owner and after quest finished.

Next, owner called this function, and transfer remaining to the another address. On the quest's address remaining amount only for 6 participants. Now another one users calls mintReceipt function from QuestFactory contract => mintedTokens increased => now users may claim more than 6 tokens. but there is only money left for 6 people and no more.

Tools Used

Manual audit

Recommended Mitigation Steps

Add flag isRemainingWithdrawn to ERC20Quest. Set this flag when withdrawRemainingTokens(address to_) called. and during processing mintReceipt check this flag and revert if remaining withdrawn already happened.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L94-L118 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L106-L135

Vulnerability details

Impact

User might buy huge amount of tokens and executing claim() function will be cost a lot of gas and it will be too expensive and possible to get out of gas error. Or even the transaction may not fit in the block.

Proof of Concept

Let's consider function claim and Quest contract.

1. As user we have bought a lot of tokens for example 50_000.
2. Next we decided to claim them.
3. We loads them from rabbitHoleReceiptContract

uint[] memory tokens = rabbitHoleReceiptContract.getOwnedTokenIdsOfQuest(questId, msg.sender);

4. Then we calculates non-claimed tokens

        uint256 redeemableTokenCount = 0;
        for (uint i = 0; i < tokens.length; i++) {
            if (!isClaimed(tokens[i])) {
                redeemableTokenCount++;
            }
        }

In point 3 and point 4 we may iterates over unbounding amount of tokens. Which might lead to out of gas error.

Tools Used

Manual audit

Recommended Mitigation Steps

add threshold parameter to claim function, and to getOwnedTokenIdsOfQuest function.

If processed tokens reach threshold parameter interrupt iteration and process fetched tokens.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L106-L135 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L95-L104

Vulnerability details

Impact

If the user often participates in quests, buys tokens he accumulates tokens that remain on his account. This leads to huge transactions cost after a while.

Proof of Concept

When user want to claim rewards Quest contract calls getOwnedTokenIdsOfQuest which iterates over all tokens which user has. All iteration cost a gas, so after a while user will have a lot of outdated tokens, but the algorithm still will be iterates over them.

        for (uint i = 0; i < msgSenderBalance; i++) {
            uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
            if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
                tokenIdsForQuest[i] = tokenId;
                foundTokens++;
            }
        }

And this function calls from Quest's claim() transaction, therefore getOwnedTokenIdsOfQuest may use enormous gas amount.

Tools Used

Manual Audit

Recommended Mitigation Steps

In Quest contract when calling claim function, each claimed token must be burned.

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L13 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L44 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96-L118 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L95-L104 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L215-L229

Vulnerability details

Impact

Might be impossible to claim rewards by users. And admins must distribute tokens manually and pay fee for this. On a huge amount of participants this leads to huge amount of fees.

Proof of Concept

Let's consider QuestFactory. It has:

    RabbitHoleReceipt public rabbitholeReceiptContract;

Which responsible for mint tokens for users.

Then consider createQuest function. Here we pass rabbitholeReceiptContract into Quest.

In Quest this field is immutable.

Now lets consider next case:

1. We initialized whole contracts.
2. We created new Quest.
3. Next we decided to change rabbitholeReceiptContract in QuestFactory for another. To do this we call: setRabbitHoleReceiptContract. And successfully changing address.
4. Next we distribute signatures to our participants.
5. Users starts to mint tokens. But here is a bug QuestFactory storages new address of rabbitholeReceiptContract, but Quest initialized with older one. So users successfully minted their tokens, but can't exchange them for tokens because the Quest's receipt contract know nothing about minted tokens.

Possible solution here is change minterAddress in the original RabbitHoleReceipt contract and manually mint tokens by admin, but it will be too expensive and the company may lost a lot of money.

Tools Used

Manual audit

Recommended Mitigation Steps

In QuestFactory contract in the function mintReceipt the rabbitholeReceiptContract must be fetched from the quest directly. To Quest Add:

function getRabbitholeReceiptContract() public view returns(RabbitHoleReceipt) {
    return rabbitHoleReceiptContract;
}

Modify mintReceipt function in QuestFactory like:

function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
    ...
    RabbitHoleReceipt rabbitholeReceiptContract = Quest(quests[questId_].questAddress).getRabbitholeReceiptContract();
    rabbitholeReceiptContract.mint(msg.sender, questId_);
    ...
}

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L215-L229

Vulnerability details

Impact

Current implementation of token vesting not resistant to:

1. Deploy this factory on different chains (you've noticed you're going to be multi-chain).
2. Deploy this factory on the same chain with different address.

Proof of Concept

1. You deployed factory to the Etherium, and held a competition with name "makeTransfer".
2. Users claimed tokens on the Etherium chain and know valid signature.
3. You launched on the Polygon network, and held a competition with the same name "makeTransfer".
4. Users may don't complete tasks on the Polygon network and just resubmit already known signature with hash. -- Here is critical error because user may cheat.

The same thing can happen if you deploy factory to the same network but to a new address, because your signature does not depend on the factory address.

Tools Used

Manual audit

Recommended Mitigation Steps

Use spec: https://eips.ethereum.org/EIPS/eip-712

See how it's implemented in Uniswap V2: https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2ERC20.sol https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2ERC20.sol#L29-L37 https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2ERC20.sol#L81-L93

Do the same.

[N-1] Make 100% basis points as constant

Instead of 10_000 create constant anf use it.

---

[N-2] Add ability to claim for another address

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L96 claims to msg.sender, but address might be compromised, and therefore you might be want to send funds for another one address.

---

[N-3] ERC1155 doesn't support ERC20

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc1155Quest.sol#L11-L64 by EIP https://eips.ethereum.org/EIPS/eip-1155 ERC1155 supports ERC721 and ERC20, but current implementation support only ERC721. Think about rewriting code to single Quest with ERC1155.

---

[N-4] Make Quest abstract.

mark _transferRewards, _calculateRewards as abstract.

---

[N-5] Use _disableInitializers in QuestFactory

Use _disableInitializers instead of initializer modifier in constructor.

---

[N-6] Rename questId to questName

Current naming misleading, under Id implied uint256 or some integer field not string.

---

[N-7] Make grantDefaultAdminAndCreateQuestRole private

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L152

---

[N-8] Wrong comment

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L175-L177 Royalty from ERC2981 specification.

---

[N-9] Use camelCase for variables

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L35 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L36 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleTickets.sol#L25

[N-10] Use toHexString for address instead of for uint256

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L60 Rewrite to:

generateAttribute('Reward Address', Strings.toHexString(rewardAddress_))

[N-11] toHexString convert a checksummed address into a non-checksummed

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/ReceiptRenderer.sol#L60

You may use EIP-55: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-55.md#specification

---

[N-12] Typo remave to remove

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L176

---

[N-13] misleading name onlyAdminWithdrawAfterEnd

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L76

this modifier should be named as withdrawAfterEnd

---

[L-1] Bad QuestFactory architecture

Please, create two different functions like: createErc20Quest and createErc1155Quest, it improves readability, it removes comparing with contractType_. Because further when you add more types createQuest will grow to a huge size with a huge copy paste and in which it will be easy to make a mistake.

---

[L-2] Bad naming rewardAmountOrTokenId_

It's hard to understand, better split to two variables rewardsInWei, rewardTokenId. Then create in Quest mark getRewardAmount() as abstract. Then for Erc1155Quest create variable rewardTokenId. Then for Erc20Quest create rewardsInWei variable and use them instead of misleading rewardAmountInWeiOrTokenId.

---

[L-3] Not enough Logs

Please add events for all functions which change state. For example: setRabbitHoleReceiptContract, setProtocolFeeRecipient and so on.

---

[L-4] Add possibility to iterates over created quests in QuestFactory

---

[G-1] Refund gas optimization

While burning token here: https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L140 fields in mappings does not clearing https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L30 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L34 Recommendation: you may save up to 1/5 of transaction cost with gas refund if rewriting burn function with:

super._burn(tokenId_);
delete questIdForTokenId[tokenId_];
delete questIdForTokenId[timestampForTokenId];

---

[G-2] Redundant modifier

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L81 withdrawRemainingTokens from Quest.sol already has onlyOwner modifier. Recommendation: Remove onlyOwner modifier from ERC20Quest.sol's withdrawRemainingTokens function.

---

[G-3] Use Bitmap for claimedList

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Quest.sol#L24

mapping(uint256 => bool) private claimedList;

You're writing information about claim token in a single slot it cost 20_000 gas. You may Bitmap (like in UniswapV3 https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/TickBitmap.sol) and save up to 15_000 gas per token further. Recommendation: Rewrite to map like this

mapping(uint256 => uint256) private claimedList;

Add function position:

function position(uint256 tokenId) private pure returns (int16 wordPos, uint8 bitPos) {
    wordPos = int16(tokenId >> 8);
    bitPos = uint8(tokenId % 256);
}

Next add function to check is token claimed and function to claim token. Use https://github.com/Uniswap/v3-core/blob/main/contracts/libraries/TickBitmap.sol as a sample.

---

[G-4] Redundant call

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L43 With current implementation __AccessControl_init has empty body and doesn't have any side effects. Recommendation: Remove call:

__AccessControl_init();

---

[G-5] Redundant encoding

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L72 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L105 All samples above has strings which are encodes and hashes, but result from call to call will be the same. Recommendation: Make it constant and use futher

bytes32 private constant ERC11555_HASH = keccak256(abi.encodePacked('erc1155'));
bytes32 private constant ERC20_HASH = keccak256(abi.encodePacked('erc20')); 

---

[G-6] Redundant field

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L219 https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L222 Field hash_ is redundant because it already contains in signature by signing algorithm definition. Recommendation: Instead of comparing:

if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();

You may just create hash, pass it to recoverSigner function and compare with claimSignerAddress.

---

[G-7] Unused inheritance

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L17 Contract ERC721Upgradeable already contains in ERC721EnumerableUpgradeable and ERC721URIStorageUpgradeable. Recommendation: Remove definition of ERC721Upgradeable it saves gas during deploy and improves readability.

---

[G-8] Unused mapping

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

mapping(uint => string) public questIdForTokenId;

Recommendation: Remove this mapping and this map filling.

---

[G-9] Non optimal definition:

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L119 In line

if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_)))

You're comparing questIdForTokenId[tokenId]) with questId_ on each cycle and use keccak256 for questId_. You can make hashing before the loop and just compare hash result in loop. Recommendation: Rewrite like:

bytes32 hash = keccak256(bytes(questId_));

for (uint i = 0; i < msgSenderBalance; i++) {
    uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
    if (keccak256(bytes(questIdForTokenId[tokenId])) == hash) {
        tokenIdsForQuest[i] = tokenId;
        foundTokens++;
    }
}

---

[G-10] Non optimal memory use and cycle iteration

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/RabbitHoleReceipt.sol#L109-L135 In this function you iterate over whole sender's token twice. But you need two iterations: The first one over all user's tokens, the second one over filtered tokens only. Recommendation: Write into last free position of tokenIdsForQuest instead of actual position. Rewrite like this:

for (uint i = 0; i < msgSenderBalance; i++) {
    uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
    if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
        tokenIdsForQuest[foundTokens] = tokenId;
        foundTokens++;
    }
}

Then rewrite second loop to:

for (uint i = 0; i < foundTokens; i++) {
    filteredTokens[i] = tokenIdsForQuest[i];
}

---

[G-11] Loads struct into memory to save gas

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L215-L229

use next pattern:

Quest memory quest = quests[questID_];

Next read from this struct, but remember that you must write into storage

---

[G-12] Check endTime_, startTime_ on the QuestFactory side to save gas

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

Check that time is in range to save gas if checks failed