Incorrect token settlement in `CollateralTracker.sol`

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L1067-L1079 https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/CollateralTracker.sol#L1010-L1022

Vulnerability details

While minting/burning options, the PanopticPool.sol calls the takeCommissionAddData()/exercise() function in CollateralTracker.sol. tokenToPay is the premium to paid/collected when the position is closed. The function converts this amount into the corresponding shares that need to be minted/burned for the user. However, the current calculation does not account for the impact of the minting/burning on totalSupply. The calculation also does not account for the changes in totalAssets() due the swapped amount swappedAmount.

Impact

If the user's shares were burnt, this would mean that the value of totalSupply() decreased, and therefore the user's remaining shares would be worth more. Whereas, if they were minted, the value of totalSupply() would increase, and the value of the sharesToMint that was calculated to compensate the user for the equivalent amount of tokens would be lesser than it should be. These inaccuracies would lead to the user being incorrectly compensated/charged while burning options.

Proof of Concept

Assume the totalSupply() is 100, totalAssets() is 100 and the user holds 25 shares.
The value of the user's shares: 25*100/100 tokens = 25 tokens

Case #1 : tokenToPay == 20

sharesToBurn = 20*100/100 = 20

20 shares are burnt and now the totalSupply() is 80

The value of the user's shares now in assets would be : (25-20)*100/80 = 6.25

The user effectively payed: 25-6.25 = 18.75 equivalent amount in tokens, which is 1.25 lesser than the intended value

Case #2 : tokenToPay == -20

sharesToMint = 20*100/100 = 20

20 shares are minted and now the totalSupply() is 120

The value of the user's shares now in assets would be : (25+20)*100/120 = 37.5

The user effectively received 37.5 - 20 = 17.5 tokens, which is 2.5 lesser than the intended value

Note: This PoC does not take into account the changes to totalAssets() during the exercise, however, the impact would be similar since it's not related to the change in totalSupply(). Additionally, it could be argued that updatedAssets should be used in the calculation as well as the post function call value of totalAssets() is directly dependent on this value.

Tools Used

Manual Review

Recommended Mitigation Steps

Account for the minted/burnt shares in the calculation of sharesToBurn/sharesToMint variables in CollateralTracker.sol::exercise() and CollateralTracker.sol::takeCommissionAddData()

Assessed type

Error

[dyedm1]

The only time the share price changes meaningfully is when there is a commission fee at mint/burn, which is intended. Other components of the tokenToPay are reflected in the inAMM/poolAssets trackers.

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof

[sammy-tm]

@dyedm1 Can you elaborate more on what you mean by "meaningfully"/ why you think this is invalid?

Can you point out what is wrong with the proof provided?

Thanks.