Admin can drain ERC20Quest by calling withdrawFee multiple times

[code423n4]

Lines of code

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

Vulnerability details

Impact

Admin can drain all token in ERC20Quest by calling withdrawFee multiple times and receive more token than it is entitled to.

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

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

Proof of Concept

diff --git a/test/Erc20Quest.spec.ts b/test/Erc20Quest.spec.ts
index d0626ee..729544a 100644
--- a/test/Erc20Quest.spec.ts
+++ b/test/Erc20Quest.spec.ts
@@ -363,6 +363,8 @@ describe('Erc20Quest', async () => {
 
       await ethers.provider.send('evm_increaseTime', [100001])
       await deployedQuestContract.withdrawFee()
+      // Admin steal more fee by calling withdrawFee again
+      await deployedQuestContract.withdrawFee()
 
       expect(await deployedSampleErc20Contract.balanceOf(deployedQuestContract.address)).to.equal(
         totalRewardsPlusFee * 100 - 1 * 1000 - 200

The test will fail because the quest contract have less than expected balance (since fee is drawn twice)

Recommended Mitigation Steps

Only allow the admin to call withdrawFee once.

[c4-judge]

kirk-baird marked the issue as duplicate of #23

[c4-judge]

kirk-baird marked the issue as satisfactory