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

Validity price of offchain orders is incorrectly checked, leading to loss of funds and DoS

n0kto

Description

The function SettlementBranch::fillOffchainOrders is invoked by the Chainlink automation keeper to fulfill offchain orders. This function validates the price. As explained in the comment above this validation, the fill price should be less than or equal to the target price when buying, to avoid paying more than expected. Conversely, the fill price should be more than or equal to the target price when selling to prevent selling your tokens at a very low price. However, the condition checks the exact opposite and fills the offchain orders only when the limits are exceeded!

This leads users to buy only when the price is higher than their target, and sell when the price is lower than their target. This results in an unexpected loss of funds and a DoS of all legitimate orders (when the fill price is correct for users).

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
...
for (uint256 i; i < offchainOrders.length; i++) {
...
@> // 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());
​
// we don't revert here because we want to continue filling other orders.
if (!ctx.isFillPriceValid) {
continue;
}
...
}
}

Risk

Likelyhood: High

  • This will occur on every offchain order.

Impact: High

  • Orders are executed only if they exceed the target price, leading to financial loss for users.

  • Revert when the fill price is valid.

Recommended Mitigation

function fillOffchainOrders(
uint128 marketId,
OffchainOrder.Data[] calldata offchainOrders,
bytes calldata priceData
)
external
onlyOffchainOrdersKeeper(marketId)
{
...
for (uint256 i; i < offchainOrders.length; i++) {
...
// 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());
+ ctx.isFillPriceValid = (ctx.isBuyOrder && ctx.offchainOrder.targetPrice >= ctx.fillPriceX18.intoUint256())
+ || (!ctx.isBuyOrder && ctx.offchainOrder.targetPrice <= ctx.fillPriceX18.intoUint256());
​
// we don't revert here because we want to continue filling other orders.
if (!ctx.isFillPriceValid) {
continue;
}
...
}
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 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.