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

Lack of Slippage Protection in `SettlementBranch::fillMarketOrder(...)` might lead to losses

aamirusmani1552

Summary

The SettlementBranch::fillMarketOrder(...) function lacks slippage protection against price shifts.

Vulnerability Details

The SettlementBranch::fillMarketOrder(...) function fetches the price from off-chain reports provided by Chainlink Data providers and uses the provided price without checking for price slippage. For instance, at the time of creating the order, the price of ARB-USD might be $1 USD. However, if the price shifts to $0.8 USD when SettlementBranch::fillMarketOrder(...) is called, the trade will execute at this new price, which might be undesirable for the user.

This protection is implemented in the SettlementBranch::fillOffchainOrder(...) function:

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;
}

GitHub Source (Lines 286-292)

There might also arise a condition when the fill price leads to the decrease in the markPrice and difference between marPrice - lastInteractoinPrice is negative which might cause loss to the user.

Proof of Concept

function test_NoSlippageProtectionInFillMarketOrders() public {
// create account
address token = address(usdc);
int128 tokenDecimals = 6;
uint256 amount = 100000 * 10 ** uint128(tokenDecimals);
SD59x18 sizeDelta = sd59x18(100e18);
​
// minting tokens to naruto
deal({ token: token, to: users.naruto.account, give: amount });
​
///////////////////////////////////////////////////
// Naruto places an order in ETHUSD market ///
///////////////////////////////////////////////////
​
uint128 narutoTrandingAccountId = createAccountAndDeposit(amount, token);
​
// get the market config with market id
uint256[2] memory marketsIdsRange;
marketsIdsRange[0] = 4;
marketsIdsRange[1] = 4;
​
MarketConfig[] memory filteredMarketsConfig = getFilteredMarketsConfig(marketsIdsRange);
​
// create the order
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: narutoTrandingAccountId,
marketId: filteredMarketsConfig[0].marketId,
sizeDelta: int128(sizeDelta.intoInt256())
})
);
​
// skip some time
skip(3 minutes);
​
// keeper fills the order
bytes memory mockSignedReport =
getMockedSignedReport(marketsConfig[4].streamId, 0.8e18);
​
vm.startPrank({ msgSender: marketOrderKeepers[4] });
​
// fill first order and open position
perpsEngine.fillMarketOrder(narutoTrandingAccountId, 4, mockSignedReport);
}

Impact

The trade might be executed at an undesirable price and the trader might incur losses.

Tools Used

  • Manual Review

  • Foundry

Recommendations

It is recommended to include slippage protection in the SettlementBranch::fillMarketOrder(...) function.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

fillMarketOrder lacks slippage protection

Support

FAQs

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