c4-bot-8 commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/types/LeftRight.sol#L293-L300

Vulnerability details

Impact

It is possible to have a scenario where one token Gross and Owed freezes while the other one accrues.

In SFPM.getAccountPremium(), a comment explains the reason for adding cap to accumulators

 // add deltas to accumulators and freeze both accumulators (for a token) if one of them overflows
                // (i.e if only token0 (right slot) of the owed premium overflows, then stop accumulating  both token0 owed premium and token0 gross premium for the chunk)
                // this prevents situations where the owed premium gets out of sync with the gross premium due to one of them overflowing
                (premiumOwed, premiumGross) = LeftRightLibrary.addCapped(
                    s_accountPremiumOwed[positionKey],
                    premiumOwed,
                    s_accountPremiumGross[positionKey],
                    premiumGross
                );

As stated, it is to prevent gross and owed premium from getting out of sync. however, the addCapped() does not prevent both tokens from going out of syn.

In a situation where either of the token premium(gross/owed) overflows, no check to ensure both tokens' premium remains synced.

Proof of Concept

addCapped() only ensures that gross and owed of token0 does not overflow, without ensuring that neither of both tokens(0 & 1) gross and owed overflows.

 return (
            LeftRightUnsigned.wrap(0).toRightSlot(r_Enabled ? z_xR : x.rightSlot()).toLeftSlot(
                l_Enabled ? z_xL : x.leftSlot()
            ),
            LeftRightUnsigned.wrap(0).toRightSlot(r_Enabled ? z_yR : y.rightSlot()).toLeftSlot(
                l_Enabled ? z_yL : y.leftSlot()
            )
        );

The implication of this is that, if for instance, a pool with one high-value token(BTC) is with a lower-value token(pepe), there is a probability that the gross and owed premium increase of BTC will not lead to an overflow, while either of the gross or owed premium of PEPE overflows. what happens is that BTC gross and owed values are updated(increased), while PEPE gross and owed remain frozen.

One possible exploit of this scenario is that, if such pools are created, malicious users could long(take out liquidity from AMM) and when closing such positions. Negative premia that ought to be paid to sellers(those that added liquidity to the AMM) will not be paid fully since the other token premia accumulator has been frozen.

Tools Used

Manual review.

Recommended Mitigation Steps

require(r_Enabled & l_Enabled || !r_Enabled & !l_Enabled);

Assessed type

Invalid Validation

c4-judge commented 4 months ago

Picodes marked the issue as duplicate of #364

c4-judge commented 4 months ago

Picodes marked the issue as unsatisfactory: Invalid

lanrebayode commented 4 months ago

@Picodes thanks for judging.

While I agree with sponsor comment in #364

Premium accumulation is completely segregated between the two pool tokens, so it's fine for one token to keep accumulating while the other stops, as long as the linked (intra-token) owed and gross accumulators are in sync.

In the instance I sited here; a pool consisting of BTC and Pepe or any pool with high value difference, if;

A very large amount of Liquidity was provided through short option minters
A malicious set of long option minters can mint long options(take out liquidity) and then go to the V3 pool to perform large amount of swaps to increase what should have been accred fees for the option they minted massively.(Note: the fees for such pools is usually high to about 1%)
When this fees become large enough, they only have to pay half of the supposed premium, since the other token pair premium have been freezed

Picodes commented 3 months ago

Considering that it's stated in the contest readme that "Very large quantities of tokens are not supported. ", and that in the described scenario the profitability of doing large swaps to avoid paying a premium without this hypothesis is hard to conceive, I still think it falls within QA at best.

c4-judge commented 3 months ago

Picodes marked the issue as not a duplicate