withdrawRemainingTokens could leave tokens in the contract

[code423n4]

Lines of code

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

Vulnerability details

Impact

The withdrawRemainingTokens function leaves tokens (for an amount equal to protocolFee) in the contract when withdraFee is called earlier due to non tracking if withdraFee was already called.

Proof of Concept

Assume balance = 150, unclaimedTokens = 0, protocolFee = 50. withdrawFee is called and balance becomes 100. When calling withdrawRemainingTokens nonClaimableTokens will be 100 - 50 = 50. 50 tokens are lost in the contract.

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

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

Recommended Mitigation Steps

Keep track if withdrawFee is called and if yes don't subtracts protocolFee when computing nonClaimableTokens.

[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 3 (High Risk)

[c4-judge]

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