The removedLiquidity variable has a potential risk of underflow vulnerability

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

In the _createLegInAMM function, the removedLiquidity variable may overflow. Once this happens, it can cause significant errors, resulting in a very large value for the removedLiquidity variable.

The variable removedLiquidity is referenced in the function _getPremiaDeltas to calculate the premium. A serious error in the variable 'removedLiquidity' will cause a significant error in the calculation of the user's premium.

Proof of Concept

The following is a partial code excerpt from the _createLegInAMM function:

        unchecked {
            // did we have liquidity already deployed in Uniswap for this chunk range from some past mint?

            // s_accountLiquidity 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 removing -ie.short selling LP positions
            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) {
                    removedLiquidity -= chunkLiquidity;
                }
            }

Note the statement: removedLiquidity -= chunkLiquidity; is contained in the unchecked{} code block, which means solidity will not check for integer overflow. If chunkLiquidity is greater than removedLiquidity, then removedLiquidity will become a very large number due to integer underflow.

Tools Used

Manual audit

Recommended Mitigation Steps

Add a conditional judgment before the statement removedLiquidity -= chunkLiquidity;

if (_isBurn) {
+	if(chunkLiquidity > removedLiquidity) revert('overflow');
	removedLiquidity -= chunkLiquidity;
}

Assessed type

Under/Overflow

[c4-judge]

Picodes marked the issue as duplicate of #516

[Picodes]

No valid scenario is described

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as partial-50

[c4-judge]

Picodes marked the issue as partial-25