Zaros Part 1

Summary

Wrong price validation when filling an off-chain order

Vulnerability Details

Whenever a keeper calls fillOffchainOrders(), he provides price data that is validated using verifyOffchainPrice(). Then, we get bidX18 and askX18 that are used to determine the indexPriceX18 depending on whether the order is a long or a short:

Then, we use that index price to determine the fill price:

Then, we have this line of code and comment above it:

We can see that according to the comment if this is a buy order, then the fill price must be less than or equal to the target price. However, we can see that this is not properly enforced. If this is a buy order, then we check whether the target price is less than or equal to the fill price or in other words, the fill price must be greater or equal to the target price. If that is a sell order, then the fill price must be greater than or equal to the target price. However, we can see that the code actually enforces that the target price is greater than or equal to the fill price or in other words, the fill price must be less than or equal to the target price.

From this, we can safely assume that either the code is wrong or the comments are wrong. However, by using logic, we can safely deduct that the code is wrong. If I am doing a buy order, I benefit from having the lowest price possible. Thus, the target price must serve as a cap and must disallow the keeper from creating a position with a higher price than my target price as this is the highest price I am willing to create a position at. If I am doing a sell order, then I benefit from having the highest price possible. Thus, the target price must be a minimum price I am willing to accept so the fill price should always be greater than it.

Impact

Wrong price validation when filling an off-chain order causing orders with invalid prices to successfully pass but orders with valid prices to fail

Tools Used

Manual Review

Recommendations

Change the signs to properly enforce what the comments are saying