BLACK-PANDA-REACH - BalancedVault doesn't consider potential break in one of the markets

[sherlock-admin]

BLACK-PANDA-REACH

medium

BalancedVault doesn't consider potential break in one of the markets

Summary

In case of critical failure of any of the underlying markets, making it permanently impossible to close position and withdraw collateral all funds deposited to balanced Vault will be lost, including funds deposited to other markets.

Vulnerability Detail

As Markets and Vaults on Perennial are intented to be created in a permissionless manner and integrate with external price feeds, it cannot be ruled out that any Market will enter a state of catastrophic failure at a point in the future (i.e. oracle used stops functioning and Market admin keys are compromised, so it cannot be changed), resulting in permanent inability to process closing positions and withdrawing collateral.

BalancedVault does not consider this case, exposing all funds deposited to a multi-market Vault to an increased risk, as it is not implementing a possibility for users to withdraw deposited funds through a partial emergency withdrawal from other markets, even at a price of losing the claim to locked funds in case it becomes available in the future. This risk is not mentioned in the documentation.

Proof of Concept

Consider a Vault with 2 markets: ETH/USD and ARB/USD.

1. Alice deposits to Vault, her funds are split between 2 markets
2. ARB/USD market undergoes a fatal failure resulting in maxAmount returned from _maxRedeemAtEpoch to be 0
3. Alice cannot start withdrawal process as this line in redeem reverts:

    if (shares.gt(_maxRedeemAtEpoch(context, accountContext, account))) revert BalancedVaultRedemptionLimitExceeded();

Impact

Users funds are exposed to increased risk compared to depositing to each market individually and in case of failure of any of the markets all funds are lost. User has no possibility to consciously cut losses and withdraw funds from Markets other than the failed one.

Code Snippet

_maxRedeemAtEpoch
https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L649-L677

check in redeem
https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L188

Tool used

Manual Review

Recommendation

Implement a partial/emergency withdrawal or acknowledge the risk clearly in Vault's documentation.

[KenzoAgada]

Please note that the duplicate issues referenced above mention paused markets and oracle fails as additional scenarios where markets can break and therefore break the vault. Issue #20 contains recommendations on how to handle stuck epochs and oracles.

[securitygrid]

Escalate for 10 USDC
This is not valid M.
The protocol is designed that way. Any protocol that requires external oracle price feeds will experience oracle misbehavior. This is inevitable. But this is only temporary and occasional. This is acceptable.

[sherlock-admin]

Escalate for 10 USDC
This is not valid M.
The protocol is designed that way. Any protocol that requires external oracle price feeds will experience oracle misbehavior. This is inevitable. But this is only temporary and occasional. This is acceptable.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[KenzoAgada]

• Note that I grouped together with this issue all submissions which speak about increased danger of vaults due to breaking markets - either by oracle failures or paused products.
• The issues say that a vault with n markets is exposed to 2n possible problem in products.
• If even only one of the products is paused, or experiences oracle issues, the vault would jam.
• There are no contingency measures in such a scenario.
• I agree with the escalation that "the protocol is designed that way". But I'm not sure that users are aware that eg. a pause in one of the products would render the whole vault inoperable.
• Therefore I think it is debatable, and understand if the sponsor won't fix, but I would maintain medium severity due to the increased risk and lack of contingency measures.

edit: additionally, a watson mentioned in the chat that in the contest readme there is the following section:

Q: In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.
A: We want to be aware of issues that might arise from Chainlink or DSU integrations

I think that this can be legitimately said to be such an issue, and AFAIK it was not mentioned specifically anywhere that a vault would break if one of 2n products breaks.

[jacksanford1]

@securitygrid I think what the submitter is saying is that a market could break for an unknown reason (could be oracle-related or not) and if the vault deposits into 10 markets and 1 market breaks in a irrecoverable way, then the funds in the other 9 markets cannot be retrieved by the depositor.

If that's the case, I'd argue this is a Medium vulnerability.

[jacksanford1]

Result:
Medium
Has duplicates
Keeping as Medium due to reasoning in previous comment.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• securitygrid: rejected