sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

cergyk - Malicious trader can bypass utilization buffer #75

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

cergyk

high

Malicious trader can bypass utilization buffer

Summary

A malicious trader can bypass the Utilisation buffer, and push utilisation to 1 on any product.

Vulnerability Detail

Perenial products use a utilization buffer to prevent taker side to DOS maker by taking up all the liquidity: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L547-L556

Makers would not be able to withdraw if utilization would reach 100% because of takerInvariant which is enforced during closeMake: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L535-L544

A malicious trader can bypass the utilisation buffer by first opening a maker position, open taker position and close previous maker position. The only constraint is that she has to use different accounts to open the maker positions and the taker positions, since perennial doesn't allow to have maker and taker positions on a product simultaneously.

So here's a concrete example:

Let's say the state of product is 900 USD on the maker side, 800 USD on the taker side, which respects the 10% utilization buffer.

Example

Using account 1, Alice opens up a maker position for 100 USD, bringing maker total to 1000 USD.

Using account 2, Alice can open a taker position for 100 USD, bringing taker to 900 USD, which respects the 10% utilization buffer still.

Now using account 1 again, Alice can close her 100 USD maker position and withdraw collateral, clearing account 1 on perennial completely.

This brings the utilization to 100%, since taker = maker = 900 USD.

This is allowed, since only takerInvariant is checked when closing a maker position, which enforces that utilization ratio is lower than or equal to 100%.

Impact

Any trader can bring the utilization up to 100%, and use that to DoS withdrawals from Products and Balanced vaults for an indefinite amount of time. This is especially critical for vaults, since when any product related to any market is fully utilized, all redeems from the balanced vault are blocked.

Code Snippet

Tool used

Manual Review

Recommendation

If a safety buffer needs to be enforced for the utilisation of products, it needs to be enforced on closing make positions as well.

arjun-io commented 1 year ago

This is a good callout and a valid issue but since the impact existed before this utilization buffer feature was added we disagree with the severity. The utilization buffer is not intended to totally fix the 100% utilization issue against malicious users, but instead provide a buffer in the normal operating case.

KenzoAgada commented 1 year ago

In addition to the sponsor's comment, also worth noting that a malicious user will not gain anything from doing this. Indeed seems like medium severity is more appropriate. Downgrading to medium.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

The assumption here is that Alice can close position immediately. However, in order to close a position Alice needs to wait the settlement and the meanwhile Alice can occur losses or profit. This is not a safe operation for Alice to do. Alice can do this if she is willing to take the risk. I think the utilization buffer works as intented in this scenario, where it forces Alice to create a maker position in order to create a taker position first and since Alice can't withdraw immediately she is subject to her maker positions pnl for at least an oracle version.

I think this an invalid issue

You've deleted an escalation for this issue.
SergeKireev commented 1 year ago

The assumption here is that Alice can close position immediately. However, in order to close a position Alice needs to wait the settlement and the meanwhile Alice can occur losses or profit. This is not a safe operation for Alice to do. Alice can do this if she is willing to take the risk. I think the utilization buffer works as intented in this scenario, where it forces Alice to create a maker position in order to create a taker position first and since Alice can't withdraw immediately she is subject to her maker positions pnl for at least an oracle version.

I think this an invalid issue

The described operation can be done atomically because the invariant takerInvariant which is bypassed here checks only next amounts, and so Alice does not in fact need to expose herself to the market (She can just fill all maker capacity before Bob's transaction, and close after the tx and before next settlement) https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L535-L544

mstpr commented 1 year ago

The assumption here is that Alice can close position immediately. However, in order to close a position Alice needs to wait the settlement and the meanwhile Alice can occur losses or profit. This is not a safe operation for Alice to do. Alice can do this if she is willing to take the risk. I think the utilization buffer works as intented in this scenario, where it forces Alice to create a maker position in order to create a taker position first and since Alice can't withdraw immediately she is subject to her maker positions pnl for at least an oracle version.

I think this an invalid issue

The described operation can be done atomically because the invariant takerInvariant which is bypassed here checks only next amounts, and so Alice does not in fact need to expose herself to the market (She can just fill all maker capacity before Bob's transaction, and close after the tx and before next settlement) https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L535-L544

You are correct, sorry for the misunderstanding of the scenario you described above. Deleting my escalations comment

mstpr commented 1 year ago

Escalate for 10 USDC

I rethought of the issue and I think this is a low issue.

Issue does not add any benefits to the user and in fact, it is even discouraging for the user to not take this action due to the funding fee increase. Since the funding fee follows a curve model the funding fee will be taken at 100% utilization is quite high and it is not a good thing that any taker would want. There are no funds in danger and there isn't any benefits of doing such thing.

sherlock-admin commented 1 year ago

Escalate for 10 USDC

I rethought of the issue and I think this is a low issue.

Issue does not add any benefits to the user and in fact, it is even discouraging for the user to not take this action due to the funding fee increase. Since the funding fee follows a curve model the funding fee will be taken at 100% utilization is quite high and it is not a good thing that any taker would want. There are no funds in danger and there isn't any benefits of doing such thing.

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

Hmm... Rethinking about the issue, I admit it might not be enough for medium severity. Leaving for Sherlock to decide.

jacksanford1 commented 1 year ago

Impact from issue:

Any trader can bring the utilization up to 100%, and use that to DoS withdrawals from Products and Balanced vaults for an indefinite amount of time. This is especially critical for vaults, since when any product related to any market is fully utilized, all redeems from the balanced vault are blocked.

Based on Sherlock's DOS rules: https://docs.sherlock.xyz/audits/judging/judging#some-standards-observed

Doesn't seem like this should be a valid Medium. @cergyk would need to explain how the attack can be profitable for the attacker or show a difference between cost incurred and damage created that is many orders of magnitude.

SergeKireev commented 1 year ago

@CergyK would need to explain how the attack can be profitable for the attacker or show a difference between cost incurred and damage created that is many orders of magnitude.

Due to the nature of BalancedVault, multiple markets of different nature are linked together. If one is fully utilized, it blocks the BalancedVault for all of the markets as stated in the initial report:

This is especially critical for vaults, since when any product related to any market is fully utilized, all redeems from the balanced vault are blocked.

So there can indeed be a magnitude between cost incurred and damage created.

Suppose Balanced Vault on markets:

Malicious user Alice would only have to fully utilize 100$ on the .$PEPE market to block 1.000.000$ in redeemable assets on $ETH because the two markets are linked by the BalancedVault

jacksanford1 commented 1 year ago

Thanks @SergeKireev. Agree in that case the cost could be very low for a DOS of significant magnitude. I don't think it's a High, but it seems like it's worthwhile to make this a Medium.

Open to arguments from @mstpr on why it should be Low instead.

jacksanford1 commented 1 year ago

Result: Medium Has duplicates Based on argument from Serge and no follow-up response from mstpr, leaving this as a Medium.

sherlock-admin2 commented 1 year ago

Escalations have been resolved successfully!

Escalation status: