Paladin - Warden Pledges contest - Picodes's results

Lines of code

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L653

Vulnerability details

Impact

In recoverERC20, the code is designed to make it looks like the owner cannot withdraw pledge rewards from the contract, although it is doable.

Proof of Concept

To bypass the requirement minAmountRewardToken[token] == 0 the owner can just call removeRewardToken and then call recoverERC20.

Recommended Mitigation Steps

Out of clarity for users, either:

• clearly indicate that the owner can withdraw funds from the contract
• or store the amounts of each token that cannot be recovered by the owner to really enforce that the owner cannot withdraw pledge rewards

Lines of code

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

Vulnerability details

Impact

votingEscrow.balance decreases over time, so votesDifference is not accurate anymore when a pledge is extended or increased. This does not lead to criticial errors but may prevent a pledge to reach its targetVotes.

Proof of Concept

Pedge.votesDifference is initialized with: vars.votesDifference = targetVotes - votingEscrow.balanceOf(receiver);

But the votingEscrow.balance decreases over time, so the votesDifference is not accurate when extendPledge or increasePledgeRewardPerVote is called, leading to fewer tokens than necessary to reach targetVotes being transfered;

Recommended Mitigation Steps

Do not store votesDifference but fetch it again in extendPledge and increasePledgeRewardPerVote;

[NC-1] Incorrect formatting:

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L28 struct Pledge{ -> struct Pledge {

[NC-2] Redundant info

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L71

(in BPS) and // bps are the same info. No need to write it twice.

[NC-3] Invalid comments

All comments related to events are incorrect (Event emitted when xx)

Example here

[NC-4] Typo

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L568

minmum -> minimum

[NC-5] Typo

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L650

tof -> of

[NC-6] Typo

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L365

ot -> to

[NC-7] Typo

https://github.com/code-423n4/2022-10-paladin/blob/d6d0c0e57ad80f15e9691086c9c7270d4ccfe0e6/contracts/WardenPledge.sol#L292

Maximum taget of votes to have (own balacne + delegation) for the receiver -> Maximum target of votes to have (own balance + delegation) for the receiver

[GAS-1] Pledge variables could be batched

In Pledge, targetVotes and rewardPerVote could safely be stored in uint128 to batch them and save a SLOAD in _pledge, extendPledge and increasePledgeRewardPerVote. Considering 18 decimals token there is no risk of overflow.

[GAS-2] It can be cheaper to avoid loading pledgeParams in memory

In _pledge, pledgeParams is loaded in memory but votesDifference is not used, hence is loaded for nothing and a SLOAD could be saved.