Panoptic - Udsen's results

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L978-L980 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L985-L994 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L998-L1000

Vulnerability details

Impact

In the SemiFungiblePositionManager._createLegInAMM function the removedLiquidity is decreased if the long option position is burned for the respective leg of the option position (positionKey) as shown below:

            if (_isBurn) {
                removedLiquidity -= chunkLiquidity; //@audit-info - since we are burning (closing) a long position, the amount of removed liquidity should decrease by the liquidity chunk of the short position
            }

The issue here is that the removedLiquidity can underflow since there is no conditional check to verify that the removedLiquidity > chunkLiquidity. Hence if the chunkLiquidity > removedLiquidity occurs then the removedLiquidity will underflow and will be assigned a very large value close to the type(uint128).max.

In the minting of the long positions (when isLong == 1) the updatedLiquidity is updated only after verifying that the startingLiquidity >= chunkLiquidity condition since no underflow can occur in the updatedLiquidity = startingLiquidity - chunkLiquidity; calculation as shown below:

            if (startingLiquidity < chunkLiquidity) { //@audit-info - check if there is enough liquidity owned by an account to be removed and if not revert
                revert Errors.NotEnoughLiquidity(); //@audit-info - revert if the account does not have enough liquidity in the Uniswap pool to be removed from the pool
            } else {
                updatedLiquidity = startingLiquidity - chunkLiquidity; //@audit-info - subtract the liquidity chunk from the starting liquidity and assign to updatedLiquidity
            }

But this same underflow protection is not implemented while closing a long positions due to the lack of the removedLiquidity > chunkLiquidity conditional check as described above. Hence if the removedLiquidity underflows and is assigned with a very large value (close to type(uint128).max) then the s_accountLiquidity[positionKey] state mapping will hold erroneous value with regard to the removedLiquidity value in the state of the mapping. This will further provide erroneous representation as to how much long positons are minted in the UniswapV3 AMM Pool since the removedLiquidity is increased by the respective liquidityChunk amount for every long position minted. Hence the state will be broken in the s_accountLiquidity[positionKey] mapping for the amount of long positions minted for the specific position (positionKey) and could provide erroneous values for third party tools and protocols which use the SemiFungiblePositionManager.getAccountLiquidity function to retrieve the liquidity associated with a given position.

Proof of Concept

                if (_isBurn) {
                    removedLiquidity -= chunkLiquidity;
                }

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L978-L980

                if (startingLiquidity < chunkLiquidity) {
                    // the amount we want to move (liquidityChunk.legLiquidity()) out of uniswap is greater than
                    // what the account that owns the liquidity in uniswap has (startingLiquidity)
                    // we must ensure that an account can only move its own liquidity out of uniswap
                    // so we revert in this case
                    revert Errors.NotEnoughLiquidity();
                } else {
                    // we want to move less than what already sits in uniswap, no problem:
                    updatedLiquidity = startingLiquidity - chunkLiquidity;
                }

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L985-L994

                if (!_isBurn) {
                    removedLiquidity += chunkLiquidity;
                }

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L998-L1000

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to implement the removedLiquidity > chunkLiquidity conditional check when burning a long positon, before the removedLiquidity -= chunkLiquidity calculation is performed. Hence the recommended modification to the code snippet is given below:

            if (_isBurn) {
                if (removedLiquidity < chunkLiquidity) {
                    revert Errors.NotEnoughLiquidity();
                } else {
                removedLiquidity -= chunkLiquidity;
                }
            }

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/tokens/ERC1155Minimal.sol#L90-L118 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/tokens/ERC1155Minimal.sol#L128-L171

Vulnerability details

Impact

The documentation of the panoptic protocol states that the SemiFungiblePositionManager contract should comply with the ERC1155 token standard as shown below:

EIP Compliance
SemiFungiblePositionManager: Should comply with ERC1155

And the SemiFungiblePositionManager contract inherits from the ERC1155 contract from the ERC1155Minimal.sol which is a minimal implementation of the ERC1155 standard. But the issue is even the minimal implementation of the ERC1155 functions in the ERC1155Minimal.sol contract does not comply with the EIP1155 standard as explained below:

The ERC1155Minimal.safeTransferFrom function and the ERC1155Minimal.safeBatchTransferFrom functions do not check whether the to address is equal to the address(0). This could lead to the loss of ERC1155 tokens if the address(0) is passed in as the to address.

Further the EIP1155 mentions the following related to the safeTransferFrom function rules and the safeBatchTransferFrom function rules, as shown below:

MUST revert if _to is the zero address

If the safeTransferFrom and safeBatchTransferFrom functions of the ERC1155Minimal.sol is used by the panoptic protocol by passing in the to address as address(0) this could lead to loss of funds to the users of the panoptic protocol who are minting or burning option positions.

Proof of Concept

    function safeTransferFrom(
        address from,
        address to,
        uint256 id,
        uint256 amount,
        bytes calldata data
    ) public {
        if (!(msg.sender == from || isApprovedForAll[from][msg.sender])) revert NotAuthorized();

        balanceOf[from][id] -= amount;

        // balance will never overflow
        unchecked {
            balanceOf[to][id] += amount;
        }

        afterTokenTransfer(from, to, id, amount);

        emit TransferSingle(msg.sender, from, to, id, amount);

        if (to.code.length != 0) {
            if (
                ERC1155Holder(to).onERC1155Received(msg.sender, from, id, amount, data) !=
                ERC1155Holder.onERC1155Received.selector
            ) {
                revert UnsafeRecipient();
            }
        }
    }

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/tokens/ERC1155Minimal.sol#L90-L118

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] calldata ids,
        uint256[] calldata amounts,
        bytes calldata data
    ) public virtual {
        if (!(msg.sender == from || isApprovedForAll[from][msg.sender])) revert NotAuthorized();

        // Storing these outside the loop saves ~15 gas per iteration.
        uint256 id;
        uint256 amount;

        for (uint256 i = 0; i < ids.length; ) {
            id = ids[i];
            amount = amounts[i];

            balanceOf[from][id] -= amount;

            // balance will never overflow
            unchecked {
                balanceOf[to][id] += amount;
            }

            // An array can't have a total length
            // larger than the max uint256 value.
            unchecked {
                ++i;
            }
        }

        afterTokenTransfer(from, to, ids, amounts);

        emit TransferBatch(msg.sender, from, to, ids, amounts);

        if (to.code.length != 0) {
            if (
                ERC1155Holder(to).onERC1155BatchReceived(msg.sender, from, ids, amounts, data) !=
                ERC1155Holder.onERC1155BatchReceived.selector
            ) {
                revert UnsafeRecipient();
            }
        }
    }

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/tokens/ERC1155Minimal.sol#L128-L171

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to add the following line of code to the safeTransferFrom and safeBatchTransferFrom functions of the ERC1155Minimal.sol contract to perform the input validation on the passed in to address and from address. This will enable the ERC1155Minimal.sol contract to comply with the EIP1155 token standard correctly (as mentioned in the documentation).

    if (to == address(0)) {
        revert ERC1155InvalidReceiver(address(0));
    }
    if (from == address(0)) {
        revert ERC1155InvalidSender(address(0));
    }

Assessed type

Other