Paladin - Warden Pledges contest - Lambda's results

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.

Lines of code

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

Vulnerability details

Impact

When calculating the amount of new reward tokens that should be transferred, extendPledge and increasePledgeRewardPerVote use the variable pledgeParams.votesDifference to calculate the added rewards:

uint256 totalRewardAmount = (rewardPerVoteDiff * pledgeParams.votesDifference * remainingDuration) / UNIT;

However, this variable is set within createPledge and refers to the initial difference (when the pledge was created):

vars.votesDifference = targetVotes - votingEscrow.balanceOf(receiver);

When extendPledge or increasePledgeRewardPerVote is called, the balance might be significantly smaller, but the totalRewardAmount is still calculated with the original one. This has two problems: 1.) Capital efficiency: The user has to provide too much reward tokens, making the system less attractive. 2.) Fees: The creator has to pay fees on the whole added reward amount, even when only a small amount of it will actually be distributed, leading to a loss of funds for the creator.

Proof Of Concept

Let's say the initial difference is $x$ at time $t$. At time $t + 86400$, when the creator calls extendPledge, the difference has decreased to $0.1x$ because a lot of user pledged. However, the additional reward amount that the creator has to transfer is still calculated with $x$ and he has to pay fees based on this value.

Recommended Mitigation

Recalculate the difference and use this value for the added reward amount (and the fees).

Lines of code

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

Vulnerability details

Impact

In createPledge, pledgeAvailableRewardAmounts[vars.newPledgeID] is increased by vars.totalRewardAmount and this amount is transferred from the creator:

IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);

In retrievePledgeRewards (and closePledge), is retrieved and transferred to the receiver:

uint256 remainingAmount = pledgeAvailableRewardAmounts[pledgeId];
...
IERC20(pledgeParams.rewardToken).safeTransfer(receiver, remainingAmount);

This logic becomes very problematic for fee-on-transfer tokens. There, less than vars.totalRewardAmount will be transferred to the contract. Therefore, retrieving the pledge rewards (or potentially also a call to _pledge, but the consequences are not that severe there) will fail because the contract does not contain this amount of tokens.

Proof Of Concept

A very simple example is the following: Someone creates a pledge with a fee-on-transfer token that takes a 1% fee on every transfer. The totalRewardAmount is 1 million tokens, but only 999'000 arrive at the contract because of the fee. When the creator immediately calls closePledge afterwards, the contract tries to transfer 1 million tokens to him (the value of pledgeAvailableRewardAmounts[pledgeId]), but this will fail, because the contract balance is smaller than that. The creator can therefore not retrieve these tokens and they are lost.

Recommended Mitigation Steps

Query and store the actual amount of tokens that was transferred, i.e. the difference between the balances.