Slippage checks are disabled in an edge case when minting/burning options; may lead to loss of funds

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L630-L630
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/PanopticPool.sol#L834-L834

Vulnerability details

Description

Both externally callable functions mintOptions() and burnOptions() include parameters tickLimitLow, tickLimitHigh which are intended for slippage protection. These functions further continue as internal functions _mintOptions(), and _burnOptions(), both having this fragment

        (tickLimitLow, tickLimitHigh) = _getSlippageLimits(tickLimitLow, tickLimitHigh);

Function _getSlippageLimits() though, whenever tickLimitLow <= tickLimitHigh reverses their order, and, moreover, if tickLimitLow == tickLimitHigh assigns (tickLimitLow, tickLimitHigh) = (MAX_SWAP_TICK, MIN_SWAP_TICK):

    function _getSlippageLimits(
        int24 tickLimitLow,
        int24 tickLimitHigh
    ) internal pure returns (int24, int24) {
        // disable slippage checks if tickLimitLow == tickLimitHigh
        if (tickLimitLow == tickLimitHigh) {
            // note the reversed order of the ticks
            return (MAX_SWAP_TICK, MIN_SWAP_TICK);
        }

        // ensure tick limits are reversed (the SFPM uses low > high as a flag to do ITM swaps, which we need)
        if (tickLimitLow < tickLimitHigh) {
            return (tickLimitHigh, tickLimitLow);
        }

        return (tickLimitLow, tickLimitHigh);
    }

As a result, when the swap is executed in _validateAndForwardToAMM(), the following happens:

        if (tickLimitLow > tickLimitHigh) {
            // if the in-the-money amount is not zero (i.e. positions were minted ITM) and the user did provide tick limits LOW > HIGH, then swap necessary amounts
            if ((LeftRightSigned.unwrap(itmAmounts) != 0)) {
                totalMoved = swapInAMM(univ3pool, itmAmounts).add(totalMoved);
            }

            (tickLimitLow, tickLimitHigh) = (tickLimitHigh, tickLimitLow);
        }

        // Get the current tick of the Uniswap pool, check slippage
        (, int24 currentTick, , , , , ) = univ3pool.slot0();

        if ((currentTick >= tickLimitHigh) || (currentTick <= tickLimitLow))
            revert Errors.PriceBoundFail();

With tickLimitLow == MAX_SWAP_TICK, and tickLimitHigh == MIN_SWAP_TICK, they get swapped, and the slippage check passes trivially. Thus, although the user requested the swap at a precise tick (tickLimitLow == tickLimitHigh), the swap will be executed regardless, at any price tick.

From the comment "disable slippage checks if tickLimitLow == tickLimitHigh" it looks like a deliberate design decision, but the rationale behind it is unclear, and it is clearly against user expectations.

Severity Rationalization

• Impact - high
• Likelihood - medium
• Severity - high

Impact

Arbitrary slippage when the user expects no slippage at all; loss of funds.

Proof of Concept

• Initialize a UniSwap pool arbitrarily
• Call function mintOptions / burnOptions of PanopticPool with tickLimitLow == tickLimitHigh
• Observe that the options are minted / burned irrespective of the requested tick.

In particular, apply the following diff to function test_Fail_mintOptions_LowerPriceBoundFail()

diff --git a/test/foundry/core/PanopticPool.t.sol b/test/foundry/core/PanopticPool.t.sol
index 025f384..d840883 100644
--- a/test/foundry/core/PanopticPool.t.sol
+++ b/test/foundry/core/PanopticPool.t.sol
@@ -2985,7 +2985,7 @@ contract PanopticPoolTest is PositionUtils {
         posIdList[0] = tokenId;
 
         vm.expectRevert(Errors.PriceBoundFail.selector);
-        pp.mintOptions(posIdList, positionSize, 0, TickMath.MAX_TICK - 1, TickMath.MAX_TICK);
+        pp.mintOptions(posIdList, positionSize, 0, TickMath.MAX_TICK - 1, TickMath.MAX_TICK-1);
     }
 
     function test_Fail_mintOptions_UpperPriceBoundFail(

and run with forge test -vvvv --match-test test_Fail_mintOptions_LowerPriceBoundFail. The change makes the test failing, in which the call mintOptions(), which was expected to revert, succeeded:

...
    │   │   │   ├─ [3974] CollateralTracker::getAccountMarginDetails(0x0000000000000000000000000000000000123456, 1518, [[1978802776077654342231847164029613 [1.978e33], 1103674500445210 [1.103e15]]], 0) [delegatecall]
    │   │   │   │   └─ ← 20282409603651670423947250952682 [2.028e31]
    │   │   │   └─ ← 20282409603651670423947250952682 [2.028e31]
    │   │   ├─ emit OptionMinted(recipient: 0x0000000000000000000000000000000000123456, positionSize: 1103674500445210 [1.103e15], tokenId: 1978802776077654342231847164029613 [1.978e33], poolUtilizations: 0)
    │   │   └─ ← ()
    │   └─ ← ()
    └─ ← call did not revert as expected

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 45.61s (42.40s CPU time)

Ran 1 test suite in 48.41s (45.61s CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/foundry/core/PanopticPool.t.sol:PanopticPoolTest
[FAIL. Reason: call did not revert as expected; counterexample: calldata=0x908278181fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00131ca863023eb037cee3987aae8d37a63ae12269ad088ff5a5378c50bb46190000000000000000000000000000000000000000000000000000000377516be40000000000000000000000000000000000000000000000000000000000000003 args=[14474011154664524427946373126085988481658748083205070504932198000989141204991 [1.447e76], 33767882826379883118500010987071557442851304961975580042921141779331499545 [3.376e73], 14886726628 [1.488e10], 3]] test_Fail_mintOptions_LowerPriceBoundFail(uint256,uint256,int256,uint256) (runs: 0, μ: 0, ~: 0)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual audit

Recommended Mitigation Steps

Remove the aforementioned special case from function _getSlippageLimits.

Assessed type

Math

[c4-judge]

Picodes marked the issue as duplicate of #302

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid

[andrey-kuprianov]

Dear @Picodes, dear @dyedm1, I kindly ask you to:

1. Deduplicate this finding from 302: they are not dups, but completely opposite. Here is why:

• The issues were bot-grouped initially together into 302_group, because they both are about slippage checks in the same functions
• This finding has also been marked as a duplicate of 302, based on superficial similarity
• Finding 302 was classified as invalid, and this finding was also wrongly automatically classified as invalid based on its status as a duplicate.
• In reality, though discussing the same aspect (slippage checks), the findings are the opposites of each other:
  • Finding 302 is based only on a few lines of code, and effectively argues only about the preceding code comment ("disable slippage checks if tickLimitLow == tickLimitHigh"); as its impact the warden argues that "slippage check is not disabled when it is expected to be disabled".
  • This finding analyzes the complete path to the opposite real impact, which is "Slippage checks are disabled in an edge case. Arbitrary slippage when the user expects no slippage at all; loss of funds.". Moreover, this finding includes the coded PoC.
  • Notice also that the proposed remediations are opposite: while finding 302 proposes a special treatment, in the form of "whenever it returns MAX_SWAP_TICK & MIN_SWAP_TICK to ensure slippage is disabled as expected by protocol" (which is an invalid recommendation, a NO-OP, as these values do indeed disable slippage checks); this finding, on the contrary, recommends removing the root cause, which will lead to refusing a mint/burn operation where tickLimitLow == tickLimitHigh, thus fixing the bug.

2. Evaluate this finding based on its own merits
The present finding describes a real bug in the system, which can lead to arbitrarily high loss of funds for the users. Please see the charts below, which depict the example data on how the system behaves when mintOptions is called with the respective tickLimitLow / tickLimitHigh. As can be seen, slippage checks kick in correctly and limit loss of funds, i.e. the deviation of the current tick from the desired value (median) when traded ITM; slippage checks limit loss the more so, the tighter slippage bounds are provided. With one exception: when tickLimitLow == tickLimitHigh, the loss of funds becomes unlimited.

How likely is such input to happen? Very likely, essentially guaranteed, because this is the way it is described e.g. in Minting a Panoption documentation:

pp.mintOptions({
    positionIdList: positionIdList,
    positionSize: 10 * 10 ** 18,
    effectiveLiquidityLimitX32: 0,
    tickLimitLow: 0,
    tickLimitHigh: 0
});    

We all know that such code fragments from documentation are frequently copy-pasted by developers.

Why is this a problem? This code fragment is intended for trading OTM, not ITM! Yes, this is exactly right! And this is the core of the problem: mintOptions() / burnOptions() calls are intended for trading both ITM and OTM. Users are expected to indicate that they want to trade ITM by making tickLimitLow > tickLimitHigh. Making tickLimitLow == tickLimitHigh == 0 is the recommended way to trade OTM. But with such input the system disables slippage checks by itself, setting them to (MAX_SWAP_TICK, MIN_SWAP_TICK).

Needless to say that a transaction with disabled slippage checks in a mempool is an open invitation to attacks, such as sandwich attacks or in general pool manipulation. Nothing prevents an attacker to shift the pool state in such a way that a transaction submitted with the intention of minting/burning options OTM, will result in being executed ITM, resulting in the unbounded losses for the user.

With kind regards,
kuprum, on behalf of the CodeWasp team.

[andrey-kuprianov]

For clarity, let me also elaborate as to why, in my understanding, this bug happened (in the audited version of the code):

• In the PanopticPool contract, reversing the slippage limits (i.e. making tickLimitLow > tickLimitHigh) is used as a flag, indicating to the SemiFungiblePositionManager contract that the swap should be performed ITM.
• Thus, function _getSlippageLimits() reverses those limits when they are not already provided in the reversed order.
• But how to reverse the case tickLimitLow == tickLimitHigh? The current implementation reverses them by assigning (tickLimitLow, tickLimitHigh) = (MAX_SWAP_TICK, MIN_SWAP_TICK), thus turning a single-point interval into a maximal interval; this is the bug.
• In the slippage checks at the end of _validateAndForwardToAMM(), (tickLimitLow, tickLimitHigh) is treated as an exclusive interval, i.e. the input tickLimitLow == tickLimitHigh should have been rejected.
• But, as explained above, _getSlippageLimits() turned an empty interval (from SFPM's point of view) into a full interval, thus slippage checks pass trivially for any actual price.
• The core of the problem seems to be in the mismatched assumptions about the API between the two contracts: while SemiFungiblePositionManager treats slippage limits as exclusive, PanopticPool (and user-facing documentation) treats them as inclusive, and thus tries to propagate even the single-point interval which should be actually rejected.
• We recommend to carefully formulate/formalize these assumptions, and properly communicate them between the developers and to the users. The separation into mintOptionsOTM / mintOptionsITM is even better, because it makes the user assumptions about trading OTM or ITM explicit.

[andrey-kuprianov]

3. Consider the evidence of the bug being already mitigated in Panoptic

Here is the fact: a few days after the audit completion, the Panoptic docs have been updated in the following way (both for mintOptions() and burnOptions()):

* instead of a single function with the signature:
  ```solidity
  function mintOptions(
       TokenId[] calldata positionIdList,
       uint128 positionSize,
       uint64 effectiveLiquidityLimitX32,
       int24 tickLimitLow,
       int24 tickLimitHigh
  )  external
  ```

* there are now two functions with these signatures:
  ```solidity
  function mintOptions(
       uint256[] positionIdList,
       uint128 positionSize,
       uint256 effectiveLiquidityLimit
  ) external nonpayable returns (bool)
  
  function mintOptionsITM(
      uint256[] positionIdList,
      uint128 positionSize,
      uint256 effectiveLiquidityLimit,
      int24 tickLimitLow,
      int24 tickLimitHigh
  ) external nonpayable returns (bool success)
  ```

I don't have access to the updated source code, so I am only guessing here, but from the API refactoring it is clear that minting/burning OTM and ITM are now clearly separated, with most probably disallowing an OTM call (mintOptions) to be executed ITM, and vice versa. I would be grateful to the sponsor for confirming or disproving my guess. The remediation in the form of refactoring is of course more preferable for code clarity, but please notice that the remediation proposed in this finding is also a valid one, in the sense that it prevents the user from losing more than they explicitly specified as slippage limits.

I was mistaken about this point, so I edited it away from the original PJQA reply... The docs I thought are new, are actually 2 years old, i.e. previously there were 2 functions (mintOptions and mintOptionsITM), which have been merged into a single function in the new version of the code. Thus the bug is freshly introduced, and unmitigated.

[c4-judge]

Picodes marked the issue as not a duplicate

[Picodes]

Tagging @dyedm1 for review.

@andrey-kuprianov thanks for your comment. My understanding is that this is by-design so isn't a Medium severity issue but falls within QA.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[dyedm1]

Yes, the TLH=TLL is a flag the users can set to disable slippage checks. The tick limits define an open interval anyway, so equal tick limits would be meaningless otherwise.

[andrey-kuprianov]

@Picodes , @dyedm1, I still don't understand how "mint/burn in-the-money" and "disable slippage checks" can possibly be together? Isn't it that the user has to pay whatever extra amount the option minted/burned is in the money? Then, as this finding outlines, the losses are unbounded without slippage checks, and such transaction is a honey-pot.

I would appreciate an explanation.

[andrey-kuprianov]

My understanding is that this is by-design so isn't a Medium severity issue but falls within QA.

That's what I am trying to explain here. Every bug is someone's design initially, until it's either fixed or exploited. I am showing that the design which disables slippage checks for a transaction which may be traded in-the-money, with unbounded losses for the user, is a bug, and not just a design choice.

[Picodes]

That's what I am trying to explain here. Every bug is someone's design initially, until it's either fixed or exploited. I am showing that the design which disables slippage checks for a transaction which may be traded in-the-money, with unbounded losses for the user, is a bug, and not just a design choice.

Please note that this is not a debate, as stated in multiple places in the code of conduct. For example:

"At the point that a judge provides a response, the conversation is over, unless you wish to provide additional facts which demonstrate inaccurate assumptions in the judge's response. Further pursuit of the conversation will result in having SR access suspended."

"Avoid engaging in any discussion and evaluation of issues submitted by yourself except to answer a question or provide additional context or clarification when requested by a judge or sponsor."

Of course, as I assume this is your issue, you're considering that it's worth Medium severity and would like to be awarded, but I don't see what additional facts your last 2 comments are bringing.

[Picodes]

Now, to answer your comments, I don't see what's unclear in the sponsor's design:

Due to this line setting tickLimitLow = tickLimitHigh would lead to the transaction reverting in all cases, so they use tickLimitLow = tickLimitHigh as a convention to have "infinite slippage". Of course, a user could not be aware of this and get arbed if he still uses tickLimitLow = tickLimitHigh but that would be a user mistake, which has never been Medium severity under C4's rules.

[andrey-kuprianov]

"Avoid engaging in any discussion and evaluation of issues submitted by yourself except to answer a question or provide additional context or clarification when requested by a judge or sponsor."

Dear @Picodes, I know all that, and I am trying to "avoid engaging in any discussion", please believe me. I only feel that the information that I have already provided has not been read carefully enough. I will provide this last bit of info for you to consider, as a response to your last comment, and then will let it for you to decide.

Due to this line setting tickLimitLow = tickLimitHigh would lead to the transaction reverting in all cases, so they use tickLimitLow = tickLimitHigh as a convention to have "infinite slippage". Of course, a user could not be aware of this and get arbed if he still uses tickLimitLow = tickLimitHigh but that would be a user mistake, which has never been Medium severity under C4's rules.

Specifically about "that would be a user mistake". I have written it already before: Nowhere in the user-facing documentation it is written such input is used to disable slippage. On the contrary:

• In the mintTokenizedPosition the docs say literally

  tickLimitLow: The lower price slippage limit when minting an ITM position (set to zero for no swapping)
  tickLimitHiigh: The higher price slippage limit when minting an ITM position (set to zero for no swapping)

  What happens when both tickLimitLow and tickLimitHiigh are set to zero? Exactly the case tickLimitLow = tickLimitHigh. Will it be "no swapping"? No, it will be unlimited swapping with disabled slippage checks.

• In the Minting a Panoption documentation (see also my previous reply above), the docs recommend setting tickLimitLow = tickLimitHigh = 0:

  Since our position is out of the money (OTM) ... We’re going to leave these off too since we’re not worried about slippage here.

I hope this demonstrates enough that the docs recommend triggering this bug. If the sponsor points me to the docs where it is explicitly written that setting tickLimitLow = tickLimitHigh disables the slippage, so this would be indeed a user mistake, and I will gladly admit that I am mistaken. But what I see so far, is that the docs provide the opposite information, and then this is a bug, I am sorry.

As I said, this is the last comment from me; please excuse me if I violated any boundaries. I am now leaving it to you to decide.

With best regards

[Picodes]

@andrey-kuprianov note that I do agree with you that the docs do not state this clearly and may even be misleading. But this isn't worth Medium severity under C4's rules: "function incorrect as to spec, issues with comments" are Low, and that's why I downgraded this report to Low instead of giving an "unsatisfactory" label.

[andrey-kuprianov]

I understand, and though disagree, I respect judge's decision.

For sponsor's benefit, as well as for future reference (in case of security incidents), I would like the following to be added to the protocol: I maintain my stance that a transaction with disabled slippage checks has no place in the mempool; no matter intentional or unintentional. I recommend to fix that as suggested in the finding.