Paladin - Warden Pledges contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

When a pledge owner increases the reward per vote or the duration of the pledge, the contract locks up more of their reward tokens than necessary.

Proof of Concept

The total amount of reward tokens issued to users is specified in createPledge() as:

vars.totalRewardAmount = (rewardPerVote * vars.votesDifference * vars.duration) / UNIT;

When a pledge is updated, the total reward amount of recalculated:

// extendPledge()
uint256 totalRewardAmount = (pledgeParams.rewardPerVote * pledgeParams.votesDifference * addedDuration) / UNIT;

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

A pledge can be updated at any time between its creation and its endTimestamp as long as it's not closed. When recalculating the new totalRewardAmount, the contract doesn't check whether anybody has already pledged. The votesDifference value can be different between the creation and update of a pledge. And the contract only allows a pledge to gather as many votes as specified in their targetVotes property.

Consider the following situation:

Pledge creation:
- initialVotes = 0
- targetVotes = 100e18
- votesDifference = 100e18
- rewardPerVote = 2
- duration = 2 weeks (1209600)

$totalRewardAmount = 2 * 100e18 * 1209600 / 1e18$ $totalRewardAmount = 241920000$

1 week passed and50e18 votes were pledged. The creator decides to update the rewardPerVote parameter to 3 to get the remaining half of them. The contract would determine the totalRewardAmount the creator has to supply as:

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

$totalRewardAmount = 3 * 100e18 * 604800 / 1e18$ $totalRewardAmount = 181440000$

So the contract locks up another 181440000 tokens. But, because 50e18 votes were already pledged, only 50e18 more votes remain. Thus, the actual amount of tokens that are distributed as a reward are:

$totalRewardAmount = 3 * 50e18 * 604800 / 1e18$ $totalRewardAmount = 90720000$

The contract locks up more funds than necessary. Those tokens are not lost. After the pledge ends, the creator can reclaim them using retrievePledgeRewards(). It's only a temporary freezing of funds.

Tools Used

none

Recommended Mitigation Steps

When a pledge is updated, recalculate the votesDifference value and determine the totalRewardAmount using that new value.

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L333 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L394 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L438 https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L271

Vulnerability details

Impact

Some tokens charge a fee on transfers. The contract can't handle those tokens. It will think that it has more tokens than it actually does. That in turn will result in a pledge not being fulfillable. The most prominent token that might take fees in the future is USDT.

Proof of Concept

When the pledge creator provides the reward tokens, the contract doesn't verify that it received the correct amount

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

totalRewardAmount is the total amount of tokens that will be distributed to users who delegate their votes. If the contract doesn't actually have a balance of totalRewardAmount, the very last call to pledge() will fail because of the following transfer at the end of _pledge():

        IERC20(pledgeParams.rewardToken).safeTransfer(user, rewardAmount);

Because the contract doesn't have rewardAmount tokens left, the tx will fail. Because the last call to pledge fails, it will never be able to be fulfilled. Nobody will be able to delegate the last votes.

Tools Used

none

Recommended Mitigation Steps

Use the actual amount of tokens the contract received instead of the user provided value.

Gas Report

• G-01: _pledge() shouldn't verify user's balance
• G-02: trigger underflow instead of explicitly checking for it
• G-03: retrievePledgeRewards() and closePledge() can be merged to reduce deployment gas

G-01: _pledge() shouldn't verify user's balance

In the _pledge() function you check whether the user has enough boost delegation & set the correct allowance. The same checks are already done in BoostV2.boost(). You're wasting gas by repeating the same checks in your contract. If the user doesn't fulfill the necessary requirements, the call will simply fail.

• https://github.com/curvefi/curve-veBoost/blob/db3dec43b6e4fac0fca1f01509f9133563f43ebb/contracts/BoostV2.vy#L189
• https://github.com/curvefi/curve-veBoost/blob/db3dec43b6e4fac0fca1f01509f9133563f43ebb/contracts/BoostV2.vy#L223
• https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L241-L242

G-02: trigger underflow instead of explicitly checking for it

By adding an if-clause to prevent underflows you're increasing the gas costs of the happy path. Just let the value underflow and trigger a revert that way. Most wallets prevent txs that revert from being executed. Thus, you're wasting gas for no reason.

• https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L267

G-03: retrievePledgeRewards() and closePledge() can be merged to reduce deployment gas

Both functions implement the same thing. One is callable before the pledge ended and the other one after. The effect of both is the same (retrieval of funds). You can merge them into a single one to reduce deployment gas.

• https://github.com/code-423n4/2022-10-paladin/blob/main/contracts/WardenPledge.sol#L456-L515