MED: 4
MED: Fees charged from entire theoretical pledge amount instead of actual pledge amount
Description
Paladin receives a 5% cut from Boost purchases, as documented on the website
"Warden takes a 5% fee on Boost purchases, and 5% on Quest incentives. However, there are various pricing tiers for Quest creators. Contact the Paladin team for more info."
Here's how fee calculation looks at createPledge function:
vars.totalRewardAmount = (rewardPerVote * vars.votesDifference * vars.duration) / UNIT;
vars.feeAmount = (vars.totalRewardAmount * protocalFeeRatio) / MAX_PCT ;
if(vars.totalRewardAmount > maxTotalRewardAmount) revert Errors.IncorrectMaxTotalRewardAmount();
if(vars.feeAmount > maxFeeAmount) revert Errors.IncorrectMaxFeeAmount();
// Pull all the rewards in this contract
IERC20(rewardToken).safeTransferFrom(creator, address(this), vars.totalRewardAmount);
// And transfer the fees from the Pledge creator to the Chest contract
IERC20(rewardToken).safeTransferFrom(creator, chestAddress, vars.feeAmount);The issue is that the fee is taken up front, assuming totalRewardAmount will actually be rewarded by the pledge. In practice, the rewards actually utilized can be anywhere from zero to totalRewardAmount. Indeed, reward will only be totalRewardAmount if, in the entire period from pledge creation to pledge expiry, the desired targetVotes will be fulfilled, which is extremly unlikely.
As a result, if pledge expires with no pledgers, protocol will still take 5%. This behavior is both unfair and against the docs, as it's not "Paladin receives a 5% cut from Boost purchases".
Impact
Paladin fee collection assumes pledges will be matched immediately and fully, which is not realistic. Therefore far too much fees are collected at user's expense.
Proof of Concept
Bob creates a pledge, with target = 200, current balance = 100, rewardVotes = 10, remaining time = 1 week.
Protocol collects (200 - 100) * 10 * WEEK_SECONDS * 5% fees
A week passed, rewards were not attractive enough to bring pledgers.
After expiry, Bob calls retrievePledgeRewards() and gets 100 * 10 * WEEK_SECONDS back, but 5% of the fees still went to chestAddress.
Recommended Mitigation Steps
Fee collection should be done after the pledge completes, in one of the close functions or in a newly created pull function for owner to collect fees. Otherwise, it is a completely unfair system.
MED: Reuse of previous voting difference in extendPledge() charges too much fees
Description
In Warden Pledge, creators can extend the life span of an existing pledge using extendPledge.
Here's the implementation:
uint256 addedDuration = newEndTimestamp - oldEndTimestamp;
if(addedDuration < minDelegationTime) revert Errors.DurationTooShort();
uint256 totalRewardAmount = (pledgeParams.rewardPerVote * pledgeParams.votesDifference * addedDuration) / UNIT;
uint256 feeAmount = (totalRewardAmount * protocalFeeRatio) / MAX_PCT ;
if(totalRewardAmount > maxTotalRewardAmount) revert Errors.IncorrectMaxTotalRewardAmount();
if(feeAmount > maxFeeAmount) revert Errors.IncorrectMaxFeeAmount();The issue is that totalRewardAmount, and more importantly, the feeAmount, are generated using the previously calculated votesDifference. It was defined in createPledge as:
vars.votesDifference = targetVotes - votingEscrow.balanceOf(receiver);votesDifference was used correctly in createPledge because during creation time, theoretically the reward amount required to pay a fulfillement until the end depends on amount of votes to transfer, which is targetVotes - votingEscrow.balanceOf(receiver);
When extending the pledge, it is entirely reasonable that balanceOf(receiver) is much higher than before, and therefore the maximum votes that could be transferred while still under targetVotes is much lower. Since fee % is taken linearly from totalRewardAmount, it is also unrealistically high.
Impact
Protocol demands much more reward amount and fees than correct amount.
Proof of Concept
Bob creates a pledge, with target = 200, current veCRV balance = 100, rewardVotes = 10, remaining time = 1 week. voteDifference = 200 - 100 = 100.
Bob locked additional CRV, and now has 180 veCRV.
After 5 days, Bob wishes to extend pledge life span and calls extendPledge( + 1 week). Calculated reward is: uint256 totalRewardAmount = (pledgeParams.rewardPerVote * pledgeParams.votesDifference * addedDuration) / UNIT; Actual calculation: totalRewardAmount = 10 * 100 * (9 * 86400) / 1e18 Correct calculation: totalRewardAmount = 10 * 20 * (9 * 86400) / 1e18 The total is 5 X more than it should be. Pledgers cannot theoretically pledge to Bob 100 votes, because his 180 current balance will only lower very gradually. Fee amount will also be 5x higher than should be: uint256 feeAmount = (totalRewardAmount * protocalFeeRatio) / MAX_PCT ;
Recommended Mitigation Steps
voteDifference should be supplied as a parameter to the extendPledge / increasePledgeRewardPerVote APIs. Creator pays linearly to voteDifference, so it does not introduce any threates to give control to user of this parameter, which is effectively set in stone in createPledge().
MED: Owner can bypass reward token protection in recoverERC20 to instantly steal all tokens in contract
Description
WardenPledge contract has a sweeping function (recoverERC20) to handle mistakenly sent ERC20 tokens:
function recoverERC20(address token) external onlyOwner returns(bool) {
if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();
uint256 amount = IERC20(token).balanceOf(address(this));
if(amount == 0) revert Errors.NullValue();
IERC20(token).safeTransfer(owner(), amount);
return true;
} function recoverERC20(address token) external onlyOwner returns(bool) {
if(minAmountRewardToken[token] != 0) revert Errors.CannotRecoverToken();
uint256 amount = IERC20(token).balanceOf(address(this));
if(amount == 0) revert Errors.NullValue();
IERC20(token).safeTransfer(owner(), amount);
return true;
}The code will not allow registered reward tokens to be transfered due to the rugging potential. It checks if minAmountRewardToken[token] != 0, which is True for registered tokens.
However, owner can easily bypass this check using removeRewardToken function:
function removeRewardToken(address token) external onlyOwner {
if(token == address(0)) revert Errors.ZeroAddress();
if(minAmountRewardToken[token] == 0) revert Errors.NotAllowedToken();
minAmountRewardToken[token] = 0;
emit RemoveRewardToken(token);
}This function sets any minAmountRewardToken[token] that is not 0, to 0.
Therefore, owner can instantly call:
removeRewardToken(TokenX)
recoverERC20(TokenX) to steal all the funds.
It's very important to not allow owner such dangerous operation because:
It is not necessary for the functioning of the protocol
It hurts user's trust of the protocol
Compromised owner can instantly steal all funds
Malicious owner can instantly steal all funds.
Impact
Owner can steal all ERC20 tokens including rewards, of the contract.
Recommended Mitigation Steps
This threat can be fully mitigated using these steps:
add mapping(IERC20 => bool) public disabledTokens; state member
In recoverERC20, require disabledTokens[token] == False
In removeRewardToken, after settings minAmountRewardToken to 0, add: disabledTokens[token] == True
This solution will limit only reward tokens from being claimed by the owner, which is the desired behavior.
MED: Excessive owner privilege - can freeze pledge creator's funds after pledging period completed.
Description
The retrievePledgeRewards() function is used by pledge creator, only after pledge endTimestamp has passed. It will collect for the creator all unused reward tokens. Since it can only operate after endTimestamp, the pledge has for all intents and purposes finished, and no pledging API except retrievePledgeRewards can work.
There is therefore no justification for this function to be gated behind the whenNotPaused modifier. It creates a needless centralization risk of freeze of funds, when those funds belong to the creator at this stage.
Impact
Owner can freeze pledge creator's funds after pledging period completed.
Recommended Mitigation Steps
Remove the whenNotPaused modifier from retrievePledgeRewards
Note that this submission is similar in spirit to VTVL finding 475, in that owner is able to freeze / delete funds after maturity period.
Comentarios