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

Offchain orders are not cancelled after the account has been liquidated

greed

Summary

In the previous version of Zaros, users were only allowed to create on-chain orders using the OrderBranch::createMarketOrder() function.

This would create a pending order that keepers would be able to fill later on.

In case the account is liquidated before the order is filled, the order is cancelled to prevent the account from having a position they did not intend to have after their liquidation.

This is done through the following line in LiquidationBranch::liquidateAccounts()

L164: MarketOrder.load(ctx.tradingAccountId).clear();

Vulnerability Details

In this new version Zaros has introduced off-chain orders that users sign on the frontend and the logic for filling them has been added to SettlementBranch::fillOffchainOrders()

However, upon liquidation, these orders are not cancelled like the pending market orders are.

Impact

This can negatively impact users and result in the users unintentionally opening a new position (and subtracting fees from their collateral) while they would not want after being liquidated.

The following coded PoC demonstrates a scenario in which a user has a long position and wants to reduce it by supplying a negative delta using off-chain orders. However the price of the longed asset crashes and keepers liquidate his position. After the liquidation, keepers fulfill his pending off-chain order, basically creating a new short position which the user did not intend to.

The test can be pasted in test\integration\perpetuals\settlement-branch\fillOffchainOrders\fillOffchainOrders.t.sol

/* @audit-info need to import :
import { UD60x18, ud60x18 } from "@prb-math/UD60x18.sol";
*/
function testOffchainOrdersNotCancelled() external {
uint256 ethBalance = 1e18;
int128 tradeSize = 10e18;
uint128 marketId = ETH_USD_MARKET_ID;
uint256 price = MOCK_ETH_USD_PRICE;
​
deal({ token: address(wEth), to: users.naruto.account, give: ethBalance });
​
vm.startPrank(users.naruto.account);
uint128 tradingAccountId = createAccountAndDeposit(ethBalance, address(wEth));
​
changePrank({ msgSender: users.naruto.account });
openManualPosition(marketId, ETH_USD_STREAM_ID, price, tradingAccountId, tradeSize);
​
price = MOCK_ETH_USD_PRICE - 100e18;
updateMockPriceFeed(marketId, price);
​
// account is not yet liquidatable
uint128[] memory liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
assertEq(1, liquidatableAccountsIds.length);
assertEq(liquidatableAccountsIds[0], 0);
​
// user wants to reduce his exposure
changePrank({ msgSender: users.naruto.account });
tradeSize = -5e18;
​
uint128 markPrice = perpsEngine.getMarkPrice(
marketId, price, tradeSize
).intoUint128();
​
bytes32 salt = bytes32(block.prevrandao);
bytes32 structHash = keccak256(
abi.encode(
Constants.CREATE_OFFCHAIN_ORDER_TYPEHASH,
tradingAccountId,
marketId,
tradeSize,
markPrice,
false,
uint120(0),
salt
)
);
​
// user signs and "sends" the order to Zaros
bytes32 digest = keccak256(abi.encodePacked("\x19\x01", perpsEngine.DOMAIN_SEPARATOR(), structHash));
(uint8 v, bytes32 r, bytes32 s) = vm.sign({ privateKey: users.naruto.privateKey, digest: digest });
​
// price crashes until to a point where account is liquidatable
price = MOCK_ETH_USD_PRICE / 2;
updateMockPriceFeed(ETH_USD_MARKET_ID, price);
​
// account is liquidatable
liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
assertEq(1, liquidatableAccountsIds.length);
assertEq(liquidatableAccountsIds[0], 1);
​
// account is liquidated
changePrank({ msgSender: liquidationKeeper });
perpsEngine.liquidateAccounts(liquidatableAccountsIds);
​
// keepers execute the off-chain order
// here we reconstruct the off-chain order of the user with its wanted values
OffchainOrder.Data[] memory offchainOrders = new OffchainOrder.Data[](1);
offchainOrders[0] = OffchainOrder.Data({
tradingAccountId: tradingAccountId,
marketId: marketId,
sizeDelta: tradeSize,
targetPrice: markPrice,
shouldIncreaseNonce: false,
nonce: 0,
salt: salt,
v: v,
r: r,
s: s
});
​
bytes memory mockSignedReport = getMockedSignedReport(ETH_USD_STREAM_ID, price);
changePrank({ msgSender: OFFCHAIN_ORDERS_KEEPER_ADDRESS });
perpsEngine.fillOffchainOrders(marketId, offchainOrders, mockSignedReport);
​
// it should fill the offchain order
bool hasOffchainOrderBeenFilled = TradingAccountHarness(address(perpsEngine))
.workaround_hasOffchainOrderBeenFilled(tradingAccountId, structHash);
assertTrue(hasOffchainOrderBeenFilled, "hasOffchainOrderBeenFilled");
​
// account is not liquidatable anymore
liquidatableAccountsIds = perpsEngine.checkLiquidatableAccounts(0, 1);
assertEq(1, liquidatableAccountsIds.length);
assertEq(liquidatableAccountsIds[0], 0);
​
// the account maintenance margin is not 0 meaning it has an opened position
(,,UD60x18 maintenanceMarginUsdX18,) = perpsEngine.getAccountMarginBreakdown(tradingAccountId);
assertNotEq(maintenanceMarginUsdX18.intoUint256(), 0);
}

Tools Used

Manual review

Recommendations

Just like classic on-chain market orders are cancelled after liquidation, off-chain orders should also be cancelled.

Updates

Lead Judging Commences

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

`LiquidationBranch.liquidateAccounts` should cancel off-chain orders of the liquidated account.

Appeal created

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

`LiquidationBranch.liquidateAccounts` should cancel off-chain orders of the liquidated account.

Support

FAQs

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

Give us feedback!