Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

The function `fillOffchainOrders` has wrong check to ensure a maximum or minimum target price

draiakoo

Summary

The function fillOffchainOrders has wrong check to ensure a maximum or minimum target price

Vulnerability Details

The function fillOffchainOrders is used by the keepers to execute orders without creating a market order on chain in order to save gas. This is done by users that sign a bunch of data to ensure the trade and then keepers will automatically execute it onchain. This data is the following:

struct Data {
uint128 tradingAccountId;
uint128 marketId;
int128 sizeDelta;
uint128 targetPrice;
bool shouldIncreaseNonce;
uint120 nonce;
bytes32 salt;
uint8 v;
bytes32 r;
bytes32 s;
}

From these data, the targetPrice is used in order to ensure a minimum price when selling and a maximum price when buying. However, if we look into the code implementation, we can notice the following:

// if the order increases the trading account's position (buy order), the fill price must be less than or
// equal to the target price, if it decreases the trading account's position (sell order), the fill price
// must be greater than or equal to the target price.
ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256())
|| (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256());

Reading the comments it makes sense because it tells that when the user is buying, the price MUST be less than or equal to the target price because it is the maximum price the user is willing to accept. For the counterpart, when the user is selling the price MUST be greater than or equal to the target price because is the minimum price which the user would accept to sell his position.
But looking into the code, the variable ctx.isFillPriceValid gets true when:

  1. Is a buy order AND the targetPrice is less than or equal to the fillPrice

  2. Is a sell order AND the target price is greater than or equal to the fillPrice

As we can see the conditional is wrong for both situations, when it is a buy order the target price should be greater than or equal to the fill price because it is the maximum acceptable price by the user. And when it is a sell order the target price should be less than or equal to the fill price because is the minimum acceptable price set by the user.

Impact

High, the user will only get his order executed as he expected if the fill price matches exactly with the target price he set. However, if the price is within the acceptable threshold set by the user it will just not execute because of this:

// we don't revert here because we want to continue filling other orders.
if (!ctx.isFillPriceValid) {
continue;
}

And if the price is out of the acceptable threshold set by the user it will be executed and the user can lose funds that he did not expect.

Tools Used

Manual review

Recommendations

Correct the conditionals:

- ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256())
- || (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256());
+ ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256())
+ || (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256());
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

fillOffchainOrders inverses the comparison between `fillPrice` and `targetPrice`

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.