sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

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

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: