xiaoming90 - Losses of some long traders can eat into the margins of others

[sherlock-admin2]

xiaoming90

high

Losses of some long traders can eat into the margins of others

Summary

The losses of some long traders can eat into the margins of others, resulting in those affected long traders being unable to withdraw their margin and profits, leading to a loss of assets for the long traders.

Vulnerability Detail

At $T0$, the current price of ETH is \$1000 and assume the following state:

Alice's Long Position 1	Bob's Long Position 2	Charles (LP)
Position Size = 6 ETH Margin = 3 ETH Last Price (entry price) = \$1000	Position Size = 6 ETH Margin = 5 ETH Last Price (entry price) = \$1000	Deposited 12 ETH

• The stableCollateralTotal will be 12 ETH
• The GlobalPositions.marginDepositedTotal will be 8 ETH (3 + 5)
• The globalPosition.sizeOpenedTotal will be 12 ETH (6 + 6)
• The total balance of ETH in the vault is 20 ETH.

As this is a perfectly hedged market, the accrued fee will be zero, and ignored in this report for simplicity's sake.

At $T1$, the price of the ETH drops from \$1000 to \$600. At this point, the settle margin of both long positions will be as follows:

Alice's Long Position 1	Bob's Long Position 2
priceShift = Current Price - Last Price = \$600 - \$1000 = -\$400 PnL = (Position Size priceShift) / Current Price = (6 ETH -\$400) / \$400 = -4 ETH settleMargin = marginDeposited + PnL = 3 ETH + (-4 ETH) = -1 ETH	PnL = -4 ETH (Same calculation) settleMargin = marginDeposited + PnL = 5 ETH + (-4 ETH) = 1 ETH

Alice's long position is underwater (settleMargin < 0), so it can be liquidated. When the liquidation is triggered, it will internally call the updateGlobalPositionData function. Even if the liquidation does not occur, any of the following actions will also trigger the updateGlobalPositionData function internally:

• executeOpen
• executeAdjust
• executeClose

The purpose of the updateGlobalPositionData function is to update the global position data. This includes getting the total profit loss of all long traders (Alice & Bob), and updating the margin deposited total + stable collateral total accordingly.

Assume that the updateGlobalPositionData function is triggered by one of the above-mentioned functions. Line 179 below will compute the total PnL of all the opened long positions.

priceShift = current price - last price
priceShift = $600 - $1000 = -$400

profitLossTotal = (globalPosition.sizeOpenedTotal * priceShift) / current price
profitLossTotal = (12 ETH * -$400) / $600
profitLossTotal = -8 ETH

The profitLossTotal is -8 ETH. This is aligned with what we have calculated earlier, where Alice's PnL is -4 ETH and Bob's PnL is -4 ETH (total = -8 ETH loss).

At Line 184 below, the newMarginDepositedTotal will be set to as follows (ignoring the _marginDelta for simplicity's sake)

newMarginDepositedTotal = _globalPositions.marginDepositedTotal + _marginDelta + profitLossTotal
newMarginDepositedTotal = 8 ETH + 0 + (-8 ETH) = 0 ETH

What happened above is that 8 ETH collateral is deducted from the long traders and transferred to LP. When newMarginDepositedTotal is zero, this means that the long trader no longer owns any collateral. This is incorrect, as Bob's position should still contribute 1 ETH remaining margin to the long trader's pool.

Let's review Alice's Long Position 1: Her position's settled margin is -1 ETH. When the settled margin is -ve then the LPs have to bear the cost of loss per the comment here. However, in this case, we can see that it is Bob (long trader) instead of LPs who are bearing the cost of Alice's loss, which is incorrect.

Let's review Bob's Long Position 2: His position's settled margin is 1 ETH. If his position's liquidation margin is $LM$, Bob should be able to withdraw $1\ ETH - LM$ of his position's margin. However, in this case, the marginDepositedTotal is already zero, so there is no more collateral left on the long trader pool for Bob to withdraw, which is incorrect.

With the current implementation, the losses of some long traders can eat into the margins of others, resulting in those affected long traders being unable to withdraw their margin and profits.

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L173

being File: FlatcoinVault.sol
173:     function updateGlobalPositionData(
174:         uint256 _price,
175:         int256 _marginDelta,
176:         int256 _additionalSizeDelta
177:     ) external onlyAuthorizedModule {
178:         // Get the total profit loss and update the margin deposited total.
179:         int256 profitLossTotal = PerpMath._profitLossTotal({globalPosition: _globalPositions, price: _price});
180: 
181:         // Note that technically, even the funding fees should be accounted for when computing the margin deposited total.
182:         // However, since the funding fees are settled at the same time as the global position data is updated,
183:         // we can ignore the funding fees here.
184:         int256 newMarginDepositedTotal = int256(_globalPositions.marginDepositedTotal) + _marginDelta + profitLossTotal;
185: 
186:         // Check that the sum of margin of all the leverage traders is not negative.
187:         // Rounding errors shouldn't result in a negative margin deposited total given that
188:         // we are rounding down the profit loss of the position.
189:         // If anything, after closing the last position in the system, the `marginDepositedTotal` should can be positive.
190:         // The margin may be negative if liquidations are not happening in a timely manner.
191:         if (newMarginDepositedTotal < 0) {
192:             revert FlatcoinErrors.InsufficientGlobalMargin();
193:         }
194: 
195:         _globalPositions = FlatcoinStructs.GlobalPositions({
196:             marginDepositedTotal: uint256(newMarginDepositedTotal),
197:             sizeOpenedTotal: (int256(_globalPositions.sizeOpenedTotal) + _additionalSizeDelta).toUint256(),
198:             lastPrice: _price
199:         });
200: 
201:         // Profit loss of leverage traders has to be accounted for by adjusting the stable collateral total.
202:         // Note that technically, even the funding fees should be accounted for when computing the stable collateral total.
203:         // However, since the funding fees are settled at the same time as the global position data is updated,
204:         // we can ignore the funding fees here
205:         _updateStableCollateralTotal(-profitLossTotal);
206:     }

Impact

Loss of assets for the long traders as the losses of some long traders can eat into the margins of others, resulting in those affected long traders being unable to withdraw their margin and profits.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/FlatcoinVault.sol#L173

Tool used

Manual Review

Recommendation

The following are the two issues identified earlier and the recommended fixes:

Issue 1

Let's review Alice's Long Position 1: Her position's settled margin is -1 ETH. When the settled margin is -ve then the LPs have to bear the cost of loss per the comment here. However, in this case, we can see that it is Bob (long trader) instead of LPs who are bearing the cost of Alice's loss, which is incorrect.

Fix: Alice -1 ETH loss should be borne by the LP, not the long traders. The stable collateral total of LP should be deducted by 1 ETH to bear the cost of the loss.

Issue 2

Let's review Bob's Long Position 2: His position's settled margin is 1 ETH. If his position's liquidation margin is $LM$, Bob should be able to withdraw $1\ ETH - LM$ of his position's margin. However, in this case, the marginDepositedTotal is already zero, so there is no more collateral left on the long trader pool for Bob to withdraw, which is incorrect.

Fix: Bob should be able to withdraw $1\ ETH - LM$ of his position's margin regardless of the PnL of other long traders. Bob's margin should be isolated from Alice's loss.

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: high(7)

[0xLogos]

Escalate

Invalid (maybe medium if I'm missing something, clearly not high)

Isn't described scenario is just a speculation? Why Alice's long position was not liquidated earlier? Even if price dropped that significant in ~1 minute there is still enough time to liquidate.

[sherlock-admin2]

Escalate

Invalid (maybe medium if I'm missing something, clearly not high)

Isn't described scenario is just a speculation? Why Alice's long position was not liquidated earlier? Even if price dropped that significant in ~1 minute there is still enough time to liquidate.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[securitygrid]

The judge should consider whether the scenario or the assumed system state (the value of some variables) described in a report will actually occur or not. This is important.

The scenario described in this report is unlikely to occur. Because Alice's position should have been liquidated before T1, why did it have to wait until bad debts occurred before it was liquidated? If the protocol has only one keeper, then such a scenario may occur when the keeper goes down. However, the protocol has multiple keepers: from third parties and from the protocol itself. It is impossible for all keepers going down.

[xiaoming9090]

The issue stands valid and remains high as it leads to a loss of assets for the long traders, as described in the "impact" section of the report.

The scenario is a valid example to demonstrate the issue on hand. The points related to the timing of the liquidation in the escalation are irrelevant, as the bugs will be triggered when liquidation is executed.

Also, we cannot expect the liquidator keeper always to liquidate the accounts before the settled margin falls below zero (bad debt) under all circumstances due to many reasons (e.g., the price dropped too fast, delay due to too many TX queues at sequencer, time delay between the oracle and market price)

The scenario where the settled margin falls below zero (bad debt) is absolutely something that will happen in the real world. In the code of the liquidation function here, even the protocol expected that the settled margin could fall below zero (bad debt) under some conditions and implement logic to handle this scenario.

// If the settled margin is -ve then the LPs have to bear the cost.
// Adjust the stable collateral total to account for user's profit/loss and the negative margin.
// Note: We are adding `settledMargin` and `profitLoss` instead of subtracting because of their sign (which will be -ve).
vault.updateStableCollateralTotal(settledMargin - positionSummary.profitLoss);

#

[Czar102]

Would like sponsors to comment on this issue @D-Ig @itsermin @rashtrakoff – is it unintended? @nevillehuang any thoughts?

Given that this occurs on accrual of bad debt, I think it should be classified at most as a Medium severity issue.

[rashtrakoff]

The old math (that is math being used in the audit version of contracts) had this side effect. The new math fixes this along with other issues.

[Czar102]

Thank you @rashtrakoff.

Planning to accept the escalation and consider this a Medium severity issue.

[nevillehuang]

Thank you @rashtrakoff.

Planning to accept the escalation and consider this a Medium severity issue.

Agree to downgrade to medium severity

[Czar102]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xLogos: accepted

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/dhedge/flatcoin-v1/pull/266.

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.