Paladin - Warden Pledges contest - Trust's results

Lines of code

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

Vulnerability details

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;
    }

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.

Tools Used

Manual audit

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.

Lines of code

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

Vulnerability details

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

1. Bob creates a pledge, with target = 200, current veCRV balance = 100, rewardVotes = 10, remaining time = 1 week. voteDifference = 200 - 100 = 100.
2. Bob locked additional CRV, and now has 180 veCRV.
3. 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 ;

Tools Used

Manual audit

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().

Lines of code

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

Vulnerability details

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

1. Bob creates a pledge, with target = 200, current balance = 100, rewardVotes = 10, remaining time = 1 week.
2. Protocol collects (200 - 100) * 10 * WEEK_SECONDS * 5% fees
3. A week passed, rewards were not attractive enough to bring pledgers.
4. After expiry, Bob calls retrievePledgeRewards() and gets 100 * 10 * WEEK_SECONDS back, but 5% of the fees still went to chestAddress.

Tools Used

Manual audit

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.

Lines of code

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

Vulnerability details

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.

Tools Used

Manual audit

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.

Issues

1. Missing event in retrievePledgeRewards

retrievePledgeRewards can only be called on expired rewards. The issue is that if there are no remaining rewards, no event is emitted even though an important state has changed - the pledge is closed. Fix by emitting a ClosePledge similarly to closePledge function.

// Set the Pledge as Closed
if(!pledgeParams.closed) pledgeParams.closed = true;
if(remainingAmount > 0) {
    // Transfer the non used rewards and reset storage
    pledgeAvailableRewardAmounts[pledgeId] = 0;
    IERC20(pledgeParams.rewardToken).safeTransfer(receiver, remainingAmount);
    emit RetrievedPledgeRewards(pledgeId, receiver, remainingAmount);
}

2. pledgePercent converts % to amount using veCRV balance, but _pledge checks amount is above "delegable_balance"

pledgePercent contains this code:

uint256 amount = (votingEscrow.balanceOf(msg.sender) * percent) / MAX_PCT;
_pledge(pledgeId, msg.sender, amount, endTimestamp);

While _pledge has this code:

if(delegationBoost.delegable_balance(user) < amount) revert Errors.CannotDelegate();

In the exported function the % is amount of actual veCRV balance. But _pledge checks the amount is over delegable_balance. This is veBalance + received delegations - sent delegations.

This creates confusion for the user:

1. If their delegable_balance is lower than veCRV balance, the pledge will not succeed.
2. If user intends the % to be from delegable balance, and delegable balance is lower than than veCRV balance, the amount calculated will be higher than user's expectation.