Owner can use recoverERC20 to drain reward tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/2cef3087052f019c8043f66f954d630b81cb16fb/contracts/WardenPledge.sol#L654

Vulnerability details

Impact

recoverERC20 does not allow recovering the token when minAmountRewardToken[token] != 0 such that the owner is not able to drain the reward tokens out of the contracts. However, the owner fully controls minAmountRewardToken[token] and can reset it to 0 with removeRewardToken, even if there is a remaining reward amount that is available for pledges.

This is a significant risk for users (especially pledge creators) that interact with the contract, as the reward tokens that they have transferred can be removed at any point.

Proof Of Concept

Consider the following diff where the owner removes the reward token and then calls recoverERC20 (which does not revert, i.e. the test fails there):

--- a/test/wardenPledge.test.ts
+++ b/test/wardenPledge.test.ts
@@ -3439,6 +3439,12 @@ describe('Warden Pledge contract tests', () => {
                 wardenPledge.connect(admin).recoverERC20(CRV.address)
             ).to.be.revertedWith('CannotRecoverToken')
 
+            await wardenPledge.connect(admin).removeRewardToken(CRV.address)
+
+            await expect(
+                wardenPledge.connect(admin).recoverERC20(CRV.address)
+            ).to.be.revertedWith('CannotRecoverToken')
+
         });
 
         it(' should block non-admin caller', async () => {

Recommended Mitigation Steps

Track for every token if there are remaining pledges. Do not allow recovering the tokens if this is the case.

[Kogaroshi]

Duplicate of #17

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as duplicate

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird marked the issue as duplicate of #17

[c4-judge]

Simon-Busch marked the issue as duplicate of #68