The algorithm used for accounting PnL is incorrect

Summary

In current algorithm, pending PnL is settled at each time a position is touched, this is incorrect. As settlement of global position is equivalent to close all positions in the system and reopen them. Meanwhile, all individual positions except current processing one keep unchanged, mismatch of global and individual accounting occurs. This also causes any individual position's final PnL to be vague, if there is any touch, such as increasing collateral, before final close of position.

Vulnerability Detail

Let's take two examples to show the problem:

File: src/libraries/PerpMath.sol
175:     function _profitLoss(FlatcoinStructs.Position memory position, uint256 price) internal pure returns (int256 pnl) {
176:         int256 priceShift = int256(price) - int256(position.lastPrice);
177:         int256 profitLossTimesTen = (int256(position.additionalSize) * (priceShift) * 10) / int256(price);
178: 
179:         if (profitLossTimesTen % 10 != 0) {
180:             return profitLossTimesTen / 10 - 1;
181:         } else {
182:             return profitLossTimesTen / 10;
183:         }
184:     }

File: src/libraries/PerpMath.sol
192:     function _profitLossTotal(
193:         FlatcoinStructs.GlobalPositions memory globalPosition,
194:         uint256 price
195:     ) internal pure returns (int256 pnl) {
196:         int256 priceShift = int256(price) - int256(globalPosition.lastPrice);
197: 
198:         return (int256(globalPosition.sizeOpenedTotal) * (priceShift)) / int256(price);
199:     }

1. Mismatch of global and individual position accounting

// 1. Alice open with 1ETH additional size at price 1000
Alice.lastPrice = 1000
Alice.additionalSize = 1ETH
global.lastPrice = 1000
global.sizeOpenedTotal = 1ETH

// 2. Bob open with 1Gwei negligible additional size at price 2000
Alice.lastPrice = 1000
Alice.additionalSize = 1ETH
Bob.lastPrice = 2000
Bob.additionalSize = 1Gwei
global.lastPrice = 2000
global.PnL = 1ETH * (2000 - 1000) / 2000 = 0.5ETH
global.sizeOpenedTotal = 1ETH + 1Gwei ~= 1ETH

// 3. Alice close at price 3000
Alice.lastPrice = 0
Alice.additionalSize = 0
Alice.PnL = 1ETH * (3000 - 1000) / 3000 = 0.67ETH
Bob.lastPrice = 2000
Bob.additionalSize = 1Gwei
global.lastPrice = 3000
global.PnL = global.PnL + 1ETH * (3000 - 2000) / 3000 = 0.5ETH + 0.33ETH = 0.83ETH
global.sizeOpenedTotal = 1Gwei

After close of Alice's position, the global sizeOpenedTotal is nearly zero, the accounted PnL of Alice is 0.67ETH, but the accounted PnL of global is 0.83ETH, they are mismatch. Liqudity providers lose more than they are actually lost.

2. vague individual PnL accounting

// 1. Alice and Bob both open with 1ETH additional size at price 1000
Alice.lastPrice = 1000
Alice.additionalSize = 1ETH
Bob.lastPrice = 1000
Bob.additionalSize = 1ETH

// 2. Alice increases negligible collateral at price 2000 and Bob does nothing
Alice.lastPrice = 2000
Alice.additionalSize = 1ETH
Alice.PnL = 1ETH * (2000 - 1000) / 2000 = 0.5ETH

// 3. Alice and Bob both close position at price 3000
Alice.PnL = Alice.PnL + 1ETH * (3000 - 2000) / 3000 = 0.5ETH + 0.33ETH = 0.83ETH
Bob.PnL = Bob.PnL + 1ETH * (3000 - 1000) / 3000 = 0ETH + 0.67ETH = 0.67ETH

We can see, though Alice and Bob open and close position at the same price and with same size, but get different PnL just due to one negligible touch before final close.

Impact

Incorrect accounting always cause loss for one side, either traders or liquidity providers.

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/libraries/PerpMath.sol#L175

Tool used

Manual Review

Recommendation

don't settle PnL until close or liquidation of position, and update lastPrice as weighted average entry price while adjust position size. This is available for both individual and global positions.

For example

// 1. Alice and Bob both open with 1ETH additional size at price 1000
Alice.lastPrice = 1000
Alice.additionalSize = 1ETH
Bob.lastPrice = 1000
Bob.additionalSize = 1ETH

// 2. Alice increases/adjust 1ETH position at price 2000 and Bob opens a new 1ETH position
Alice.lastPrice = (1ETH * 1000 + 1ETH * 2000) / 2ETH = 1500
Alice.additionalSize = 2ETH
Bob.lastPriceOfPositionA = 1000
Bob.additionalSizeOfPositionA = 1ETH
Bob.lastPriceOfPositionB = 2000
Bob.additionaSizeOfPositionB = 1ETH

// 3. close all positions at price 3000
Alice.PnL = 2ETH * (3000 - 1500) / 3000 = 1ETH
Bob.PnLOfPositionA = 1ETH * (3000 - 1000) / 3000 = 0.67ETH
Bob.PnLOfPositionB = 1ETH * (3000 - 2000) / 3000 = 0.33ETH
Bob.PnL = 0.67ETH + 0.33ETH = 1ETH

We can see the overall PnL of Alice and Bob is equal, as PnL caused by increasing size on an existing position at some price should always be equivalent to open a new position with same size and same price.

Duplicate of #186

sherlock-audit / 2023-12-flatmoney-judging

KingNFT - The algorithm used for accounting PnL is incorrect #161