RabbitHole Quest Protocol contest - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

The quest participants can mint receipts of a certain quest once they completed on-chain tasks. Also the quest creators put the endTime of the quest in the quest creation.

The quest creator uses the endTime because they are allow to pay rewards for those on-chain completed tasks for a certain period of time. If the Rabbithole protocol allows participants to mint receipts even after the Quest.endTime then the quest creator will lost rewards tokens they don't wanted to lost.

The quest creator can withdraw the remaining tokens after the quest ends but if the protocol allows mint receipts even if the quest has ended then the quest creator will withdraw less tokens. The quest creator uses an endTime parameter because it is the range time they want to pay rewards for completed on-chain tasks, so mint receipts after the endTime will affect the quest creators rewards.

There is an off-chain protection because the participant needs a valid sig hash in order to mint the receipt but there should be a on-chain validation in order to protect the quest creators rewards.

Proof of Concept

As you can see in the mintReceipt function, the participant can mint receipts even if the quest has ended.

File: QuestFactory.sol
219:     function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
220:         if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint();
221:         if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
222:         if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
223:         if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();
224: 
225:         quests[questId_].addressMinted[msg.sender] = true;
226:         quests[questId_].numberMinted++;
227:         emit ReceiptMinted(msg.sender, questId_);
228:         rabbitholeReceiptContract.mint(msg.sender, questId_);
229:     }

Tools used

VSCode

Recommended Mitigation Steps

Add a validation in the mintReceipt() function where it is only possible to mint a receipt if the block.timestamp is in the Quest.startTime/Quest.endTime range.

Lines of code

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

Vulnerability details

Impact

The RabbitHole participant can have many receipts from all the quest he has participated. The RabbitHoleReceipt.sol::getOwnedTokenIdsOfQuest() function helps to get the receipts which are owned by the user per questId.

If the participant accumulate a lot of RabbitHoleReceipts the for statement which iterates through all receipts from the participant can run out of gas.

The Quest.sol::claim() function will be reverted for users who have many receipts causing the rewards may be trapped for the participants and the quest creator because the ERC20Quest.sol::withdrawRemainingTokens() is only able to withdraw the non claimable tokens.

Proof of Concept

The RabbitHoleReceipt.sol::getOwnedTokenIdsOfQuest() function calculates the user balance in the line 113. Then the user balance is used in the for statement in the line 117.

The participant balance could be a large amount of receipts because the user can participate in many quests then the for statement could be reverted by insufficient gas.

File: RabbitHoleReceipt.sol
109:     function getOwnedTokenIdsOfQuest(
110:         string memory questId_,
111:         address claimingAddress_
112:     ) public view returns (uint[] memory) {
113:         uint msgSenderBalance = balanceOf(claimingAddress_);
114:         uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
115:         uint foundTokens = 0;
116: 
117:         for (uint i = 0; i < msgSenderBalance; i++) {
118:             uint tokenId = tokenOfOwnerByIndex(claimingAddress_, i);
119:             if (keccak256(bytes(questIdForTokenId[tokenId])) == keccak256(bytes(questId_))) {
120:                 tokenIdsForQuest[i] = tokenId;
121:                 foundTokens++;
122:             }
123:         }

I created a basic test where you can see the receipt is not burned after the rewards claim, then the participant is accumulating many receipts:

it('the receipt is not burned after the reward was claimed', async () => {
    // Receipts are not burned after the rewards claim()
    // 1. Mint a new Receipt for the firstAddress
    await deployedRabbitholeReceiptContract.mint(firstAddress.address, questId)
    // 2. Check the firstAddress receipt balance
    expect(await deployedRabbitholeReceiptContract.balanceOf(firstAddress.address)).to.equal(1)
    await deployedQuestContract.start()
    await ethers.provider.send('evm_increaseTime', [86400])
    // 3. Claim the rewards as the firstAddress
    await deployedQuestContract.connect(firstAddress).claim()
    // 4. After the rewards was claimed, the receipt is not burned then the user is accumulating many receipts
    expect(await deployedRabbitholeReceiptContract.balanceOf(firstAddress.address)).to.equal(1)
})

Tools used

VSCode

Recommended Mitigation Steps

If the rewards was claimed then burn the participant receipt.