RabbitHole Quest Protocol contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

When users claim from any Quest contract, it will call to function getOwnedTokenIdsOfQuest() of RabbitHoleReceipt contract to get all token ids for that quest owned by caller. Function getOwnedTokenIdsOfQuest() will loop through all receipt owned by caller and filter token for that quest

function getOwnedTokenIdsOfQuest(
    string memory questId_,
    address claimingAddress_
) public view returns (uint[] memory) {
    uint msgSenderBalance = balanceOf(claimingAddress_);
    uint[] memory tokenIdsForQuest = new uint[](msgSenderBalance);
    uint foundTokens = 0;

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

    uint[] memory filteredTokens = new uint[](foundTokens);
    uint filterTokensIndexTracker = 0;

    for (uint i = 0; i < msgSenderBalance; i++) {
        if (tokenIdsForQuest[i] > 0) {
            filteredTokens[filterTokensIndexTracker] = tokenIdsForQuest[i];
            filterTokensIndexTracker++;
        }
    }
    return filteredTokens;
}

For each quest, normally each user can only mint 1 receipt, therefore only have 1 token per quest, but users can purchase more receipt in open market and have more than 2 tokens per quest. In addition, RabbitHoleReceipt contract hold receipts for all quests.

So if attacker somehow can create a quest where value of each receipt is neglectible or zero, he can transferred all these spam receipts to victim, making the claim call of victim more expensive in term of gas cost. Technically, it is even possible to DOS victim claim when the gas cost is break the block gas limit.

Proof of Concept

Function getOwnedTokenIdsOfQuest() is used in Quest.claim() function

function claim() public virtual onlyQuestActive {
    if (isPaused) revert QuestPaused();

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

    if (tokens.length == 0) revert NoTokensToClaim();
    ...
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using a data structure that can query all token for a quest quickly. For example, a mapping of owner to questId to array.

Lines of code

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

Vulnerability details

Impact

There are 2 type of Quest, Erc20Quest and Erc1155Quest. Both contracts have common property that they should hold enough reward token for totalParticipants. However, if number of receipt minted is less than totalParticipants, owner is allowed to withdraw remaining token to an address he specified. This check is implemented correctly in Erc20Quest where owner can only withdraw nonClaimableTokens.

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);

    uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
    uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
    IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
}

However, it did not have the same check in Erc1155Quest.

function withdrawRemainingTokens(address to_) public override onlyOwner { 
  // @audit owner should not be able to withdraw all tokens before users claimed
    super.withdrawRemainingTokens(to_);
    IERC1155(rewardToken).safeTransferFrom(
        address(this),
        to_,
        rewardAmountInWeiOrTokenId,
        IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
        '0x00'
    );
}

As the result, all tokens in Erc1155Quest can be withdraw before any user claim their receipts.

Proof of Concept

Please add this test to Erc1155Quest.spec.ts

it.only('revert claiming when owner withdraw all tokens', async () => {
  await deployedRabbitholeReceiptContract.mint(owner.address, questId)
  await deployedQuestContract.start()

  await ethers.provider.send('evm_increaseTime', [86400])

  expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(0)

  const totalTokens = await deployedRabbitholeReceiptContract.getOwnedTokenIdsOfQuest(questId, owner.address)
  expect(totalTokens.length).to.equal(1)

  expect(await deployedQuestContract.isClaimed(1)).to.equal(false)
  // owner withdraw
  await deployedQuestContract.connect(owner).withdrawRemainingTokens(owner.address)

  // will revert here
  await deployedQuestContract.claim()
  expect(await deployedSampleErc1155Contract.balanceOf(owner.address, rewardAmount)).to.equal(1)
  await ethers.provider.send('evm_increaseTime', [-86400])
})

Tools Used

Manual Review

Recommended Mitigation Steps

Consider only allowing owner to withdraw non claimable tokens in Erc1155Quest

Lines of code

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

Vulnerability details

Impact

In QuestFactory contract, users need to provide a signature signed by claimSignerAddress to mint a RabbitHole receipt. It has been seen that signed data is only msg.sender and questId_

function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
    if (quests[questId_].numberMinted + 1 > quests[questId_].totalParticipants) revert OverMaxAllowedToMint();
    if (quests[questId_].addressMinted[msg.sender] == true) revert AddressAlreadyMinted();
    if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
    if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned(); 
  // @audit signature replay on other chain

    quests[questId_].addressMinted[msg.sender] = true;
    quests[questId_].numberMinted++;
    emit ReceiptMinted(msg.sender, questId_);
    rabbitholeReceiptContract.mint(msg.sender, questId_);
}

Because of lacking the domain of the contract that is going to consume the signature (chainId, contract address, version,...), the signature can be reused for another verification on another contract or different chain.

• Protocol deployed on more than 1 chain (e.g Ethereum and Polygon)
• Deploy new QuestFactory contract and signature can be used in both old and new version
• Hash value of completely different protocol that combine of (address, string) can also be used in QuestFactory

Proof of Concept

In mintReceipt() function, hash_ is only has msg.sender and questId_ as can be seen on the code snippet on Impact section.

In recoverSigner() function, there is no additional info about domain is added

function recoverSigner(bytes32 hash_, bytes memory signature_) public pure returns (address) {
    bytes32 messageDigest = keccak256(abi.encodePacked('\x19Ethereum Signed Message:\n32', hash_));
    return ECDSAUpgradeable.recover(messageDigest, signature_);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding domain of contract to prevent signature being used on different chain/contract