Malicious vault owners can make users loose funds by frontrunning their txns

[hats-bug-reporter]

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

Description:
Description
The Vault contract contains a _vaultFee parameter which can be changed by _feeAdministrator. The _feeAdministrator address is chosen by the vault deployer at the time of deployment.

Currently the vault implementation allows _vaultFee to be upto 100%.

    /// @notice Sets a new vault fee, taken from input amount.
    function _setVaultFee(uint256 fee) internal {
        require(fee <= 1e18);  // dev: VaultFee is maximum 100%.
        _vaultFee = fee;
        emit SetVaultFee(fee);
    }

https://github.com/catalystdao/catalyst/blob/main/evm/src/CatalystVaultCommon.sol#L347-L351

This exposes a vulnerability by which malicious vault owners can forcefully make users pay more fee than intended (upto 100% of the deposit/swap amount).

The _vaultFee is currently used in these functions:

• depositMixed()
• localSwap()
• sendAsset()
• sendAssetFixedUnit()

Attack Scenario
An example attack can look like this:

1. A user initiates a cross chain asset call from ChainA to ChainB.
2. The _feeAdministrator sees this txn in mempool and frontruns it to set fee to 90%.
3. User's sendAsset txn gets executed. Here only 10% of user's original amount will be added to vault escrow accounting, the rest 90% will be considered as fee.
4. Relayer invokes the processPacket call on ChainB. Here the token transfer will fail as the amount is less than what user originally agreed upon (minOutAmt validation).
5. Relayer invokes the processPacket call on ChainA for acknowledgement. Now only 10% amount (escrowed in Step3) gets refunded to user.

In this attack user lost 90% of his original funds.

I am aware that catalyst owner can change the _feeAdministrator but since the owner will be a Timelock there will always be a delay in this rescue.

Due to this issue the security of Catalyst cross chain AMM will be fully dependent upon the mercy and goodwill of vault deployers.

Attachments

1. Proof of Concept (PoC) File
   NA

2. Revised Code File (Optional)

   Consider taking a maxAcceptedFee as input for the sendAsset call and if the _vaultFee is greater than this value then revert.

[reednaa]

dublicate of #13

[akshaysrivastav]

Hey @reednaa

Here #13 (comment) you mentioned that users can prevent themselves using minOutAmt.

But this report shows how even after using a minOutAmt the user still faces fund loss. I believe this report should be considered valid.

[reednaa]

It is a specifically a dublicate of #13 (comment).
The migration is as I mention: Checking if U is larger than a certain number.