DOS would happen in some instances of minting or burning an ITM option

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L435-L458
https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/README.md#L256-L257

Vulnerability details

Proof of Concept

First would be key to note that from the contest's readMe, protocol has stated that tokens that revert on zero value transfers are in scope, i.e https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/README.md#L256-L257

| [Revert on zero value approvals](https://github.com/d-xo/weird-erc20?tab=readme-ov-file#revert-on-zero-value-approvals) | ✅ Yes |

Now take a look at https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L435-L458

    function uniswapV3SwapCallback(
        int256 amount0Delta,
        int256 amount1Delta,
        bytes calldata data
    ) external {
        CallbackLib.CallbackData memory decoded = abi.decode(data, (CallbackLib.CallbackData));
        CallbackLib.validateCallback(msg.sender, FACTORY, decoded.poolFeatures);

        address token = amount0Delta > 0
            ? address(decoded.poolFeatures.token0)
            : address(decoded.poolFeatures.token1);

        uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta);

        // @audit
        SafeTransferLib.safeTransferFrom(token, decoded.payer, msg.sender, amountToPay);
    }

We can see that this is the function that is used and called by the pool during an ITM option mint/burn, however from this line in Uniswap's native implementation we can see that if no swaps were to occur and this function gets called, then both amount0Delta and amount1Delta will be 0, due to this check uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta); the amount1Delta ends being attempted to be sent which itself is 0 now since not all instances of mints/burns would include swaps, this would occur, and considering the first paragraph in this report proving that protocol supports tokens that revert on zero token transfers
, then this attempt of minting and burning is effectively DOS'd.

Impact

DOS to attempts of minting/burning options, cause if amount0Delta and amount1Delta is 0 the query by Uniswap to uniswapV3SwapCallback() function would fail.

Recommended Mitigation Steps

Consider checking both amount0Delta and amount1Delta before attempting to transfer, after checking if amount0Delta > 0 don't just blindly send amount1Delta if it can be 0, consider applying changing https://github.com/code-423n4/2024-04-panoptic/blob/833312ebd600665b577fbd9c03ffa0daf250ed24/contracts/SemiFungiblePositionManager.sol#L435-L458

    function uniswapV3SwapCallback(
        int256 amount0Delta,
        int256 amount1Delta,
        bytes calldata data
    ) external {
        CallbackLib.CallbackData memory decoded = abi.decode(data, (CallbackLib.CallbackData));
        CallbackLib.validateCallback(msg.sender, FACTORY, decoded.poolFeatures);

-        address token = amount0Delta > 0
-            ? address(decoded.poolFeatures.token0)
-            : address(decoded.poolFeatures.token1);

+        address token;
+        uint256 amountToPay;

-        uint256 amountToPay = amount0Delta > 0 ? uint256(amount0Delta) : uint256(amount1Delta);

+        if (amount0Delta > 0) {
+            token = address(decoded.poolFeatures.token0);
+            amountToPay = uint256(amount0Delta);
+        } else if (amount1Delta > 0) {
+            token = address(decoded.poolFeatures.token1);
+            amountToPay = uint256(amount1Delta);
+        } else {
+            return; // If both deltas are zero , no transfer needed since no swap was done
+        }


        SafeTransferLib.safeTransferFrom(token, decoded.payer, msg.sender, amountToPay);
    }

Alternatively, consider outrightly documenting that these tokens are not supported.

Assessed type

DoS

[c4-judge]

Picodes marked the issue as primary issue

[dyedm1]

dup #435 ?

[c4-judge]

Picodes marked the issue as duplicate of #435

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)