Incorrect handling of PnL during liquidation
The incorrect handling of PnL during liquidation led to an error in the protocol's accounting mechanism, which might result in various issues, such as the loss of assets and the stable collateral total being inflated.\\nFirst Example\\nAssume a long position with the following state:\\nMargin Deposited = +20\\nAccrued Funding = -100\\nProfit & Loss (PnL) = +100\\nLiquidation Margin = 30\\nLiquidation Fee = 25\\nSettled Margin = Margin Deposited + Accrued Funding + PnL = 20\\nLet the current `StableCollateralTotal` be $x$ and `marginDepositedTotal` be $y$ at the start of the liquidation.\\nFirstly, the `settleFundingFees()` function will be executed at the start of the liquidation process. The effect of the `settleFundingFees()` function is shown below. The long trader's `marginDepositedTotal` will be reduced by 100, while the LP's `stableCollateralTotal` will increase by 100.\\n```\\nsettleFundingFees() = Short/LP need to pay Long 100\\n\\nmarginDepositedTotal = marginDepositedTotal + funding fee\\nmarginDepositedTotal = y + (-100) = (y - 100)\\n\\nstableCollateralTotal = x + (-(-100)) = (x + 100)\\n```\\n\\nSince the position's settle margin is below the liquidation margin, the position will be liquidated.\\nAt Line 109, the condition `(settledMargin > 0)` will be evaluated as `True`. At Line 123:\\n```\\nif (uint256(settledMargin) > expectedLiquidationFee)\\nif (+20 > +25) => False\\nliquidatorFee = settledMargin\\nliquidatorFee = +20\\n```\\n\\nThe `liquidationFee` will be to +20 at Line 127 below. This basically means that all the remaining margin of 20 will be given to the liquidator, and there should be no remaining margin for the LPs.\\nAt Line 133 below, the `vault.updateStableCollateralTotal` function will be executed:\\n```\\nvault.updateStableCollateralTotal(remainingMargin - positionSummary.profitLoss);\\nvault.updateStableCollateralTotal(0 - (+100));\\nvault.updateStableCollateralTotal(-100);\\n\\nstableCollateralTotal = (x + 100) - 100 = x\\n```\\n\\nWhen `vault.updateStableCollateralTotal` is set to `-100`, `stableCollateralTotal` is equal to $x$.\\n```\\nFile: LiquidationModule.sol\\n    function liquidate(uint256 tokenId) public nonReentrant whenNotPaused liquidationInvariantChecks(vault, tokenId) {\\n..SNIP..\\n        // Check that the total margin deposited by the long traders is not -ve.\\n        // To get this amount, we will have to account for the PnL and funding fees accrued.\\n        int256 settledMargin = positionSummary.marginAfterSettlement;\\n\\n        uint256 liquidatorFee;\\n\\n        // If the settled margin is greater than 0, send a portion (or all) of the margin to the liquidator and LPs.\\n        if (settledMargin > 0) {\\n            // Calculate the liquidation fees to be sent to the caller.\\n            uint256 expectedLiquidationFee = PerpMath._liquidationFee(\\n                position.additionalSize,\\n                liquidationFeeRatio,\\n                liquidationFeeLowerBound,\\n                liquidationFeeUpperBound,\\n                currentPrice\\n            );\\n\\n            uint256 remainingMargin;\\n\\n            // Calculate the remaining margin after accounting for liquidation fees.\\n            // If the settled margin is less than the liquidation fee, then the liquidator fee is the settled margin.\\n            if (uint256(settledMargin) > expectedLiquidationFee) {\\n                liquidatorFee = expectedLiquidationFee;\\n                remainingMargin = uint256(settledMargin) - expectedLiquidationFee;\\n            } else {\\n                liquidatorFee = uint256(settledMargin);\\n            }\\n\\n            // Adjust the stable collateral total to account for user's remaining margin.\\n            // If the remaining margin is greater than 0, this goes to the LPs.\\n            // Note that {`remainingMargin` - `profitLoss`} is the same as {`marginDeposited` + `accruedFunding`}.\\n            vault.updateStableCollateralTotal(int256(remainingMargin) - positionSummary.profitLoss);\\n\\n            // Send the liquidator fee to the caller of the function.\\n            // If the liquidation fee is greater than the remaining margin, then send the remaining margin.\\n            vault.sendCollateral(msg.sender, liquidatorFee);\\n        } else {\\n            // If the settled margin is -ve then the LPs have to bear the cost.\\n            // Adjust the stable collateral total to account for user's profit/loss and the negative margin.\\n            // Note: We are adding `settledMargin` and `profitLoss` instead of subtracting because of their sign (which will be -ve).\\n            vault.updateStableCollateralTotal(settledMargin - positionSummary.profitLoss);\\n        }\\n```\\n\\nNext, the `vault.updateGlobalPositionData` function here will be executed.\\n```\\nvault.updateGlobalPositionData({marginDelta: -(position.marginDeposited + positionSummary.accruedFunding)})\\nvault.updateGlobalPositionData({marginDelta: -(20 + (-100))})\\nvault.updateGlobalPositionData({marginDelta: 80})\\n\\nprofitLossTotal = 100\\nnewMarginDepositedTotal = globalPositions.marginDepositedTotal + marginDelta + profitLossTotal\\nnewMarginDepositedTotal = (y - 100) + 80 + 100 = (y + 80)\\n\\nstableCollateralTotal = stableCollateralTotal + -PnL\\nstableCollateralTotal = x + (-100) = (x - 100)\\n```\\n\\nThe final `newMarginDepositedTotal` is $y + 80$ and `stableCollateralTotal` is $x -100$, which is incorrect. In this scenario\\nThere is no remaining margin for the LPs, as all the remaining margin has been sent to the liquidator as a fee. The remaining margin (settled margin) is also not negative. Thus, there should not be any loss on the `stableCollateralTotal`. The correct final `stableCollateralTotal` should be $x$.\\nThe final `newMarginDepositedTotal` is $y + 80$, which is incorrect as this indicates that the long trader's pool has gained 80 ETH, which should not be the case when a long position is being liquidated.\\nSecond Example\\nThe current price of rETH is $1000.\\nLet's say there is a user A (Alice) who makes a deposit of 5 rETH as collateral for LP.\\nLet's say another user, Bob (B), comes up, deposits 2 rETH as a margin, and creates a position with a size of 5 rETH, basically creating a perfectly hedged market. Since this is a perfectly hedged market, the accrued funding fee will be zero for the context of this example.\\nTotal collateral in the system = 5 rETH + 2 rETH = 7 rETH\\nAfter some time, the price of rETH drop to $500. As a result, Bob's position is liquidated as its settled margin is less than zero.\\n$$ settleMargin = 2\\ rETH + \\frac{5 \\times (500 - 1000)}{500} = 2\\ rETH - 5\\ rETH = -3\\ rETH $$\\nDuring the liquidation, the following code is executed to update the LP's stable collateral total:\\n```\\nvault.updateStableCollateralTotal(settledMargin - positionSummary.profitLoss);\\nvault.updateStableCollateralTotal(-3 rETH - (-5 rETH));\\nvault.updateStableCollateralTotal(+2);\\n```\\n\\nLP's stable collateral total increased by 2 rETH.\\nSubsequently, the `updateGlobalPositionData` function will be executed.\\n```\\nFile: LiquidationModule.sol\\n    function liquidate(uint256 tokenId) public nonReentrant whenNotPaused liquidationInvariantChecks(vault, tokenId) {\\n..SNIP..\\n        vault.updateGlobalPositionData({\\n            price: position.lastPrice,\\n            marginDelta: -(int256(position.marginDeposited) + positionSummary.accruedFunding),\\n            additionalSizeDelta: -int256(position.additionalSize) // Since position is being closed, additionalSizeDelta should be negative.\\n        });\\n```\\n\\nWithin the `updateGlobalPositionData` function, the `profitLossTotal` at Line 179 will be -5 rETH. This means that the long trader (Bob) has lost 5 rETH.\\nAt Line 205 below, the PnL of the long traders (-5 rETH) will be transferred to the LP's stable collateral total. In this case, the LPs gain 5 rETH.\\nNote that the LP's stable collateral total has been increased by 2 rETH earlier and now we are increasing it by 5 rETH again. Thus, the total gain by LPs is 7 rETH. If we add 7 rETH to the original stable collateral total, it will be 7 rETH + 5 rETH = 12 rETH. However, this is incorrect because we only have 7 rETH collateral within the system, as shown at the start.\\n```\\nFile: FlatcoinVault.sol\\n    function updateGlobalPositionData(\\n        uint256 _price,\\n        int256 _marginDelta,\\n        int256 _additionalSizeDelta\\n    ) external onlyAuthorizedModule {\\n        // Get the total profit loss and update the margin deposited total.\\n        int256 profitLossTotal = PerpMath._profitLossTotal({globalPosition: _globalPositions, price: _price});\\n\\n        // Note that technically, even the funding fees should be accounted for when computing the margin deposited total.\\n        // However, since the funding fees are settled at the same time as the global position data is updated,\\n        // we can ignore the funding fees here.\\n        int256 newMarginDepositedTotal = int256(_globalPositions.marginDepositedTotal) + _marginDelta + profitLossTotal;\\n\\n        // Check that the sum of margin of all the leverage traders is not negative.\\n        // Rounding errors shouldn't result in a negative margin deposited total given that\\n        // we are rounding down the profit loss of the position.\\n        // If anything, after closing the last position in the system, the `marginDepositedTotal` should can be positive.\\n        // The margin may be negative if liquidations are not happening in a timely manner.\\n        if (newMarginDepositedTotal < 0) {\\n            revert FlatcoinErrors.InsufficientGlobalMargin();\\n        }\\n\\n        _globalPositions = FlatcoinStructs.GlobalPositions({\\n            marginDepositedTotal: uint256(newMarginDepositedTotal),\\n            sizeOpenedTotal: (int256(_globalPositions.sizeOpenedTotal) + _additionalSizeDelta).toUint256(),\\n            lastPrice: _price\\n        });\\n\\n        // Profit loss of leverage traders has to be accounted for by adjusting the stable collateral total.\\n        // Note that technically, even the funding fees should be accounted for when computing the stable collateral total.\\n        // However, since the funding fees are settled at the same time as the global position data is updated,\\n        // we can ignore the funding fees here\\n        _updateStableCollateralTotal(-profitLossTotal);\\n    }\\n```\\n\\nThird Example\\nAt $T0$, the marginDepositedTotal = 70 ETH, stableCollateralTotal = 100 ETH, vault's balance = 170 ETH\\nBob's Long Position Alice (LP)\\nMargin = 70 ETH\\nPosition Size = 500 ETH\\nLeverage = (500 + 20) / 20 = 26x\\nLiquidation Fee = 50 ETH\\nLiquidation Margin = 60 ETH\\nEntry Price = $1000 per ETH Deposited = 100 ETH\\nAt $T1$, the position's settled margin falls to 60 ETH (margin = +70, accrued fee = -5, PnL = -5) and is subjected to liquidation.\\nFirstly, the `settleFundingFees()` function will be executed at the start of the liquidation process. The effect of the `settleFundingFees()` function is shown below. The long trader's `marginDepositedTotal` will be reduced by 5, while the LP's `stableCollateralTotal` will increase by 5.\\n```\\nsettleFundingFees() = Long need to pay short 5\\n\\nmarginDepositedTotal = marginDepositedTotal + funding fee\\nmarginDepositedTotal = 70 + (-5) = 65\\n\\nstableCollateralTotal = 100 + (-(-5)) = 105\\n```\\n\\nNext, this part of the code will be executed to send a portion of the liquidated position's margin to the liquidator and LPs.\\n```\\nsettledMargin > 0 => True\\n(settledMargin > expectedLiquidationFee) => (+60 > +50) => True\\nremainingMargin = uint256(settledMargin) - expectedLiquidationFee = 60 - 50 = 10\\n```\\n\\n50 ETH will be sent to the liquidator and the remaining 10 ETH should goes to the LPs.\\n```\\nvault.updateStableCollateralTotal(remainingMargin - positionSummary.profitLoss) =>\\nstableCollateralTotal = 105 ETH + (remaining margin - PnL)\\nstableCollateralTotal = 105 ETH + (10 ETH - (-5 ETH))\\nstableCollateralTotal = 105 ETH + (15 ETH) = 120 ETH\\n```\\n\\nNext, the `vault.updateGlobalPositionData` function here will be executed.\\n```\\nvault.updateGlobalPositionData({marginDelta: -(position.marginDeposited + positionSummary.accruedFunding)})\\nvault.updateGlobalPositionData({marginDelta: -(70 + (-5))})\\nvault.updateGlobalPositionData({marginDelta: -65})\\n\\nprofitLossTotal = -5\\nnewMarginDepositedTotal = globalPositions.marginDepositedTotal + marginDelta + profitLossTotal\\nnewMarginDepositedTotal = 70 + (-65) + (-5) = 0\\n\\nstableCollateralTotal = stableCollateralTotal + -PnL\\nstableCollateralTotal = 120 + (-(5)) = 125\\n```\\n\\nThe reason why the profitLossTotal = -5 is because there is only one (1) position in the system. So, this loss actually comes from the loss of Bob's position.\\nThe `newMarginDepositedTotal = 0` is correct. This is because the system only has 1 position, which is Bob's position; once the position is liquidated, there should be no margin deposited left in the system.\\nHowever, `stableCollateralTotal = 125` is incorrect. Because the vault's collateral balance now is 170 - 50 (send to liquidator) = 120. Thus, the tracked balance and actual collateral balance are not in sync.
To remediate the issue, the `profitLossTotal` should be excluded within the `updateGlobalPositionData` function during liquidation.\\n```\\n// Remove the line below\\n profitLossTotal = PerpMath._profitLossTotal(// rest of code)\\n\\n// Remove the line below\\n newMarginDepositedTotal = globalPositions.marginDepositedTotal // Add the line below\\n _marginDelta // Add the line below\\n profitLossTotal\\n// Add the line below\\n newMarginDepositedTotal = globalPositions.marginDepositedTotal // Add the line below\\n _marginDelta\\n\\nif (newMarginDepositedTotal < 0) {\\n    revert FlatcoinErrors.InsufficientGlobalMargin();\\n}\\n\\n_globalPositions = FlatcoinStructs.GlobalPositions({\\n    marginDepositedTotal: uint256(newMarginDepositedTotal),\\n    sizeOpenedTotal: (int256(_globalPositions.sizeOpenedTotal) // Add the line below\\n _additionalSizeDelta).toUint256(),\\n    lastPrice: _price\\n});\\n        \\n// Remove the line below\\n _updateStableCollateralTotal(// Remove the line below\\nprofitLossTotal);\\n```\\n\\nThe existing `updateGlobalPositionData` function still needs to be used for other functions besides liquidation. As such, consider creating a separate new function (e.g., updateGlobalPositionDataDuringLiquidation) solely for use during the liquidation that includes the above fixes.\\nThe following attempts to apply the above fix to the three (3) examples described in the report to verify that it is working as intended.\\nFirst Example\\nLet the current `StableCollateralTotal` be $x$ and `marginDepositedTotal` be $y$ at the start of the liquidation.\\nDuring funding settlement:\\nStableCollateralTotal = $x$ + 100\\nmarginDepositedTotal = $y$ - 100\\nDuring updateStableCollateralTotal:\\n```\\nvault.updateStableCollateralTotal(int256(remainingMargin) - positionSummary.profitLoss);\\nvault.updateStableCollateralTotal(0 - (+100));\\nvault.updateStableCollateralTotal(-100);\\n```\\n\\nStableCollateralTotal = ($x$ + 100) - 100 = $x$\\nDuring Global Position Update:\\nmarginDelta = -(position.marginDeposited + positionSummary.accruedFunding) = -(20 + (-100)) = 80\\nnewMarginDepositedTotal = marginDepositedTotal + marginDelta = ($y$ - 100) + 80 = ($y$ - 20)\\nNo change to StableCollateralTotal here. Remain at $x$\\nConclusion:\\nThe LPs should not gain or lose in this scenario. Thus, the fact that the StableCollateralTotal remains as $x$ before and after the liquidation is correct.\\nThe `marginDepositedTotal` is ($y$ - 20) is correct because the liquidated position's remaining margin is 20 ETH. Thus, when this position is liquidated, 20 ETH should be deducted from the `marginDepositedTotal`\\nNo revert during the execution.\\nSecond Example\\nDuring updateStableCollateralTotal:\\n```\\nvault.updateStableCollateralTotal(settledMargin - positionSummary.profitLoss);\\nvault.updateStableCollateralTotal(-3 rETH - (-5 rETH));\\nvault.updateStableCollateralTotal(+2);\\n```\\n\\nStableCollateralTotal = 5 + 2 = 7 ETH\\nDuring Global Position Update:\\nmarginDelta = -(position.marginDeposited + positionSummary.accruedFunding) = -(2 + 0) = -2\\nmarginDepositedTotal = marginDepositedTotal + marginDelta = 2 + (-2) = 0\\nConclusion:\\nStableCollateralTotal = 7 ETH, marginDepositedTotal = 0 (Total 7 ETH tracked in the system)\\nBalance of collateral in the system = 7 ETH. Thus, both values are in sync. No revert.\\nThird Example\\nDuring funding settlement (Transfer 5 from Long to LP):\\nmarginDepositedTotal = 70 + (-5) = 65\\nStableCollateralTotal = 100 + 5 = 105\\nTransfer fee to Liquidator\\n50 ETH sent to the liquidator from the system: Balance of collateral in the system = 170 ETH - 50 ETH = 120 ETH\\nDuring updateStableCollateralTotal:\\n```\\nvault.updateStableCollateralTotal(remainingMargin - positionSummary.profitLoss) =>\\nstableCollateralTotal = 105 ETH + (remaining margin - PnL)\\nstableCollateralTotal = 105 ETH + (10 ETH - (-5 ETH))\\nstableCollateralTotal = 105 ETH + (15 ETH) = 120 ETH\\n```\\n\\nStableCollateralTotal = 120 ETH\\nDuring Global Position Update:\\nmarginDelta= -(position.marginDeposited + positionSummary.accruedFunding) = -(70 + (-5)) = -65\\nmarginDepositedTotal = 65 + (-65) = 0\\nConclusion:\\nStableCollateralTotal = 120 ETH, marginDepositedTotal = 0 (Total 120 ETH tracked in the system)\\nBalance of collateral in the system = 120 ETH. Thus, both values are in sync. No revert.
The following is a list of potential impacts of this issue:\\nFirst Example: LPs incur unnecessary losses during liquidation, which would be avoidable if the calculations were correctly implemented from the start.\\nSecond Example: An error in the protocol's accounting mechanism led to an inflated increase in the LPs' stable collateral total, which in turn inflated the number of tokens users can withdraw from the system.\\nThird Example: The accounting error led to the tracked balance and actual collateral balance not being in sync.
```\\nsettleFundingFees() = Short/LP need to pay Long 100\\n\\nmarginDepositedTotal = marginDepositedTotal + funding fee\\nmarginDepositedTotal = y + (-100) = (y - 100)\\n\\nstableCollateralTotal = x + (-(-100)) = (x + 100)\\n```\\n