Limit Price Can Be Bypassed

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketLib.sol#L101

Vulnerability details

Users could have orders executed at prices that were not agreed upon by the limit and stop order parameters that were set. The worst case will lead to users having orders wrongly executed at a loss for the users, and in most cases, users will execute orders at unexpected and unfavorable prices.

The reason this is the case is because the stop limit order check uses && instead of ||. Currently, it will only revert if both the limit order AND the stop loss order are met. This is wrong as it should revert if either check is not met.

Impact

Orders will not be honored leading to potential loss of funds or entering at undesirable price

Proof of Concept

• Alice opens a 1x long position longing ETH with an entry price of $4000
• Alice creates another order to close the position with a stop loss of $3800 and a limit price of $4000
• ETH drops to $3800
• The stop loss is triggered
• Order is attempted to be executed
• validateTrade is called
  • The else if (limitPrice > 0 && stopPrice > 0) is entered
  • validateLimitPrice returns false since limitPrice ($4000) is greater than the tradePrice ($3800)

      if (tradeAmount < 0 && limitPrice > tradePrice) {
          return false;
      }
  

  • validateStopPrice returns true since stopPrice is not lower than oraclePrice

      if (stopPrice < oraclePrice) {
          return false;
      }
  

• The overall validateTrade check will now pass since both validateLimitPrice and validateStopPrice did not return false.
• Order finishes executing, and Alice has lost $200 on an order because the limit price that was set was not properly validated.

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the && to || as follows:

            if (
                !validateLimitPrice(tradePrice, tradeAmount, limitPrice)
                    || !validateStopPrice(oraclePrice, tradePrice, tradeAmount, stopPrice, auctionData)
            ) {
                revert LimitStopOrderDoesNotMatch();
            }

Assessed type

Invalid Validation

[syuhei176]

Orders in PerpMarket, unlike those in CEX, have both limit and stop price attributes in a single order.

For example, if the current price is 1000, you can place a buy order with a limit price of 900 and a stop price of 1100. The order will be executed when either condition is met.

Therefore, the revert condition should be when neither condition is met, so && should be used.

[alex-ppg]

The submission specifies that an order will be considered valid if either its limit or its stop price has been validated when both attributes have been defined for it. Per the Sponsor's comments, this is desirable behavior as the code would effectively permit an order to be executed outside a particular range.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[Tomiwasa0]

Thank you for judging @alex-ppg,

Using the sponsor's example, I believe this is an issue.

Current Market Price (1000)

When a user sets a trade (900) and goes long

his/her take profit region should be above his entry while his stop should always be below his entry price.

limit at 1200
stop limit at 800
is a valid trade.

For stop limit

if (trade amount > 0) {
// buy
if (stopPrice > oraclePrice) {
return false;
}
if (trade price > Bps.upper(oraclePrice, decayedSlippageTorelance)) {
return false;
}

For limit/Take profit

if (trade amount > 0 && limit price < trade price) {
return false;
}

BUT Based on the code's implementation, using the sponsor's example if

current at 1000
entry at 900
limit is at 1200
stop limit at 1100
Even though the stop is invalid the trade order is processed.

Hence if the market goes down to 900 the trade is activated but if the price keeps failing the system fails to trigger a stop limit order because of an invalid stop limit.

Thus the user who has automated his trading system by executing the trade and setting a stop and limit will be unfairly liquidated when the system should have taken him out of the trade at 900 because the stop limit is invalid at 1100.

This action also applies to the short trade example, this MUST revert if any of the Limits are invalid to protect the user because that was why it was added in the first place, everything doesn't always go according to the plan while trading. If you need any more explanation, I would be more than happy to elaborate. Thank you.

[nnez]

Hello @alex-ppg,
I'd also like to weigh in here since my findings #217 is a duplicate of this submission.

First, I think we should establish that the order represents order's maker or user's intent.
For example,

• a user submits a buy order with stop price would translate to the intent that the user wants their order to be executed after the certain price is reached.
• a users submits a buy order with limit price would translate to the intent that the user wants their order to be executed less than the limit price, in other words, get a good deal.

Second, if we take user's intent into account and evaulaute sponsor's example.
A buy order with limit=900 and stop=1100 at a current price at 1000 doesn't make sense in term of user's intent.
Why would the user expect that their order to be executed after the price reaches 1100 but also wants the settled price to be less than 900.

If they want to buy low (price less than 900) and also don't want to miss out if the price doesn't come down and go up pass 1100, it would make more sense to submit two separate orders, a limit order and a stop order.

Third, we should also establish a common understanding of stop-limit order because it represents the intent of users submitting order specifying both limit and stop price at a given point in time.

A simple research on google yields:

• https://corporatefinanceinstitute.com/resources/career-map/sell-side/capital-markets/stop-limit-order/
• https://www.investopedia.com/terms/s/stop-limitorder.asp

Basically, stop-limit order is a stop order that triggers another limit order.
The stop price dictates the price whether the order is triggered, then the limit price dictates the price at which the order is filled.
Meaning that both stop price and limit price should both be valid to be considered a valid trade.

From all above, I think that if we were to reject this issue, it should be on the ground that protocol will not support stop-limit order not that the order attributes can have both limit and stop price and will be executed when either condition is met.

Because if the protocol do support stop-limit order, the logic to validate this order is definitely flawed, and can cause users's fund loss from unintended execution of users' order as this finding and mine describes.

[alex-ppg]

Hey @Tomiwasa0 and @nnez, thank you for your contributions!

While I understand that you may believe the Sponsor's intentions to be counterintuitive, a configuration with a limit at 900 and a stop at 1_100 when the current price is 1_000 effectively instructs the trade to be executed after a fluctuation of 10%. This trade is sensible if the creator of the trade believes the asset to be fairly valued at 1_000, and intends to make a profit in either direction.

I do not see a security vulnerability pointed out, and the code fulfills the Sponsor's intended purpose which can also be considered a sensible approach albeit perhaps rarely supported.

[Tomiwasa0]

Thanks @alex-ppg,

function validateLimitPrice(uint256 trade price, int256 trade amount, uint256 limit price)
internal
pure
returns (bool)

uint256 tradePrice = Math.abs(tradeResult.payoff.perpEntryUpdate + tradeResult.payoff.perpPayoff)
* Constants.Q96 / Math.abs(tradeAmount);

Trades are executed at Trade Price because it contains the perpEntryUpdate which serves as the entry price. limit order use in the protocol is not for entry but exit (Take Profit point) , limit is used for calculating the profit point where the position should be close

if (tradeAmount > 0 && limitPrice < tradePrice) {
return false;
}
limitPrice < tradePrice meaning if where you are taking your profit is below your entry point when you go long it should revert

and stop limit where a user wants to leaves the market with minimum loss, This must always be below the Trade Price(entry) for a valid trade to be executed.

Following the example used, For a trade to be executed below Oracle price, Trade Price(Entry) is the entry point and must be below not the limitprice. Kindly check out the struct for order execution

struct PerpOrder {
OrderInfo info;
uint64 pairId;
address entryTokenAddress;
int256 tradeAmount;
int256 marginAmount;
uint256 takeProfitPrice; // take profit and not entry
uint256 stopLossPrice;
uint64 slippageTolerance;
uint8 leverage;
address validatorAddress;
bytes validationData;
}
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/markets/perp/PerpOrder.sol#L9-L21

struct PerpOrderV3 {
OrderInfo info;
uint64 pairId;
address entryTokenAddress;
string side;
uint256 quantity; // same as trade amount, name change but the are still acting the same way
uint256 marginAmount;
uint256 limitPrice; // take profit and not entry
uint256 stopPrice;
uint8 leverage;
bool reduceOnly;
bool closePosition;
bytes auctionData;
}
https://github.com/code-423n4/2024-05-predy/blob/a9246db5f874a91fb71c296aac6a66902289306a/src/markets/perp/PerpOrderV3.sol#L9-L21

[alex-ppg]

Hey @Tomiwasa0, thank you for your contribution. I strongly advise adhering to PJQA guidelines so as to avoid risking your SR access.

I do not see any new information introduced in your reply and will retain my original ruling. No further feedback is expected for this submission.