Quest owner can withdraw funds before the quest has ended

[code423n4]

Lines of code

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

Vulnerability details

Impact

The withdrawRemainingTokens() function is supposed to allow the quest owner to withdraw any remaining funds after the quest has ended. But, the function is callable at any time. The owner is able to withdraw the tokens before any user has minted a receipt or claimed their reward. Since these rewards are used as a form of payment for finishing a quest, this results in a loss of funds for users.

Proof of Concept

The withdrawRemainingTokens() function implements no check to prevent the owner from calling it before the endTime was reached:

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

1. Alice creates a quest
2. Alice funds the quest with 100,000 tokens and starts it
3. Alice calls withdrawRemainingTokens() right after starting it to withdraw 80,000 tokens (20,000 are reserved for the protocol and thus not accessible)

Users will be able to claim their receipts until the remaining 20,000 tokens are distributed. That means that only 1/4 of the users are able to withdraw their rewards. Additionally, the protocol won't be able to claim their fees either since there are no funds left in the contract.

Tools Used

none

Recommended Mitigation Steps

Add the following check to withdrawRemainingRewards():

require(block.timestamp > endTime);

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[kirk-baird]

This issue is invalid. The check exists in super.withdrawRemainingTokens() here which has the modifier onlyAdminWithdrawAfterEnd

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid