c4-bot-9 commented 4 months ago

Lines of code

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

Vulnerability details

Impact

Nonexercisable positions can be exercisable

Proof of Concept

The protocol checks if a position is exercisable, i.e. at least one of its legs is outside of predefined width, by currentTick >= _strike + rangeUp) || (currentTick < _strike - rangeDown condition.

However, it also includes the case when the tick is right at the width, which should be nonexercisable.

As per the validation of exercisable position logic, the position should be 1 tick out of the range of the current tick:

Contract: PanopticPool.sol

1218:         // on forced exercise, the price *must* be outside the position's range for at least 1 leg
1219:         touchedId[0].validateIsExercisable(twapTick);

And validateIsExercisable checks the issue as :

Contract: TokenId.sol

578:     function validateIsExercisable(TokenId self, int24 currentTick) internal pure {
579:         unchecked {
580:             uint256 numLegs = self.countLegs();
581:             for (uint256 i = 0; i < numLegs; ++i) {
582:                 (int24 rangeDown, int24 rangeUp) = PanopticMath.getRangesFromStrike(
583:                     self.width(i),
584:                     self.tickSpacing()
585:                 );
586: 
587:                 int24 _strike = self.strike(i);
588:                 // check if the price is outside this chunk
589:                 if ((currentTick >= _strike + rangeUp) || (currentTick < _strike - rangeDown)) { 
590:                     // if this leg is long and the price beyond the leg's range:
591:                     // this exercised ID, `self`, appears valid
592:                     if (self.isLong(i) == 1) return; // validated
593:                 }
594:             }
595:         }
596: 
597:         // Fail if position has no legs that is far-out-of-the-money
598:         revert Errors.NoLegsExercisable();
599:     }

Looking at L:589, it also checks the equivalency - not 1 tick difference only which means that the positions that don´t have this difference can also be exercisable.

In addition, rangeup calculation is based on rounding up - which we can say that it´s really being forced to be inside the ticks even if it might not be:

Contract: PanopticMath.sol

371:     function getRangesFromStrike(
372:         int24 width,
373:         int24 tickSpacing
374:     ) internal pure returns (int24, int24) {
375:         return (
376:             (width * tickSpacing) / 2,
377:             int24(int256(Math.unsafeDivRoundingUp(uint24(width) * uint24(tickSpacing), 2))) //@audit rounding up
378:         );
379:     }

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend refactoring the logic to below:

-  if ((currentTick >= _strike + rangeUp) || (currentTick < _strike - rangeDown)) { 
+  if ((currentTick > _strike + rangeUp) || (currentTick < _strike - rangeDown)) {

Assessed type

Invalid Validation

c4-judge commented 4 months ago

Picodes marked the issue as primary issue

dyedm1 commented 4 months ago

Actually, Uniswap defines the tickLower as in-range but the tickUpper as out-of-range by convention. You also misunderstood that comment -- one leg (out of four legs each with their own Uniswap chunk) must be out of range, not 1 tick out of range.

Picodes commented 4 months ago

This is indeed the convention. See for example https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L328C17-L328C28.