Open c4-bot-3 opened 4 months ago
Picodes marked the issue as primary issue
Picodes marked the issue as satisfactory
Picodes marked the issue as selected for report
Hello judge, I'd like to dispute the severity of this report based on the following reasons:
When passing duplicates to mint a position on an insolvent account, first of all, the position's owner is playing a race against a liquidator (because the account is insolvent). If the position's owner opts to use this flaw to mint a new position, the liquidator will anyways liquidate all the user's positions, seizing the position's owner collateral.
When liquidating a position, it is true that is possible to pass duplicate positions on the positionList of the account to be liquidated, and indeed this will cause the accountMarging to be bigger than what it really is, thus, this check is bypassed, but, when the actual positions of the provided positionList are burnt as part of the liquidation process this is what will happen:
positionSize
will be 0, thus, when the second match of the same duplicate position is attempted to be burnt, the positionSize is 0, thus, on the SFPM the tx will be reverted because the positionSize
is 0, thus, users are not at risk.When force exercising, only 1 position can be forced exercise at a time, so, it is not like by passing duplicates on the positionList could be possible to charge multiple times the OptionBuyer on the position being exercised.
When settling long premium, only the last position of the positionList is the one that will settle premium on, the rest of the positions only matter to validate if the positionList is indeed the user's position and to validate if the user is solvent after settling the long premium. If it is not solvent, the tx reverts. The only harm that this can cause is to force an account that is insolvent to pay the already owed premium to the option seller, but, anyway, if the account is already insolvent, the account anyways would be liquidated, premium would be settled and user's collateral would be seized.
Because of the above points, I'd like to ask for a reconsideration of the severity assessed in this report, since the most impactful consequence is that users could mint 1 position above their limit, but as a result of doing it, their own accounts will fall into liquidation, thus, they will end up losing their collateral and gaining nothing in return.
Hi @stalinMacias.
For point 1, allowing to mint/burn a position for a insolvent account contradicts one of the main invariant in contest readme: Users should not be allowed to mint/burn options or pay premium if their end state is insolvent (at the fast oracle tick, or both ticks if the fast and slow oracle ticks are further away than the threshold)
For point 2, the report never stated that an attacker can liquidate a solvent account. I am aware of what you are saying.
For point 3, the point is the user can pass a forged positionIdListExercisee and force exercise the position even though the exercisee is insolvent.
For point 4, allowing a non solvent account to be settleLongPremium
-ed contradicts the readme as well settleLongPremium - Force a solvent option buyer to pay any premium owed to sellers. Options sellers will want to call this if a large portion of the total premium owed in their chunk is pending/has not been settled.
.
Hey @pkqs90
Not because an invariant is broken, a report should automatically be awarded as a high severity, it should be assessed what is the impact of breaking such an invariant that determines the final severity. The fact that an invariant is broken makes the report a valid concern, but that does not warrant a high sev automatically.
For point 1, a user who is trying to abuse this problem would cause that his account falls into liquidation mode. Since assets never leave the Panoptic ecosystem (they are just moved between pools), the end result of forcing the mint of this extra position would be that the user losses his collateral because of the account getting liquidated and the user not getting anything in return. Because of this the loss falls on the "attacker" side when his collateral is seized, not on the protocol,
For point 3, the purpose of force exercising a position is that an OptionSeller can forcefully close a Buyer's position so that the OptionSeller can close his position. If the Buyer's account is insolvent, there is no need to even force the exercise of a single position since the whole account can be liquidated. The OptionBuyer won't need to pay the forceExerciseFees and instead will get an extra commission for liquidating the position.
For point 4, contradicting the readme is breaking an invariant, for this reason, the problem is indeed valid, but as for the severity, similar to point 3, if the account is already insolvent, there are more incentives to liquidate the position, seize more collateral and get an extra percentage than rather just settling the long premium of a single position.
Hey @stalinMacias,
For point 1, I think something to consider here is that the liquidation does not happen automatically, as does in a CEX. Since the protocol is onchain and relies on liquidation bots, the liquidation may not happen in some period of time.
The user can use this time period to open another position and increase the leverage, and as the price moves, the position will be healthy again, and the user "saves" himself from liquidation.
To make a comparison in CEX, this is like when a user is supposed to be liquidated, he instead creates more long/short positions to increase leverage, and when the price moves in the correct direction and account is healthy again, he sells his position to avoid being liquidated.
The difference is this cannot actually happen in a CEX because liquidation is done automatically, but can happen here.
For point 1, I think something to consider here is that the liquidation does not happen automatically, as does in a CEX. Since the protocol is onchain and relies on liquidation bots, the liquidation may not happen in some period of time. The user can use this time period to open another position and increase the leverage, and as the price moves, the position will be healthy again, and the user "saves" himself from liquidation.
You've just listed a couple of external requirements for this problem to actually have an impact.
This seems to match more the description of a medium severity rather than a high sev.
2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
I've made my points and will let the judge take the final say in this.
@stalinMacias thanks for your points.
I don't think here "liquidation bots not liquidating the insolvent position during a period of time" is an external requirement considering how critical liquidations are to Panoptic. My reasoning was that even if the loss of funds is not "direct", being able to properly control when positions can be opened is a key feature, and the fact that you can "increase your leverage" while being insolvent prevents proper risk control.
However, it's true that this is not strictly speaking a scenario where "assets can be stolen/lost/compromised directly" so I'll downgrade to Med.
Picodes changed the severity to 2 (Med Risk)
Hi @Picodes you have made the following comment:
I don't think here "liquidation bots not liquidating the insolvent position during a period of time" is an external requirement considering how critical liquidations are to Panoptic.
But as per the Panoptic documentation
the Liquidation Bot
is expected to monitor all positions opened in a pool across all accounts at regular time intervals ensuring real-time tracking of the collateral status of each position and to trigger an alert of the underwater accounts and not to liquidate the position directly.
The following is stated in the Panoptic documentation :
The bot is programmed to identify accounts that become liquidatable, flagging any account that falls below the required collateral thresholds.
Upon identifying a liquidatable account, the bot triggers an alert. Liquidators can then engage by interacting directly with the smart contracts governing the pool.
Since the liquidation bots are not programmed to liquidate the positions but to alert the liquidators at regular time intervals, of the underwater accounts
for liquidations, there can be a time period between account going underwater and being liquidated. Hence this seems to be a probable scenario to occur where the attacker can use this time period to open another position and increase the leverage, and as the price moves, the position will be healthy again, and the user "saves" himself from liquidation
as stated by @pkqs90 above.
I would kindly like to know your opinion on the above.
Thank you
@udsene I am not sure to understand what additional facts you are bringing. As I said above I agree with you that it's not an external requirement as it's meant to happen, but the loss of funds isn't direct
@Picodes sorry it seems I had misunderstood your comment. Thanks for the clarification.
Lines of code
https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L1367-L1391 https://github.com/code-423n4/2024-04-panoptic/blob/main/contracts/PanopticPool.sol#L887-L893
Vulnerability details
Impact
The underlying issue is that
_validatePositionList()
does not check for duplicate tokenIds. Attackers can use this issue to bypass solvency checks, which leads to several impacts:This also conflicts a main invariant stated in contest readme:
Users should not be allowed to mint/burn options or pay premium if their end state is insolvent
.Bug Description
First, let's see why
_validatePositionList
does not check for duplicate tokenIds. For a user position hash, the first 8 bits is the length of tokenIds which overflows, last 248 bits is the xor hash. However, we can easily add 256 tokenIds of the same kind to create a duplicate positionHash.For example:
Hash(key0, key1, key2) == Hash(key0, key1, key2, key0, key0, ..., 256 more key0)
. This way, we can add any tokenId we want while still arriving the same position hash.PanopticPool.sol
PanopticMath.sol
Then, let's see how duplicate ids can bypass solvency check. The solvency check is in
_validateSolvency()
, which is called by all the user interaction functions, such as mint/burn/liquidate/forceExercise/settleLongPremium. This function first checks for a user passed inpositionIdList
(which we already proved can include duplicates), then calls_checkSolvencyAtTick()
to calculate thebalanceCross
(collateral balance) andthresholdCross
(required collateral) for all tokens.The key is the collateral balance includes the premium that is collected for each of the positions. For most of the positions, the collected premium should be less than required collateral to keep this position open. However, if a position has been open for a long enough time, the fees it accumulated may be larger than required collateral.
For this kind of position, we can duplicate it 256 times (or multiple of 256 times, as long as gas fee is enough), and make our collateral balance grow faster than required collateral. This can make a insolvent account "solvent", by duplicating the key tokenId multiple times.
CollateralTracker.sol
Now we have shown how to bypass the
_validateSolvency()
, we can bypass all related checks. Listing them here:Proof of Concept
In this report, we will only prove the core issue: duplicate tokenIds are allowed, and won't craft complicated scenarios for relevant impacts.
Add the following test code in
PanopticPool.t.sol
, we can see that passing 257 of the same token ids can still work formintOptions()
.Tools Used
Manual review
Recommended Mitigation Steps
Add a check in
_validatePositionList
that the length is shorter thanMAX_POSITIONS
(32).Assessed type
Invalid Validation