search

sherlock-audit / 2024-09-symmio-v0-8-4-update-contest-judging

0 stars 0 forks source link

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

Open sherlock-admin4 opened 1 week ago

sherlock-admin4 commented 1 week ago

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

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:

MoonKnightDev commented 1 week ago

Normal liquidators can also call the settleAndForceClose functions.