A user can call the depositMixed function and mint shares without depositing any token

[hats-bug-reporter[bot]]

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

Description: Description\ The depositMixed function breaks when token deposited is 0:

if (token == address(0)) break;

The break keyword only ends the loop. But the code outside the for loop continues to run. And outside the for loop, there's no other check that stops the code from running and minting vaultTokens to a user.

An attacker would:

• Deposit a non-existing token
• Set the minOut to zero
• The break keyword ends the for loop
• The attacker deposits nothing to the contract as a result
• The code outside the for loop continues to run
• vaultTokens is calculated and minted to the attacker

1. Proof of Concept (PoC) File

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

2. Revised Code File (Optional)

The code should revert when token == 0 and not break.

[reednaa]

When break is called in this sequence: https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultAmplified.sol#L324-L325

It implies that the vault doesn't have more tokens. The sequence here is not user input.

[ololade97]

Assuming the vault doesn't have more tokens and breaks, it's only the for loop that breaks. The rest of the code runs and share is minted.

[reednaa]

It is an address check not integer. The number of tokens is uncorrolated of that statement.

[ololade97]

It implies that the vault doesn't have more tokens.

The initial response prompted me to write this - "Assuming the vault doesn't have more tokens...".

The issue here is what happens if the for loop breaks and fund is not transferred to the contract? This is my argument.

[reednaa]

Your attack flow as described here

An attacker would:

• Deposit a non-existing token
• Set the minOut to zero
• The break keyword ends the for loop
• The attacker deposits nothing to the contract as a result
• The code outside the for loop continues to run
• vaultTokens is calculated and minted to the attacker

Does not work because you cannot deposit a token that is part of the vault, nor can you setup the vault such that it has a undeployed token. That is because the vault loops over the tokens that it was created with. When it reaches the end of the list, it stops the for loop (break), the doesn't go over any more tokens.

It is very healthy to try to create PoC for these kinds of attacks. Many others have done it with simplar and more complicated issues, and you can even draw inspiration from them to write your own PoC or playgrounds.

[ololade97]

Thanks. I will provide a PoC.

[ololade97]

You're right. Played with the for loop. I could see that even if the loop breaks, the rest of the code in the for loop continues. Andd thanks for your patience.