Lesser vaultTokens than the specified minOut can be minted because of insufficient check

[hats-bug-reporter[bot]]

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

Description: Description\ The depositMixed function allows users to deposit multiple tokens and mint vault tokens in return. It takes a minOut parameter which specifies the minimum number of vault tokens to mint.

However, there is a bug in how minOut is checked - it is possible to mint fewer vault tokens than the specified minOut, without the transaction reverting.

Attack Scenario\ minOut is meant to protect users from price slippage by reverting if the minted tokens are below the minimum

The check if (minOut > vaultTokens) revert ReturnInsufficient(vaultTokens, minOut); compares minOut to the calculated vaultTokens

However, it is checking minOut > vaultTokens instead of vaultTokens < minOut

So if vaultTokens are less than minOut, the check still passes when it should revert.

Attachments https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystVaultVolatile.sol#L350-L418

1. Proof of Concept (PoC) File

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

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

2. Recommendation

if (vaultTokens < minOut) {
  revert ReturnInsufficient(vaultTokens, minOut); 
}

[reednaa]

minOut > vaultTokens and vaultTokens < minOut equivalent. (3 > 2 === 2 < 3). Please show me how you can circumvent the check.

I will tag invalid until then.

[ololade97]

vaultTokens is the total amount the contract can send and will send to a user.

minOut is the lowest amount expected by a user.

Both checks are important. However, the contract only checked the first one - if there is enough tokens in the contract to send.

On this equation: minOut > vaultTokens and vaultTokens < minOut equivalent. (3 > 2 === 2 < 3).

I'd rather not check minOut against vaultTokens to know if there's enough tokens in the contract. I'd check if vaultTokens is not 0.

Then check vaultTokens against minOut to satisfy user gets not less than minOut:

if (vaultTokens < minOut) { revert ReturnInsufficient(vaultTokens, minOut); }

[reednaa]

https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultVolatile.sol#L402-L404

Vault tokens refers to the number of tokens to mint to the user in this case. Furthermore, in this case there is no need to check the tokens in the pool since it is minting tokens rather than sending. https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultVolatile.sol#L410-L411

[ololade97]

But there's no check that ensures a user doesn't get less than the minOut a user specifies.

[reednaa]

https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultVolatile.sol#L402-L411

Say vaultTokens = 499 Minout = 500 then:

 uint256 vaultTokens = 499

 if (500 > 499) revert ReturnInsufficient(499, 500); 

 _mint(msg.sender, 499);

===

 uint256 vaultTokens = 499

 if (true) revert ReturnInsufficient(499, 500); 

Do you disagree?

[ololade97]

You're right. Thanks.