Incase a malicious attack occurs and the quest is paused, the owner won't be able to withdraw some of his tokens back.

[code423n4]

Lines of code

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

Vulnerability details

Impact

Incase a malicious attack occurs and the quest is paused, the owner won't be able to withdraw some of his tokens back, as a result the tokens will be stuck in the quest contract.

Proof of Concept

By asking the sponsor - a quest can be in paused state as an emergency situation (malicious attack, some issue on their
off-chain signature etc). When a quest is paused by the owner, the function claim is uncallable and reverts, duo to that users are unable to claim their receipts and receive rewards.

Lets say a malicious attack occurs and the owner successfuly pauses the quest contract, so the attackers can't claim the receipts.
Basically this stops the malicious attack, but doesn't fix the main problem as at the end of the quest the owner won't be able to withdraw the remaining tokens for the minted receipts the attackers posses. Yes the attackers won't be able to claim their receipts as the quest contract is paused, but neither the owner will be able to withdraw the tokens back, as a result the tokens will be stuck in the contract.

The owner should have a way to withdraw the tokens allocated for the unclaimed receipts incase of malicious attack.

contracts/Erc20Quest.sol

79: /// @dev Every receipt minted should still be able to claim rewards (and cannot be withdrawn).

81:  function withdrawRemainingTokens(address to_) public override onlyOwner {
82:        super.withdrawRemainingTokens(to_);
83:
84:        uint unclaimedTokens = (receiptRedeemers() - redeemedTokens) * rewardAmountInWeiOrTokenId;
85:        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;
86:        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);
87:    }

Tools Used

Manual review.

Recommended Mitigation Steps

Consider applying a check in the function withdrawRemainingTokens, which incase the contract is paused.
The owner will be able to withdraw all of the remaining tokens in the contract:

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

        if (isPaused) {
        uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee();
        IERC20(rewardToken).safeTransfer(to_, nonClaimableTokens);

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

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[CodingNameKiki]

Hey @kirk-baird, can you take a look at this again.

This isn't a duplicate of #42, the issue here describes how in an emergency situation (when the quest is paused) duo to a malicious attack, can lead to loss of funds as the owner won't be able to withdraw some of his tokens back.

[kirk-baird]

Agreeing with @CodingNameKiki that this is not a duplicate of #42 as it is related to pausing and users not being able to call claim() if it is paused.

Pausing issues are covered by the centralisation risk issue in the public report and so I'm going to downgrade this to QA.

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-b

[c4-judge]

kirk-baird marked the issue as grade-a