Funds will get stuck forever in the Erc20Quest contract if withdrawFee() is called before withdrawRemainingTokens()

[code423n4]

Lines of code

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

Vulnerability details

Impact

withdrawFee() is used for sending the protocol fees to protocolFeeRecipient. And withdrawRemainingTokens() is used for transferring nonClaimableTokens back to the Quest admin. In the withdrawRemainingTokens function, when calculating the amount of tokens to withdraw, the protocolFee() is deducted from it. But if the withdrawFee was already called there is no need to deduct it. Thus Quest admin loses tokens equal to protocolFee.

Proof of Concept

The unfortunate event happens as such:

1. Quest has ended. Users and Admins are happy.
2. Someone calls the withdrawFee() function and the protocol fees are sent to the address set by the factory contract. So the balance of the contract is less now.
3. The Quest admin wakes up late and tries to withdraw the tokens which are not supposed to be claimed by anyone. The exact amount of tokens sent to him are calculated as follows:

uint256 nonClaimableTokens = IERC20(rewardToken).balanceOf(address(this)) - protocolFee() - unclaimedTokens;

4. Here the formula deducts the protocolFee() once again, which is already deducted from the contract balance.
5. Which means the Quest admin will loose tokens worth of protocolFee.

Tools Used

VS code, Manual analysis

Recommended Mitigation Steps

I suggest, once the protocol fees are withdrawn, it should be marked as such with a state variable. And then use this state variable to decide whether the protocolFee should be deducted from the contract balance.

[c4-judge]

kirk-baird marked the issue as duplicate of #42

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as duplicate of #61

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird changed the severity to 2 (Med Risk)