Protocol Admin can abuse fee withdrawal mechanism

[code423n4]

Lines of code

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

Vulnerability details

The protocol allows the admin to withdraw the protocolFee after a quest's end time which is calculated based on the number of redeemed receipts times by the reward amount where the questFee is applied to this (initially 20%).

The admin can call protocolFee() several times and this value increases as more users redeem their receipt for the reward, entitling the protocol to collect more fees for hosting the quest.

Impact

The modifier onlyAdminCanWithdraw does not have any functionality stopping the function withdrawFee() from being executed on chain multiple times. There is also no variable that keeps track of how much has been withdrawn in terms of protocol fees. This means the value returned by protocolFee() can be abused and allow for the multiple withdrawals of the protocol fee value if the quest contract's reward token balance is sufficient enough.

Proof of Concept

Provide direct links to all referenced code in GitHub.
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/Erc20Quest.sol#L89-L104

Potential example:

A quest which paid 100 USDC for completing an on-chain task.
Max total participants of 50
After the quest end time, only 25 receipts were redeemed for a receipt so far.
The contract balance is initially 6000 USDC. After receipt redeems, the balance is 3500.

If an admin called the withdrawFee() function in a transaction, protocolFee() would be calculated to equate to 500 USDC, transferring that amount to the protocolFeeRecipient. As there is no variable holding the amount of fees withdrawn, the admin could call this 6 more times draining the quest contract of any USDC as the reward token to pay the other 25 receipt holders the reward they are due.

Tools Used

Manual Code Review, Visual Studio

Recommended Mitigation Steps

• Create a global variable for ERC20 Quests that keep tracked of how much protocol fees have already been withdrawn which is updated in withdrawFee() just before safeTransfer.
• in withdrawFee(), the amount sent to the protocolFeeReceipt should be protocolFee() - 'alreadyWithdrawn'
• Apply this variable to all the calculations in the various functions that will be affected by this such as: withdrawFee(), withdrawRemainingTokens()

[c4-judge]

kirk-baird marked the issue as duplicate of #23

[c4-judge]

kirk-baird marked the issue as satisfactory