mstpr-brainbot - Unintended Vault Operation Due to Product Settling and Oracle Version Skips

[sherlock-admin]

mstpr-brainbot

high

Unintended Vault Operation Due to Product Settling and Oracle Version Skips

Summary

Vault operation can behave unexpectedly when products settle and skip versions in their oracles. This problem arises when the vault assumes a non-existent version of a product, leading to miscalculations in the _assetsAtEpoch() function. This inaccuracy can affect the distribution of shares among depositors, redeemers, and claimers, disrupting the balance of the market.

Vulnerability Detail

When a product progresses to a new version of its oracle, it first settles at the current version plus one, then jumps to the latest version. For instance, if a product's latest version is 20 and the current version is 23, the product will first settle at version 21 and then jump from 21 to 23, bypassing version 22. This process can lead to unintended behavior in the vault, particularly if the vault's deposited epoch points to the bypassed version, which can result in a potential underflow and subsequent rounding to zero.

Consider this scenario: A vault is operating with two markets, each having two products. Let's assume the vault has equal positions and collateral in both markets, with all assets priced at $1 for simplicity. Also, assume that Market0 contains product A and product B, while Market1 contains product C and product D.

The latest version of product A is 20, while the vault's latest epoch is 2, which corresponds to version 20 for product A. Similarly, the latest version of product C is 10, with the vault's latest epoch at 2, corresponding to version 10 for product A.

Assume there are no positions created in product C, D, and also no deposits made to the vault, meaning the vault has not opened any positions on these products either. The oracle version for product C, D gets updated twice consecutively. Since there are no open positions in product C, D, their products will progress to version 12 directly, bypassing version 11.

This circumstance renders the epoch stale, which will lead to miscalculations in deposits, claims, and redemptions. During the _assetsAtEpoch() function, the vault incorrectly assumes that there is a version 11 of product C, D. However, these products have skipped version 11 and advanced to 12. Since the latest version of product C, D is greater than the version the vault holds, the vault presumes that version 11 exists. However, as version 11 doesn't exist, the _accumulatedAtEpoch will be the negative of whatever was deposited. This leads the vault to incorrectly assume a large loss in that market. Consequently, depositors can gain more shares, while redeemers and claimers may receive fewer shares.

Impact

If the above scenario happens, vault users can incur extreme losses on claim and redeem operations and depositors can earn uneven shares.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L84-L130
settle

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/interfaces/types/PrePosition.sol#L133-L136
if no open positions in preposition settle version is current version

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L376

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L823-L843
checks for +1 version, if latestVersion > currentVersion, it assumes vault lost all the funds in that market.

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L797-L805

Tool used

Manual Review

Recommendation

Don't check +1 version, check the current version recorded for that epoch in vault and the latest version of the product, cover all the versions

[kbrizzle]

It is guaranteed that all applicable versions have a corresponding checkpoint in the underlying markets at version n because product.settle() is called on every market (here from here) when an epoch snapshot takes place.

Further, when there is a deposit or withdrawal _updateMakerPosition will create a new position in the underlying product here, which will create a checkpoint for n + 1.

[KenzoAgada]

Suspected as much but wanted to make sure... Closing as invalid.

[KenzoAgada]

After internal discussion: the issue is valid, but the watson didn't fully identify the exact complex edge case where it will materialize. Therefore downgrading to medium severity.

[mstpr]

Escalate for 10 USDC

I am not sure what's the edge case, it is straightforward.

A vault is synced with its underlying product as long as it is interacting with its products. A product can advance more oracle versions while vault still outdates with another one if there isn't any interactions been made in due time. At the end of the _settle function the correct and the most updated oracle version is synced. However, just in the settlement process vault only checks the latest synced version of its and the one after that (which there might not be a +1 version due to skipping).

If a product has in version 12 and the previous version of it was 10 (means that product skipped version 11 and went directly to version 12) and the vaults latest synced version is 10 (no interactions made with vault so vault still thinks the latest version of the product is 10), the next time someone calls deposit/redeem/claim or in general anything that would trigger the settlement process, the accumulated assets at that epoch will be calculated as follows:

products latest version of the underlying product is 10. So _accumulatedAtEpoch will calculate the balance respect to oracle version 10 and 11. However, there is no version 11, and default it will be 0 and since the latest version of the product (12) > 10 (latest synced oracle version of vault) the accounting will be mess. Since there is no oracle version 11, the accumulated balance will be calculated as the negative of version 10. If the epoch is stale, there can be deposits/redeems/claims in the latestEpoch and when the sync() happens the accounting will go off.

Also, this is directly related with user funds and huge losses can be reported, so I think this is a valid high.

[sherlock-admin]

Escalate for 10 USDC

I am not sure what's the edge case, it is straightforward.

A vault is synced with its underlying product as long as it is interacting with its products. A product can advance more oracle versions while vault still outdates with another one if there isn't any interactions been made in due time. At the end of the _settle function the correct and the most updated oracle version is synced. However, just in the settlement process vault only checks the latest synced version of its and the one after that (which there might not be a +1 version due to skipping).

If a product has in version 12 and the previous version of it was 10 (means that product skipped version 11 and went directly to version 12) and the vaults latest synced version is 10 (no interactions made with vault so vault still thinks the latest version of the product is 10), the next time someone calls deposit/redeem/claim or in general anything that would trigger the settlement process, the accumulated assets at that epoch will be calculated as follows:

products latest version of the underlying product is 10. So _accumulatedAtEpoch will calculate the balance respect to oracle version 10 and 11. However, there is no version 11, and default it will be 0 and since the latest version of the product (12) > 10 (latest synced oracle version of vault) the accounting will be mess. Since there is no oracle version 11, the accumulated balance will be calculated as the negative of version 10. If the epoch is stale, there can be deposits/redeems/claims in the latestEpoch and when the sync() happens the accounting will go off.

Also, this is directly related with user funds and huge losses can be reported, so I think this is a valid high.

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.

[securitygrid]

Comment from a waston:

/**
     * @notice Rebalances the collateral and position of the vault without a deposit or withdraw
     * @dev Should be called by a keeper when a new epoch is available, and there are pending deposits / redemptions
     */
    function sync() external {
        syncAccount(address(0));
    }

sync() Should be called by a keeper when a new epoch is available. If the keeper is running normally, there will be no such assumptions in the report: if a product's latest version is 20 and the current version is 23. Because sync() will call product._settle internally.
so the above scenario will not happen.

[mstpr]

/**
     * @notice Rebalances the collateral and position of the vault without a deposit or withdraw
     * @dev Should be called by a keeper when a new epoch is available, and there are pending deposits / redemptions
     */
    function sync() external {
        syncAccount(address(0));
    }

sync() Should be called by a keeper when a new epoch is available. so the above scenario will not happen.

Product advances an oracle version and vault syncs.

When a product advances to version 12 from 10 by skipping version 11 (which is possible if there are no open positions in version 10)

and if the epoch is stale there can be deposits made by users.

Now, even if keeper would call the sync() after 1 second the above scenario is consistent.

[KenzoAgada]

I don't have a lot to add on this escalation.
On one hand, it seems the watson identified a valid critical issue.
On the other hand, if I understand the sponsor correctly, they said that report was not very useful or detailed nor enough to really understand and fix the problem.
Up to Sherlock to decide severity.

[jacksanford1]

@securitygrid Are you satisfied with @mstpr's last response?

I don't believe the likelihood and severity potential for this issue combine to warrant a High.

However it seems to be a legitimate issue (or very close to describing one) and so it seems like a Medium.

[securitygrid]

If the keeper is working, why is there a delay of 2 versions?

[jacksanford1]

I guess @mstpr can answer that best

[mstpr]

I don't think you get the issue here @securitygrid. As stated above, vaults and products separate things. Product advance a version first then vault syncs.

Vault only advances a version if the underlying products advances to the next version. It is completely not about keepers.
If marketA (2 products, long and short) and marketB (2 products, long and short) are in oracle version 3 and vault is in epoch 2,
when marketA advances to version 3 but marketB is still in version 2, the vault is still in epoch2, vault can't advance to epoch3 before marketB is advanced to version 3 aswell.

[jacksanford1]

@securitygrid Any thoughts on mstpr's latest comment?

[jacksanford1]

Result:
Medium
Unique

On one hand, it seems the watson identified a valid critical issue.
On the other hand, if I understand the sponsor correctly, they said that report was not very useful or detailed nor enough to really understand and fix the problem.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: rejected