Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xiaoming90 - Inconsistent in the liquidation fee leads to unfairness in liquidation process #52

Open
sherlock-admin4 opened this issue Oct 8, 2024 · 1 comment
Labels
Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin4
Copy link

sherlock-admin4 commented Oct 8, 2024

xiaoming90

High

Inconsistent in the liquidation fee leads to unfairness in liquidation process

Summary

Reserve vault results in unfairness in the liquidation process. PartyA using force close mechanism to liquidate a position will receive more liquidation fees compared to normal liquidators as they have access to the funds in the reserve vault.

This creates unfairness to the protocol's liquidation process and to the existing liquidators, as they will receive lesser liquidation fees than Alice (PartyA). Without a fair and effective liquidation process, the protocol's solvent will be at risk, as bad positions will not be liquidated in a timely manner, and some liquidators might not want to participate in the process due to unfairness in the system.

Root Cause

  • Inconsistent in the liquidation fee given to PartyA and liquidators as PartyA performs liquidation via force close mechanism has access to funds in liquidatee's reserve vault, while normal liquidators do not.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Assume that PartyB's reserve vault has 1000 USD. During force closing, if PartyB's account is underwater, even with the additional support from its reserve vault, PartyB's account will be liquidated, as per Lines 127-138 below.

In Lines 128 and 129, the protocol transfers all the existing funds (1000 USD) from PartyB's reserve vault to PartyB's allocated balance. Thus, before the liquidation process is executed at Line 137 below, the PartyB's allocated balance will increase by 1000 USD.

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/ForceActions/ForceActionsFacetImpl.sol#L127

File: ForceActionsFacetImpl.sol
051: 	function forceClosePosition(
..SNIP..
112: 		require(partyAAvailableBalance >= 0, "PartyAFacet: PartyA will be insolvent");
113: 		if (partyBAvailableBalance >= 0) {
..SNIP..
118: 		} else if (partyBAvailableBalance + int256(reserveAmount) >= 0) {
..SNIP..
127: 		} else { // @audit-info Code block for liquidating position
128: 			accountLayout.reserveVault[quote.partyB] = 0;
129: 			accountLayout.partyBAllocatedBalances[quote.partyB][quote.partyA] += reserveAmount;
130: 			emit SharedEvents.BalanceChangePartyB(quote.partyB, quote.partyA, reserveAmount, SharedEvents.BalanceChangeType.REALIZED_PNL_IN);
131: 			int256 diff = (int256(quote.quantityToClose) * (int256(closePrice) - int256(sig.currentPrice))) / 1e18;
132: 			if (quote.positionType == PositionType.LONG) {
133: 				diff = diff * -1;
134: 			}
135: 			isPartyBLiquidated = true;
136: 			upnlPartyB = sig.upnlPartyB + diff;
137: 			LibLiquidation.liquidatePartyB(quote.partyB, quote.partyA, upnlPartyB, block.timestamp);
138: 		}

As a result, when the LibLiquidation.liquidatePartyB is executed at Line 137 above, the computed availableBalance will be 1000 USD smaller when the liquidation process is triggered via the forceClosePosition function compared to the standard liquidation process that is triggered via LiquidationFacet.liquidatePartyB by the liquidators, as shown below. This can be proven via the formula within partyBAvailableBalanceForLiquidation function.

**Normal scenario without access to reserve vault's funds**
availableBalance = partyBAllocatedBalances - lockedValue + PnL
availableBalance = 1000 - 100 - 2000 = -1100

**Force close with access the reserve vault's funds => partyBAllocatedBalances will 1000 USDC higher (1000 => 2000)**
availableBalance = partyBAllocatedBalances - lockedValue + PnL
availableBalance = 2000 - 100 - 2000 = -100

Assume Alice is entitled to execute the force close function against PartyB, while Bob is a normal liquidator that can only liquidate PartyB via the standard LiquidationFacet.liquidatePartyB function.

When Alice triggers the liquidation process, the availableBalance will be -100 instead of -1000. Thus, when computing the liquidation fee that the liquidator is entitled to at Line 39 below, the computed liquidation fee (remainingLf) will be higher. This means that if Alice triggered the liquidation process, she would receive more liquidation fees compared to Bob because she has access to funds within PartyB's reserve vault. In this case, Alice is the liquidator and will receive the liquidation fee.

As a result, this creates unfairness within the system and to the existing liquidators as they (e.g., Bob) will receive lesser liquidation fees compared to Alice.

https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/libraries/LibLiquidation.sol#L29

File: LibLiquidation.sol
23: 	function liquidatePartyB(address partyB, address partyA, int256 upnlPartyB, uint256 timestamp) internal {
24: 		AccountStorage.Layout storage accountLayout = AccountStorage.layout();
25: 		MAStorage.Layout storage maLayout = MAStorage.layout();
26: 		QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
27: 
28: 		// Calculate available balance for liquidation
29: 		int256 availableBalance = LibAccount.partyBAvailableBalanceForLiquidation(upnlPartyB, partyB, partyA);
30: 
31: 		// Ensure Party B is insolvent
32: 		require(availableBalance < 0, "LiquidationFacet: partyB is solvent");
..SNIP..
37: 		// Determine liquidator share and remaining locked funds
38: 		if (uint256(-availableBalance) < accountLayout.partyBLockedBalances[partyB][partyA].lf) {
39: 			remainingLf = accountLayout.partyBLockedBalances[partyB][partyA].lf - uint256(-availableBalance);
40: 			liquidatorShare = (remainingLf * maLayout.liquidatorShare) / 1e18;
41: 
42: 			maLayout.partyBPositionLiquidatorsShare[partyB][partyA] =
43: 				(remainingLf - liquidatorShare) /
44: 				quoteLayout.partyBPositionsCount[partyB][partyA];
45: 		} else {
46: 			maLayout.partyBPositionLiquidatorsShare[partyB][partyA] = 0;
47: 		}

Impact

This creates unfairness to the protocol's liquidation process and to the existing liquidators, as they will receive lesser liquidation fees than Alice. Without a fair and effective liquidation process, the protocol's solvent will be at risk, as bad positions will not be liquidated in a timely manner, and some liquidators might not want to participate in the process due to unfairness in the system.

PoC

No response

Mitigation

The liquidation process for Alice and existing liquidators should be consistent to ensure fairness. In the above case, consider either of the following solutions:

  • Do not take into consideration the reserve vault's funds during liquidation within the forceClosePosition function; OR
  • Take into consideration the reserve vault's funds within the LiquidationFacet.liquidatePartyB for consistency.
@MoonKnightDev
Copy link

Normal liquidators can also call the settleAndForceClose functions.

@sherlock-admin3 sherlock-admin3 added Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed labels Oct 8, 2024
@sherlock-admin3 sherlock-admin3 changed the title Careful Fiery Scallop - Inconsistent in the liquidation fee leads to unfairness in liquidation process xiaoming90 - Inconsistent in the liquidation fee leads to unfairness in liquidation process Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

3 participants