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

User can withdraw all collateral when a position has enough profit so if liquidated no collateral can be deducted

Summary

In TradingAccountBranch::withdrawMargin, it is checked that the margin balance is greater than the required initial margin once collateral has been withdrawn:
https://github.com/Cyfrin/2024-07-zaros/blob/69ccf428b745058bea08804b3f3d961d31406ba8/src/perpetuals/branches/TradingAccountBranch.sol#L382-L392
If a user has a position with enough profit (i.e. unrealized PnL is greater than the required maintenance margin), this user can withdraw all collateral. If, later, that position becomes liquidatable, this user has no collateral so none can be deducted.

Vulnerability Details

Let's assume a user opens a long position of 1 contract at 3,000 of ETHUSD. Let's also assume the maintenance margin rate required is 5%:
Position value: 1 * 3,000 = 3,000
Maintenance margin: 3,000 * 5% = 150
As soon as this position is in profit above 150, the user can withdraw all collateral. With zero collateral, if the position`s profit fall below 150, it will get liquidated (position will get closed) but no collateral will be deducted.

Impact

In this PoC, a position is created, when enough profit is accrued, collateral is withdrawn. Later, price is updated so the position becomes liquidatable and it is liquidated.
Add this test into liquidateAccounts.t.sol:

function test_All_Collateral_Withdrawn() external {
uint256 amountToDeposit = 100e18;
deal({ token: address(wstEth), to: users.naruto.account, give: amountToDeposit });
uint128 tradingAccountId = createAccountAndDeposit(amountToDeposit, address(wstEth));
uint128 marketId = 0;
int128 amount = 10e18;
MarketConfig memory fuzzMarketConfig = getFuzzMarketConfig(marketId);
perpsEngine.createMarketOrder(
OrderBranch.CreateMarketOrderParams(tradingAccountId, fuzzMarketConfig.marketId, amount)
);
bytes memory mockSignedReport =
getMockedSignedReport(fuzzMarketConfig.streamId, fuzzMarketConfig.mockUsdPrice);
address marketOrderKeeper = marketOrderKeepers[fuzzMarketConfig.marketId];
changePrank({ msgSender: marketOrderKeeper });
perpsEngine.fillMarketOrder(tradingAccountId, fuzzMarketConfig.marketId, mockSignedReport);
//Price updated to make the position with enough profit so collateral can be withdrawn
updateMockPriceFeed(uint128(fuzzMarketConfig.marketId), 2e23);
// it should transfer the withdrawn amount to the sender
changePrank({ msgSender: users.naruto.account });
uint256 newMarginCollateralBalance = convertUd60x18ToTokenAmount(
address(wstEth), perpsEngine.getAccountMarginCollateralBalance(tradingAccountId, address(wstEth))
);
perpsEngine.withdrawMargin(tradingAccountId, address(wstEth), newMarginCollateralBalance);
//Price updated to make the position liquidatable
updateMockPriceFeed(uint128(fuzzMarketConfig.marketId), 8e22);
uint128[] memory accountsIds;
accountsIds = new uint128[](1);
accountsIds[0] = tradingAccountId;
changePrank({ msgSender: liquidationKeeper });
perpsEngine.liquidateAccounts({ accountsIds: accountsIds });
}

As can be seen in the event emitted, the liquidatedCollateralUsd is zero even though the requiredMaintenanceMarginUsd is 4e21and liquidationFeeUsd is 5e18

LogLiquidateAccount(keeper: ERC1967Proxy: [0x50795785161296B0A64914b4ee9285bAF70371Cd], tradingAccountId: 1, amountOfOpenPositions: 1,
requiredMaintenanceMarginUsd: 4000002000000000000000 [4e21], marginBalanceUsd: -200000100000000000000000 [-2e23],
liquidatedCollateralUsd: 0, liquidationFeeUsd: 5000000000000000000 [5e18])

Tools Used

Foundry

Recommendations

A minimum amount of collateral (e.g. required initial margin) should always remain in the user account as long as he has got open positions.

Updates

Lead Judging Commences

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

A user with a big enough positive PnL can withdraw his margin and avoid liquidation fee in case they get liquidated

Appeal created

blckhv Auditor
12 months ago
0xrststn Submitter
12 months ago
blckhv Auditor
12 months ago
0xrststn Submitter
12 months ago
blckhv Auditor
12 months ago
0xrststn Submitter
12 months ago
blckhv Auditor
12 months ago
draiakoo Auditor
12 months ago
blckhv Auditor
12 months ago
draiakoo Auditor
12 months ago
blckhv Auditor
12 months ago
0xrststn Submitter
12 months ago
inallhonesty Lead Judge
12 months ago
inallhonesty Lead Judge
11 months ago
inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

A user with a big enough positive PnL can withdraw his margin and avoid liquidation fee in case they get liquidated

Support

FAQs

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