hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 2 forks source link

There is no check against setting "minOut" to zero #68

Open hats-bug-reporter[bot] opened 8 months ago

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

Github username: -- Twitter username: 97Sabit Submission hash (on-chain): 0x3d7281573f7522cc131a8e44606498a2f7738e86c5a0d81018b020878d40b42e Severity: medium

Description: Description\ Users are at liberty to set minOut to any amount they want in the localSwap function.

But the rationale behind minOut is to serve as a protection against getting zero amount from a swap and frontrunning attack.

However, there is no check in place to ensure that minOut cannot be set to zero. Users can knowingly or mistakenly set minOut to zero. This exposes users to frontrunning attack and getting zero amount from a swap.

  1. Proof of Concept (PoC) File

    https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystVaultVolatile.sol#L565-L572

reednaa commented 8 months ago

Some users might want to set minout to zero.

If we didn't accept zero as a valid minout, they could easily circumvent the minout by setting it to 1. It is true that the explicit way is more verbose.

The latter recommendation is a dublicate of https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/13

ololade97 commented 8 months ago

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.

The above is the relevant excerpt from issue #13. However, it's also not similar to this issue.

It's true some users may want to set minOut to zero. But they are at the risk of frontrunning.