mstpr-brainbot - Market Allows Zero Amount Positions Which Can Cause Vault Rebalancing to Revert

[sherlock-admin]

mstpr-brainbot

high

Market Allows Zero Amount Positions Which Can Cause Vault Rebalancing to Revert

Summary

When adjusting positions, it correctly calculates the amount and respects limits, but it runs into a problem when dealing with a zero amount in the openMake and closeMake functions. This problem occurs when the Vault doesn't wish to change any position but still communicates a "0" amount to the product, resulting in revert and halting the rebalancing process.

Vulnerability Detail

When decreasing a position, the Market Vault calculates the available room and permits only the closing of positions up to the point where the maker equals the taker. If the market's taker is greater than the maker, the amount is deemed as zero. Conversely, when increasing a position, the Vault checks if the product's maker limit is exceeded. If it is, the Vault assigns the amount as zero. Following this, the Vault executes the closeMake and openMake functions using the calculated amount.

However, an issue arises with the Vault's openMake and closeMake functions operating with a zero amount. Even though the Vault might not want to open or close any position, it still calls the product with a "0" amount. This leads to the modifiers throwing an error, and the rebalancing process gets stuck.

Impact

In such cases where maker limit is exceeded in product or takers > makers vaults rebalancing will be stucked. Therefore, I'll call it as high

Code Snippet

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

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

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

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

Tool used

Manual Review

Recommendation

If the calculated openMake/closeMake amount is "0" don't call the products functions. Just exit the function.

[mstpr]

Escalate for 10 USDC

This is a valid issue.

The problem stated here is not that the products allow "0" amount positions to be created. It is the opposite, it shouldn't allow the "0" amount positions. When vault decides that the maker limit is reached, it sets the position to be opened as "0" and calls the product. If the position to be opened calculated as "0" the product will revert because of the modifiers.

Example:

Imagine the product has more takers than makers and the position needs to be decreased in vault. Vault will calculate the closeMake amount as 0 as seen here, and calls the product with closeMake(0)
https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L502-L509

closeMake function has a modifier called takerInvariant which will be executed after the function body and it checks the socialization factor which is maker/taker, and since takers are greater than makers this modifier will revert because socialization factor indeed lesser than 1.

https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial/contracts/product/Product.sol#L535-L544

So, vault knew that its going to revert and that's why it calculated it as 0 but it shouldn't have called the product because product allows "0" amount positions and modifiers will revert.

same is applicable with openMake. If the makerLimit is exceeded in product, vault calculates the amount of positions to be opened as "0" but it calls the product with openMake("0") and it reverts in the makerInvariant modifier.

The correct behaviour would be exiting the function if the position to be opened or closed found to be "0".

[sherlock-admin]

Escalate for 10 USDC

This is a valid issue.

The problem stated here is not that the products allow "0" amount positions to be created. It is the opposite, it shouldn't allow the "0" amount positions. When vault decides that the maker limit is reached, it sets the position to be opened as "0" and calls the product. If the position to be opened calculated as "0" the product will revert because of the modifiers.

Example:

Imagine the product has more takers than makers and the position needs to be decreased in vault. Vault will calculate the closeMake amount as 0 as seen here, and calls the product with closeMake(0)
https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L502-L509

closeMake function has a modifier called takerInvariant which will be executed after the function body and it checks the socialization factor which is maker/taker, and since takers are greater than makers this modifier will revert because socialization factor indeed lesser than 1.

https://github.com/sherlock-audit/2023-05-perennial/blob/0f73469508a4cd3d90b382eac2112f012a5a9852/perennial-mono/packages/perennial/contracts/product/Product.sol#L535-L544

So, vault knew that its going to revert and that's why it calculated it as 0 but it shouldn't have called the product because product allows "0" amount positions and modifiers will revert.

same is applicable with openMake. If the makerLimit is exceeded in product, vault calculates the amount of positions to be opened as "0" but it calls the product with openMake("0") and it reverts in the makerInvariant modifier.

The correct behaviour would be exiting the function if the position to be opened or closed found to be "0".

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.

[arjun-io]

We're looking into this for the socialization case with a 0 value position. However, in general an openMake/closeMake call (even if 0) is required for each rebalance because a settlement version needs to be stamped for that action. This ensures that valueAtVersion(version + 1) is available for an given stamped version in the vault.

[arjun-io]

This does appear to be a valid issue although the fix recommended is incorrect. The vault should be opening 0 value positions in some way when the socialization or makerLimit cases are hit. We'll take a closer look into the right fix here

We would say this is a medium issue though because it is an edge case in the markets and does not result in loss of user funds

[KenzoAgada]

I didn't fully understand the original issue, but the escalation made it clear.
Agreeing with sponsor that issue is a valid medium.

[jacksanford1]

Ok, valid medium

[jacksanford1]

Result:
Medium
Unique
Based on consensus from Lead Judge and Arjun after seeing escalator's comment.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• mstpr: accepted