roguereddwarf - Payoff definitions that can cross zero price are not supported

[sherlock-admin]

roguereddwarf

medium

Payoff definitions that can cross zero price are not supported

Summary

The price that is returned by an Oracle can be transformed by a "Payoff Provider" to transform the Oracle price and implement different payoff functions (e.g. 3x Leveraged ETH, ETH*ETH).

According to the documentation, any payoff function is supported: https://docs.perennial.finance/mechanism/payoff

The problem is that this is not true.

Vulnerability Detail

Payoff functions that transform the Oracle price such that the transformed price can cross from negative to positive or from positive to negative are not supported.

This is because there is a location in the code, where there is division by currentPrice: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L487

Thus there is a risk of division-by-zero if the price was allowed to cross the 0 value.

Impact

In contrast to what is specified in the Docs, not all payoff functions are supported which can lead to a situation where it causes division-by-zero and thereby DOS.

Code Snippet

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

Tool used

Manual Review

Recommendation

Establish which payoff functions are supported and make it clear in the docs or find a meaningful way to handle the above case. If currentPrice==0, a meaningful value could be to set currentPrice=1

[arjun-io]

We'll update the docs to clarify some payoffs which aren't supported

[SergeKireev]

Escalate for 10 USDC

This should be low/informational, since payoffs crossing zero price are indeed supported because absolute value of the price is taken in account to compute leverage: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L486

However the concept of leverage itself makes no sense if price == 0.

This is an undefined behavior in a very very unlikely edge case (Someone defines a zero crossing payoff, and this payoff ends up being exactly zero for a prolonged duration)

Additionally the recommendation seems to do more harm than good:

If currentPrice==0, a meaningful value could be to set currentPrice=1

If market conditions mark an asset to zero, it seems best to fail than to adjust the price to another arbitrary value

[sherlock-admin]

Escalate for 10 USDC

This should be low/informational, since payoffs crossing zero price are indeed supported because absolute value of the price is taken in account to compute leverage: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L486

However the concept of leverage itself makes no sense if price == 0.

This is an undefined behavior in a very very unlikely edge case (Someone defines a zero crossing payoff, and this payoff ends up being exactly zero for a prolonged duration)

Additionally the recommendation seems to do more harm than good:

If currentPrice==0, a meaningful value could be to set currentPrice=1

If market conditions mark an asset to zero, it seems best to fail than to adjust the price to another arbitrary value

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.

[roguereddwarf]

Escalate for 10 USDC

I disagree with the first escalation and think this should remain "Medium" severity.

The docs clearly state that ANY payoff function is supported which includes a payoff function that crosses zero.

This is an edge case in which the Vault does not work, i.e. users cannot interact with the Vault which puts the users' funds at risk because they are unable to withdraw them and thereby are at risk of liquidation / adverse price movements.

[sherlock-admin]

Escalate for 10 USDC

I disagree with the first escalation and think this should remain "Medium" severity.

The docs clearly state that ANY payoff function is supported which includes a payoff function that crosses zero.

This is an edge case in which the Vault does not work, i.e. users cannot interact with the Vault which puts the users' funds at risk because they are unable to withdraw them and thereby are at risk of liquidation / adverse price movements.

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]

This is indeed quite an edge case, but it is there, and part of an audit's job is to find the edge cases. Product owners can choose any payoff function, including one that returns 0 at some instances. The issue can cause temporary DOS due to product flywheel jamming. I think medium severity is appropriate.

[mstpr]

Didn't escalated it because @SergeKireev already explained in very good. This is indeed an informational or invalid issue.

As stated in the readme:

if the product payoff is dodgy, then its users responsibility to deposit in such product.

Also, how can currentPrice returns "0" ? If the price of something is "0" then how can you potentially trade and even leverage the asset? As @SergeKireev said, if the current price returns to be "0", then it's best to leave it because there is something wrong with the oracle.

Also, I don't think document needed to say "0 * ETH" positions are not allowed. It's clearly doesn't make sense to inform users in the document that this is not accepted.

Clearly, 0 payoff functions are not supported because they don't make sense. Also, if any of the products price is cross to 0 Perennial still allows that, it just reverts because that's the right thing to do.

[jacksanford1]

I'm leaning towards Invalid because the Sherlock judging rules state that temporary freezing/DOS is not a valid issue: https://docs.sherlock.xyz/audits/judging/judging#some-standards-observed

Could be valid if @roguereddwarf can come up with an attack vector beyond temporary freezing.

@mstpr also brings up a "trusted actor" problem which I haven't fully evaluated.

[roguereddwarf]

The impact beyond temporary freezing is that a user can e.g. not redeem his funds in an emergency event, risking further losses.

[jacksanford1]

Ok, I agree with the premise that an extended DOS could result in unintended behavior like liquidations which the user may not have a chance to prevent when the DOS ends.

@roguereddwarf @mstpr @KenzoAgada @SergeKireev How should we think about the realistic length of the DOS and the damage that could result to users because of it? Is this DOS going to last a few blocks maximum? Or a few months potentially?

[SergeKireev]

@jacksanford1 With all due respect, discussing the details of the DOS in this case is missing the bigger picture;

A market accepting a zero price is broken in more serious ways:

During the period in which the oracle returns zero, any trader Alice can open positions of arbitrary size without having any collateral deposited since maintenance is zero: https://github.com/sherlock-audit/2023-05-perennial-SergeKireev/blob/c90b017ad432b283bcfbec594f80e18204ee98c3/perennial-mono/packages/perennial/contracts/product/types/position/AccountPosition.sol#L68-L73

and collateral can be zero: https://github.com/sherlock-audit/2023-05-perennial-SergeKireev/blob/c90b017ad432b283bcfbec594f80e18204ee98c3/perennial-mono/packages/perennial/contracts/collateral/Collateral.sol#L211-L227

So if the oracle ever updates again to a non-zero price, Alice creates a shortfall of an arbitrary size on the protocol.

In that regard, by reverting, BalancedVault has a more reasonable behavior than the underlying market when oracle price is zero.

However BalancedVault links many heterogenous markets together, and in the scenario in which one fails, the whole vault fails and funds may stay locked up. I would argue that this report is an example of the broader #232 and thus should be a valid duplicate.

[mstpr]

@jacksanford1 How can price be "0" ? I think we need the answer for this question.

Obviously creating a "0 * ETH" payoff product is not make sense and even though its created nobody would deposit to such product.

If price is "0" this can be possible because of an oracle error. I don't think Chainlink validations are a medium finding in Sherlock. If it is, then there are many other submissions regards to that. Example: https://github.com/sherlock-audit/2023-05-perennial-judging/issues/62 I am sure there are many others aswell

[KenzoAgada]

@roguereddwarf @mstpr @KenzoAgada @SergeKireev How should we think about the realistic length of the DOS and the damage that could result to users because of it? Is this DOS going to last a few blocks maximum? Or a few months potentially?

I would consider it few blocks maximum.

Anyway, @SergeKireev 's last comment pretty much convinced me that actually reverting is a relatively logical thing to do in this scenario.

My original thinking was more like, we can not know how funky the product owner would define his payout function. (shifting, steps, more inventions...). And if it's 0, accidentally or purposefully, indeed the product would be jammed. But as the escalators wrote, there's no real meaning to a payoff function returning 0.

I already considered this quite the edge case before... At the moment I agree with the escalators (love using that word 🙂) that medium severity is too much.

[jacksanford1]

Ok, the original issue impact was focused on DOS, so I'll stay on that topic. If Kenzo is correct and the length of the DOS is a "few blocks maximum" then this should be a low severity issue. But open to arguments from @roguereddwarf or anyone else as to why it should stay a Medium.

[roguereddwarf]

I'd like to add that the price of an asset might just go to zero (look e.g. what we've seen with FTX and FTT).

In this case it should still be possible for users of the Vault that have redeemed their shares to call the claim function and get their share of the assets that were within the Vault and have not been lost (pointing this out because someone might argue price=0 implies that there are no funds left in the Vault):

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

Which would however revert when _rebalance is called which is my whole point in this report.

While the title of my report says "that can cross zero price" this technically includes the case when the price of an asset just goes to zero due to an issue with the asset.

In this case the DOS can be for an unlimited amount of time.

[KenzoAgada]

While the payoff function might transform the price and return 0 and then withdrawals would be bricked, I don't think the oracle price can return 0... Chainlink has a minAnswer which is bigger than 0.

[jacksanford1]

Fair point from Kenzo that @roguereddwarf can respond to. I also think FTT is a tough example because I'm not aware that it ever went to 0. And I can't think of another token that actually hit zero ever. I'm not saying that it's impossible, I'm saying that the likelihood is extremely low because there's always a chance that a token could have value in the future, so there is always some bid for it. And if the severity of this issue is based on the likelihood of a token hitting zero, then I think this is relevant.

[roguereddwarf]

Fair points, and yeah there could be a small bid to get the price above 0.

Quoting from @jacksanford1 from a previous message.

Ok, I agree with the premise that an extended DOS could result in unintended behavior like liquidations which the user may not have a chance to prevent when the DOS ends.

How should we think about the realistic length of the DOS and the damage that could result to users because of it? Is this DOS going to last a few blocks maximum? Or a few months potentially?

Here's the exotic payoff function that can result in a DOS for a few months:

As the price goes below 1700 USD and stays there, the issue becomes permanent.

[jacksanford1]

Ok @SergeKireev this makes a very important distinction between "price returned by oracle" and "transformed price after the payoff function is applied."

I think the issue is saying that the transformed price is the one that is problematic if it hits zero. This would make more sense and could be a valid Medium.

@arjun-io Do you agree that the payoff function in the above graph is realistic?

[arjun-io]

@jacksanford1 It could be realistic since any payoff function is technically possible. I think the question here might be severity, since this isn't really a naturally occurring case and the payoff function would need to be explicitly designed in such a way that a 0 price is feasible for a long period of time.

[SergeKireev]

Ok @SergeKireev this makes a very important distinction between "price returned by oracle" and "transformed price after the payoff function is applied."

I think the issue is saying that the transformed price is the one that is problematic if it hits zero. This would make more sense and could be a valid Medium.

@arjun-io Do you agree that the payoff function in the above graph is realistic?

Np, I don't think this changes our discussion above, I agree with the feasability of such a Payoff function.

It can make sense in implementing some derivative products such as options, as noted by @roguereddwarf in latest comment. However these products don't work more broadly on perennial as they allow for unbounded sized positions with zero collateral (see my previous comment). As such, they wouldn't make sense in the context of a BalancedVault either, that's why I argue for this issue to be a low severity.

[mstpr]

regards to @roguereddwarf payoff function,

I don't think anybody would deposit to such product that defines the payoff like this. Technically we can even create a payoff function where the price is always "0" but as long as there are no user depositing then nothing to worry about imo. Also, I think if price is ever "0" best thing to do is revert so even in such payoff it makes sense to revert.

[jacksanford1]

Agree with others that this payoff function is actually realistic and could mimic an options payoff, etc. So I'll assume there is a world where this exists and users deposit into it.

In that case, I'm sorry to drag this on, but I'd be interested in @roguereddwarf's response to Serge's comment as probably the final thing holding this back from being Medium severity:

However these products don't work more broadly on perennial as they allow for unbounded sized positions with zero collateral (see my previous comment). As such, they wouldn't make sense in the context of a BalancedVault either, that's why I argue for this issue to be a low severity.

[roguereddwarf]

Isn't this then just saying more broadly that there are other things that break under the assumption of a realistic payoff function?

If what Serge is saying is correct, then part A of the protocol breaks before part B under the assumption of a realistic payoff function.

In the case that Serge is wrong, then part A does not break but part B breaks.

Either way there is an issue with realistic payoff functions (options payoff).

[SergeKireev]

Isn't this then just saying more broadly that there are other things that break under the assumption of a realistic payoff function?

If what Serge is saying is correct, then part A of the protocol breaks before part B under the assumption of a realistic payoff function.

In the case that Serge is wrong, then part A does not break but part B breaks.

Either way there is an issue with realistic payoff functions (options payoff).

Yeah I agree with that, only thing is since it's not possible to create a functional market for this feasible payoff function, it is very unlikely somebody would do it at all.

Then this means that a BalancedVault created upon such a market would be even less likely to exist, which means that there is no use implementing the remediation suggested in this report, and the severity should be low

[roguereddwarf]

Why is it not possible?

Assume the payoff function I posted above:

Assume the current price of ETH is 2000 USD.

Now the price drops below 1700 USD and we run into the issue.

Nothing prevents one from creating such a payoff function and it works fine as long as the price is above 1700 USD.

[SergeKireev]

Why is it not possible?

Assume the payoff function I posted above:

Assume the current price of ETH is 2000 USD.

Now the price drops below 1700 USD and we run into the issue.

Nothing prevents one from creating such a payoff function and it works fine as long as the price is above 1700 USD.

As I said earlier, the market would be completely broken, since it allows to take positions of any size without collateral

[roguereddwarf]

@jacksanford1 With all due respect, discussing the details of the DOS in this case is missing the bigger picture;

A market accepting a zero price is broken in more serious ways:

During the period in which the oracle returns zero, any trader Alice can open positions of arbitrary size without having any collateral deposited since maintenance is zero: https://github.com/sherlock-audit/2023-05-perennial-SergeKireev/blob/c90b017ad432b283bcfbec594f80e18204ee98c3/perennial-mono/packages/perennial/contracts/product/types/position/AccountPosition.sol#L68-L73

and collateral can be zero: https://github.com/sherlock-audit/2023-05-perennial-SergeKireev/blob/c90b017ad432b283bcfbec594f80e18204ee98c3/perennial-mono/packages/perennial/contracts/collateral/Collateral.sol#L211-L227

So if the oracle ever updates again to a non-zero price, Alice creates a shortfall of an arbitrary size on the protocol.

In that regard, by reverting, BalancedVault has a more reasonable behavior than the underlying market when oracle price is zero.

However BalancedVault links many heterogenous markets together, and in the scenario in which one fails, the whole vault fails and funds may stay locked up. I would argue that this report is an example of the broader #232 and thus should be a valid duplicate.

This is the previous comment you are referring to.

Maybe I have missed the broader picture in that I didn't realize that in addition to the DOS there are other problems as well (which as you rightfully point out are more serious).

However I identified the payoff function causing the issue in the first place and even the sponsor said this can be a realistic scenario.

As long as the price of ETH would not drop below 1700 USD in my example there would be no issue so this might only be discovered when it's too late.

I agree that this finding is probably best considered as a duplicate of #232.

Can we agree on this @SergeKireev @jacksanford1 ?

[SergeKireev]

Agree with this, thanks for acknowledging @roguereddwarf

[jacksanford1]

Result: Medium Unique It seems like this issue is likely enough and severe enough to be considered a Medium. However I don't believe it is similar enough to #232 to be considered a duplicate, so it will be a unique Medium.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• roguereddwarf: accepted
• SergeKireev: rejected

[arjun-io]

Fixed via a comment: https://github.com/equilibria-xyz/perennial-mono/pull/203