top of page

C4 Audit Report - Paladin

I've competed in this contest between 27/10/22-30/10/22 and achieved third place. Repo is here.


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:

  1. removeRewardToken(TokenX)

  2. recoverERC20(TokenX) to steal all the funds.

It's very important to not allow owner such dangerous operation because:

  1. It is not necessary for the functioning of the protocol

  2. It hurts user's trust of the protocol

  3. Compromised owner can instantly steal all funds

  4. 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:

  1. add mapping(IERC20 => bool) public disabledTokens; state member

  2. In recoverERC20, require disabledTokens[token] == False

  3. 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.

 


0 comments

Recent Posts

See All

Comments


bottom of page