BLACK-PANDA-REACH - Balanced Vault Upgrade can trigger a mass liquidation event

[sherlock-admin]

BLACK-PANDA-REACH

medium

Balanced Vault Upgrade can trigger a mass liquidation event

Summary

maxCollateral or targetLeverage can be changed to lower values than that of previous values during upgrade, which will trigger liquidation events when new targetLeverage < old targetLeverage OR new maxCollateral < old maxCollateral

Vulnerability Detail

https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVaultDefinition.sol#L86-L88

targetLeverage = targetLeverage_;

maxCollateral = maxCollateral_;

When upgrading vault from previous implementation to new one, it should have previousImplementation_.maxCollateral <= maxCollateral_ and previousImplementation_.targetLeverage <= targetLeverage_.

Although, there is no check like this while upgrading, so new contract could have smaller values than what previous implementation had. As a result, it will make all the already opened positions which was leveraging previousImplementation_.targetLeverage, to get liquidated as after upgrade : new targetLeverage < prevImpl.targetLeverage.

Reducing the maxCollateral parameter means that the upgraded vault will have a lower limit on the total amount of collateral it can hold. If the existing positions in the vault exceed this new limit, it can result in insufficient collateral capacity to support those positions. This can lead to liquidation events causing financial losses for the vault users.

As can be seen below, in constructor, if long and short from the previous markets are checked, we suggest to check maxCollateral and targetLeverage also while upgrading.

https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVaultDefinition.sol#L127-L132

uint256 previousTotalMarkets_ = previousImplementation_.totalMarkets();
if (previousTotalMarkets_ > totalMarkets_) revert BalancedVaultDefinitionMarketsMismatchedWithPreviousImplementationError();
for (uint256 marketId; marketId < previousTotalMarkets_; marketId++) {
    MarketDefinition memory previousMarket_ = previousImplementation_.markets(marketId);
    if (previousMarket_.long != marketDefinitions_[marketId].long) revert BalancedVaultDefinitionMarketsMismatchedWithPreviousImplementationError();
    if (previousMarket_.short != marketDefinitions_[marketId].short) revert BalancedVaultDefinitionMarketsMismatchedWithPreviousImplementationError();
}

Impact

Upgrade can trigger a mass liquidation event if upgraded with a lower maxCollateral or targetLeverage. Thus causing financial losses for the vault users.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVaultDefinition.sol#L86-L88

Tool used

Manual Review

Recommendation

We recommend to have a check while upgrading in constructor to make sure new maxCollateral or targetLeverage is not lower than previous one.

[KenzoAgada]

While such a sanity check would be nice, the previousImplentation functionality and checks are optional. Admin is anyway generally trusted with parameters. Considering as a valid low.

[arjun-io]

Changing these parameters won't result in an instant liquidation - the vault should correctly handle deleveraging and reducing collateral without risking liquidation. If this is not the case, we would consider this a valid bug but there are no details in this report which show why a liquidation would happen