Protocol can benefit illegally by draining the pool gradually through _vaultFee

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0xd4b93bd899e8d4cd368be5a784be39dc05bfdec26e681b4b0450d3bf9e1486bd Severity: high

Description: Description\ The localSwap function allows the "fromAsset" and the "toAsset" to be one same token. For every swap, a _vaultFee is removed. A _vaultFee could either be <=1e18.

  require(fee <= 1e18);  // dev: VaultFee is maximum 100%.

And _vaultFee is removed from the "fromAsset".

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

Invariably, _vaultFee will be removed from the "toAsset" in the pool since "fromAsset" and "toAsset" are the same.

As a result, a malicious protocol or a malicious user can swap same asset in order for the _vaultFee to be removed from the pool and transferred to the protocol.

For example, a malicious protocol or a malicious user (who wants to drain the pool and benefit the protocol) can:

• Deposit same "fromAsset" and "toAsset" into the contract say 100DAI
• Set minOut to 100DAI to avoid loss
• Get back 100DAI
• And pay _vaultFee to the protocol from the "toAsset".

Note that the "toAsset" in the pool will keep decreasing and not increase as a result.

1. Proof of Concept (PoC) File

   https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystVaultCommon.sol#L348

https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystVaultAmplified.sol#L807

[reednaa]

• Deposit same "fromAsset" and "toAsset" into the contract say 100DAI
• Set minOut to 100DAI to avoid loss
• Get back 100DAI
• And pay _vaultFee to the protocol from the "toAsset".

Set minOut to 100DAI to avoid loss

That is not how the minout works.

[ololade97]

minOut is a user input based on the code. It only works in accordance wit what a user specifies.

[reednaa]

Please provide PoC.

[ololade97]

But the vulnerability is too obvious to require a PoC. Swapping one same token is allowed and this vulnerability is bound to happen.

[reednaa]

Without a PoC I have determined it to be invalid. You have 2 options:

1. Create a PoC. You can base it of exampleTest. It has documentation in the readme: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/main/evm/test/ExampleTest.t.sol
2. Dispute my verdict with Hats.

[ololade97]

I understand. But why invalid without a reason?

[reednaa]

The issue does not make sense. As in: I wrote the code for Catalyst but cannot understand any part of this issue.

I don't know how to provide you with any reason except that

• Deposit same "fromAsset" and "toAsset" into the contract say 100DAI
• Set minOut to 100DAI to avoid loss
• Get back 100DAI
• And pay _vaultFee to the protocol from the "toAsset".

1. Deposit? This is a local swap? I assume you mean

   function localSwap(
       DAI,
       DAI,
       100*10**18,
       100*10**18
   )

2. You cannot swap 100 DAI for 100 DAI. The implementation contains an optimisation that causes this to fail. It will instead return (assume 1000 tokens in the vault) 1000 * 100/(1000+100) = 90 tokens. This is assuming a vault fee of 0.
3. No, you do not get back 100 DAI.
4. Vault fee would be subtracted from the 90 tokens such that you get even less. I don't understand how you can come to any other conclusion.

This is my final verdict. I won't accept a PoC anymore for this issue and you need to raise it to Hats if you want a second opinion.

[ololade97]

I apologise if I've crossed the line. I will work on a PoC and raise it to Hats if anything meaningful comes out of it.