hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

Other
1 stars 2 forks source link

The user can deploy vault by exceeding the limit of max asset which will revert in middle of transaction #35

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

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

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x59d80e239d0514de0c87c8157aa0965f9c3ed12fef2c62d1e13dc0863d665b74 Severity: low

Description: Description\

Anyone can call CatalystFactory.deployVault() to deploy the catalyst vault with user defind assets which is transferred to vault before the set and configure of vault takes place.

However, there is one issue which will always revert the transactions or failure of vault deployment when the user exceeds the MAX_ASSETS. The MAX_ASSETS is 3 for catalyst vaults which means users creating the vault with 4 or more assets the transaction will always revert due to this check.

The users should be restricted to 3 assets while calling the deployVault(). This will be known to user by advance if they exceed MAX_ASSETS otherwise they will know just before the end of transaction after sending the assets(can be 10) to vault address. This is indeed bad user experience and loss of gas fee for users if the transaction reverting just before end of function.

Recommendation to fix

Explicitely check the passed assets must be less than MAX_ASSETS while calling CatalystFactory.deployVault().

reednaa commented 6 months ago

The MAX_ASSETS is set by the template rather than the factory. So if someone wanted a vault with more than 3 assets, then can change the constant in the vault (though it increases the gas cost) and use that template. If we enforced this on the factory, we would have to also deploy a new factory.

We intend to use the same factory for all vaults since it simplifies event gathering for Ethers v5.