IllIllI - Price band caps apply to decreasing orders, but not to liquidations

[sherlock-admin2]

IllIllI

medium

Price band caps apply to decreasing orders, but not to liquidations

Summary

Price band caps limit the price at which an order can be settled (e.g. someone trying to reduce their exposure in order to avoid liquidation), but liquidations have no such limitation.

Vulnerability Detail

Price bands are used in order to ensure that users can't trade a extreme prices, which would result in lower-than usual fees, and liquidations to be less likely, because borrowing fees, funding fees, and liquidation penalties are all based on the opening notional value, rather than the current position size's value, and the reduced fee wouldn't be enough incentive to liquidate the position.

Having price caps means that even if there are two willing parties willing to settle a trade at a market-determined price, they will be prevented from doing so. In traditional financial markets, there are also trading halts when there is extreme price movement. The difference here is that while no trades are allowed during market halts in traditional finance, in the Perpetual system, liquidations are allowed to take place even if users can't close their positions.

Impact

The whole purpose of the OracleMaker is to be able to provide liquidity at all times, though this liquidity may be available at a disadvantageous price. If there's a price band, anyone who tries to exit their position before they're liquidated (incurring a fee charged on top of losing the position), will have their orders rejected, even at the disadvantageous price. Note that orders interacting with the OracleMaker, and with other non-whitelisted makers (other traders) are executed by Relayers, who are expected to settle orders after a delay, so definitionally, they'll either be using a stale oracle price or will be executing after other traders have had a change to withdraw their liquidity. In either case, during periods of high volatility and liquidations, the price being used will no longer match the market's clearing price.

Code Snippet

Orders modifying/creating positions have price band checks:

// File: src/clearingHouse/ClearingHouse.sol : ClearingHouse._openPosition()   #1

321                if (params.isBaseToQuote) {
322                    // base to exactOutput(quote), B2Q base- quote+
323                    result.base = -oppositeAmount.toInt256();
324                    result.quote = params.amount.toInt256();
325                } else {
326                    // quote to exactOutput(base), Q2B base+ quote-
327                    result.base = params.amount.toInt256();
328                    result.quote = -oppositeAmount.toInt256();
329                }
330            }
331:@>         _checkPriceBand(params.marketId, result.quote.abs().divWad(result.base.abs()));

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L321-L331

but liquidations don't have any price caps, and dont have any authorization checks, which means it can be executed without going through the order gateways and their relayers:

// File: src/clearingHouse/ClearingHouse.sol : ClearingHouse.params   #2

160        /// @inheritdoc IClearingHouse
161        function liquidate(
162            LiquidatePositionParams calldata params
163:       ) external nonZero(params.positionSize) returns (int256, int256) {

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/clearingHouse/ClearingHouse.sol#L160-L163

Tool used

Manual Review

Recommendation

Don't use the price bands for trades against the OracleMaker. As is shown by some of my other submissions, removing the price bands altogether is not safe.

[sherlock-admin2]

2 comment(s) were left on this issue during the judging contest.

santipu_ commented:

Low - Relayers will execute delayed order with the current market price, not a stale one. And the spread price will never be enough to make the price deviate too much to fail the price band check. If it happened, it should be considered an admin mistake.

takarez commented:

seem valid; medium(2)

[vinta]

This is invalid I suppose. Indeed, openPosition() does have price band but liquidate() doesn't. However, liquidation is "liquidator takes over the liquidatable position at Pyth oracle price". So no need to have price band for liquidation I think.

Though I agree that trading with OracleMaker doesn't really need price band, but I guess it won't hurt if we still check price band for OracleMaker. It's simpler on implementation (no extra code to check whether it's trading with OracleMaker).

[IllIllI000]

@vinta This submission is about the fact that a normal user will be unable to reduce their position if the OracleMaker's price is outside the price bands, leading to a liquidation they can't do anything to prevent. Can you elaborate on what part is invalid?

[vinta]

@IllIllI000 But how does OracleMaker's price be outside the price band? Since the price band is based on the oracle price +- xx% and OracleMaker's price is from the same oracle. Liquidation is also using the same oracle price.

[IllIllI000]

@vinta if traders keep hitting the same side of the bid or ask, the OracleMaker quotes a worse and worse price for the next trade. Eventually, the next 'worse price' will be pushed outside of the price bands, even though the pyth/oracle price is still at the midpoint of (within) the price band. The liquidation will be using the midpoint, but a trader wanting to reduce their position by reducing against the OracleMaker will only have access to the 'worse price', which may be outside of the bands and will therefore be rejected

[vinta]

@IllIllI000 You're right, that would be a problem. Yes, this is valid! Thank you for pointing out this.

[sherlock-admin4]

The protocol team fixed this issue in the following PRs/commits: https://github.com/perpetual-protocol/perp-contract-v3/pull/17

[nirohgo]

Escalate

This is a low:

1. As the finding mentions, this may only be relevant for relayed trades that are delayed during high volatility. (either trader-to-trader or trader-to-oracle-maker)
2. With regards to the Oracle maker, it always quotes within its configured max spread from the oracle price, so as long as the maximum spread is smaller than the price band it won't quote outside the price band (Admins are trusted to set these values correctly).
3. So the only relevant case is trader-to-trader positions sent through the relayer where the pyth (market) price drops so quickly that it's outside the position's price band by the time it's settled (but not quickly enough to take the position directly from solid to liquidatable because then liquidation bots win anyway). However, if market conditions are that voletile and the trader tries to do an emeargency exit before liquidation, they can always use the Oracle Maker through the relayer which, as mentioned, will work inspite of the price band, or the SpotHedge maker directly.

[sherlock-admin2]

Escalate

This is a low:

1. As the finding mentions, this may only be relevant for relayed trades that are delayed during high volatility. (either trader-to-trader or trader-to-oracle-maker)
2. With regards to the Oracle maker, it always quotes within its configured max spread from the oracle price, so as long as the maximum spread is smaller than the price band it won't quote outside the price band (Admins are trusted to set these values correctly).
3. So the only relevant case is trader-to-trader positions sent through the relayer where the pyth (market) price drops so quickly that it's outside the position's price band by the time it's settled (but not quickly enough to take the position directly from solid to liquidatable because then liquidation bots win anyway). However, if market conditions are that voletile and the trader tries to do an emeargency exit before liquidation, they can always use the Oracle Maker through the relayer which, as mentioned, will work inspite of the price band, or the SpotHedge maker directly.

You've created a valid escalation!

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.

[IllIllI000]

The goal of the OracleMaker is to provide liquidity essentially at all times in order to collect fees (...undesirable because he won’t be earning fees on that side https://perp.notion.site/PythOraclePool-e99a88be051f4bc8be0b1310eb982cd4 ), and requiring that the max spread be smaller than the price bands severely limits this ability, so I don't think it's reasonable to say that the admin is supposed to do this when it contradicts the design of the maxSpread being a Sensitivity of the maker’s price premium to its risk exposure https://perp.notion.site/PythOraclePool-e99a88be051f4bc8be0b1310eb982cd4 , not a proxy for the price bands. Furthermore, the escalator's assertion that the price bands must be smaller than the max spread essentially trades one risk for another, without regard to the effects of the other risk (see the other valid bugs relating to abusing the price bands). I'll also note that the sponsor has already provided a PR for this issue, so it's not a risk they're willing to take.

[nirohgo]

Still, a lot of stars need to align for this to happen (max spread happens to be larger than price bands, sudden but not too sudden price drop, OM quote is at the maximum spread) and when they do, the trader has the option to use SHBM.

[nevillehuang]

I believe medium severity is appropriate for this issue.

[WangSecurity]

Agree that this issue should remain medium since it causes loss of funds under certain external conditions and breaks core functionality.

Planning to reject the escalation and leave the issue as it is.

[nirohgo]

@WangSecurity please note that this is invalid because it requires a (trusted) admin to make a configuration error: The Price Band config is the general, systemwide restriction on position price deviation from the oracle price. It applies to all positions and will fail/revert any position settlement that breaks this config. The max spread config is a specific configuration for the Oracle Maker that determines the maximum it's price can deviate from the oracle price. Setting a specific config to a value that exceeds a system wide limit (and will surely fail because of that) is a clear admin error.

[WangSecurity]

But as I understand, @IllIllI000 proves here and here how your assumption can be broken, no? And as I understand, you agree with it here.

[nirohgo]

But as I understand, @IllIllI000 proves here and here how your assumption can be broken, no? And as I understand, you agree with it here.

@WangSecurity both comments you mentioned are not proofs but rather are based on a claim (that setting contradicting values to these configurations is not an admin error), I'm making a counter claim (that it is). I suppose you need to make a call based on your own judgement between the two claims.

[WangSecurity]

As I understand from this comment shows that the spread is not neccesseraly larger/smaller than the price band and the issue will happen if the spread is larger than the price band, then it will allow to bypass price bands during liquidations as shown here. I see that there are lots of conditions that have to align to make it work, but this is a broken core functionality allowing the attacker to bypass the price bands.

Hence, I believe medium is appropriate, planning to reject the escalation and leave the issue as it is.

[nirohgo]

@WangSecurity the first comment simply states that there's a legitimate reason to set the specific OM max spread to be larger then the general limitation of the price band. (which I claim is an admin error). The second comment merely states that the trader price will be worse than the liquidation price (which is by design and has nothing to do with this escalation) and that the trader price can be rejected because it may exceed the price band limit which again, can only happen if admins set the OM max spread to a larger value than the price band setting (which I claim is an admin error).

[WangSecurity]

@nirohgo @IllIllI000 the question is: is it documented anywhere (docs/code comments/discord msgs) that the max spread cannot exceed price bands, i.e. is it an invariant that the team will hold?

[IllIllI000]

No it's not - not that I'm aware of

[nirohgo]

@nirohgo @IllIllI000 the question is: is it documented anywhere (docs/code comments/discord msgs) that the max spread cannot exceed price bands, i.e. is it an invariant that the team will hold?

@WangSecurity So, you're saying that on Sherlock a trusted admin error only counts as such if there's a specific documentation of it? The fact that it stems from basic logic is not enough?

[WangSecurity]

@nirohgo please share where that logic comes from.

[nirohgo]

@nirohgo please share where that logic comes from.

@WangSecurity Sure: The admin sets one configuration (price band) that limits price deviation from oracle across all positions (and will fail any settlement that breaks this limit). The OM max spread configuration determines by how much the OM price can deviate from oracle (so you can think about it as a more specific configuration that applies to the OM price). If the admin sets max spread to be larger than Price Band they are making a logical error (because whenever the spread exceeds price band the settlement will fail). Its as if the admin would be breaking a rule they themselves set in another config.

[IllIllI000]

I disagree that the intention was to use the price bands as a global limit for all trades. If you look at the origin of the bands, it was this test where a user gains an advantage by trading with themselves with an arbitrary price

    // FIXME: We shouldn't allow this to happen
    // probably add price band in OrderGatewayV2 or ClearingHouse,
    // only allow order.price to be oracle price +- 10% when settling orders
    // See https://app.asana.com/0/1202133750666965/1206662770651731/f
    function test_SettleOrder_AtExtremePrice() public {

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/test/orderGatewayV2/OrderGatewayV2.settleOrder.int.t.sol#L1269-L1273

I had forgotten that I had asked this, but it's clear that their intention for the band was to protect against the scenario in the test, where a user trades with themselves:

IllIllI — 03/14/2024
hi, can you elaborate on the problem that is solved by having price bands? 
    // FIXME: We shouldn't allow this to happen
    // probably add price band in OrderGatewayV2 or ClearingHouse,
    // only allow order.price to be oracle price +- 10% when settling orders
    // See https://app.asana.com/0/1202133750666965/1206662770651731/f

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/test/orderGatewayV2/OrderGatewayV2.settleOrder.int.t.sol#L1269-L1273
is it just to avoid fat finger issues, or is there some other issue with allowing any price, that I'm missing? it looks like there isn't any negative effect to the system besides maybe the funding fee, since margin covers the price difference. the asana link is private, so I can't view it 
bchen4 ʕ̢·͡˔·ོɁ̡ — 03/15/2024
if taker order match maker order via OrderGatewayV2 with 1 wei price, these two position's openNotional are 1 wei, so they might not to pay borrowingFee or fundingFee 
and there might has problem when liquidation, because we will calculate penalty by openNotional, so liquidator might not has incentive to liquidate and it will increase bad debt risk of our system

They ended up going with using bands in the clearinghouse, where they note that a user can choose an arbitrary price, and reference the test above, and so they add a price band check later in the function.

[nirohgo]

I disagree that the intention was to use the price bands as a global limit for all trades. If you look at the origin of the bands, it was this test where a user gains an advantage by trading with themselves with an arbitrary price

    // FIXME: We shouldn't allow this to happen
    // probably add price band in OrderGatewayV2 or ClearingHouse,
    // only allow order.price to be oracle price +- 10% when settling orders
    // See https://app.asana.com/0/1202133750666965/1206662770651731/f
    function test_SettleOrder_AtExtremePrice() public {

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/test/orderGatewayV2/OrderGatewayV2.settleOrder.int.t.sol#L1269-L1273

I had forgotten that I had asked this, but it's clear that their intention for the band was to protect against the scenario in the test, where a user trades with themselves:

IllIllI — 03/14/2024
hi, can you elaborate on the problem that is solved by having price bands? 
    // FIXME: We shouldn't allow this to happen
    // probably add price band in OrderGatewayV2 or ClearingHouse,
    // only allow order.price to be oracle price +- 10% when settling orders
    // See https://app.asana.com/0/1202133750666965/1206662770651731/f

https://github.com/sherlock-audit/2024-02-perpetual/blob/02f17e70a23da5d71364268ccf7ed9ee7cedf428/perp-contract-v3/test/orderGatewayV2/OrderGatewayV2.settleOrder.int.t.sol#L1269-L1273
is it just to avoid fat finger issues, or is there some other issue with allowing any price, that I'm missing? it looks like there isn't any negative effect to the system besides maybe the funding fee, since margin covers the price difference. the asana link is private, so I can't view it 
bchen4 ʕ̢·͡˔·ོɁ̡ — 03/15/2024
if taker order match maker order via OrderGatewayV2 with 1 wei price, these two position's openNotional are 1 wei, so they might not to pay borrowingFee or fundingFee 
and there might has problem when liquidation, because we will calculate penalty by openNotional, so liquidator might not has incentive to liquidate and it will increase bad debt risk of our system

They ended up going with using bands in the clearinghouse, where they note that a user can choose an arbitrary price, and reference the test above, and so they add a price band check later in the function.

Don't see how that's relevant as it doesn't change the fact that setting OM spread to be larger than Price Band will only result in the admin DOSing their own system (whenever OM price exceeds the price band).

[IllIllI000]

It's documentation of an intended usage that has a flaw in the design, leading to a negative outcome which I believe @WangSecurity is trying to point out here, and is the reason I submitted this finding. I believe all of the facts have been provided, so let's hear what they have to say.

[WangSecurity]

It's not clear in readme / code comments / docs that the protocol planned on respecting the discussed invariant. Planning to reject the escalation and leave the issue as it is.

[sherlock-admin3]

The Lead Senior Watson signed off on the fix.

[Evert0x]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• nirohgo: rejected