Amount of tokens returned (nonClaimableTokens) in withdrawRemainingTokens() is incorrect if withdrawFee() has been called

[code423n4]

Lines of code

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

Vulnerability details

Impact

If withdrawFee() has been called, then the value of nonClaimableTokens in withdrawRemainingTokens() is incorrect. When this happens, the quest owner will get less tokens then they are supposed to when they call withdrawRemainingTokens()

Proof of Concept

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L85
Consider this line of code. Let the current balance = b.

If withdrawFee() is called immediately before withdrawRemainingTokens(), the caller (quest owner) will get b - protocolFee() - protocolFee() - unclaimedTokens tokens back (since balance after withdrawFee is b - protocolFee())

If withdrawRemainingTokens() is not preceded by withdrawFee(), the caller (quest owner) will get b - protocolFee() - unclaimedTokens tokens back

Tools Used

VSCode

Recommended Mitigation Steps

Maintain a counter initialized to 0 (say protocolFeeCount). Set it to receiptRedeemers() at the end of withdrawFee().

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L97
Then, in protocolFee(), this line should be changed to return ((receiptRedeemers() - protocolFeeCount)* rewardAmountInWeiOrTokenId * questFee) / 10_000;

[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)