A deployed vault can be deployed again.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x08845b1c7713d61ed63e6b4b5abbcbb128841c5beee32489382a954d6241c4a7 Severity: medium

Description: Description\ A deployed vault can be deployed again. This is due to a lack of validation to verify that the vault about to be deployed hasn't already been deployed before deploying it. This will lead to duplicate vaults.

Attack Scenario\ An adversary can exploit the vulnerability to create bad duplicates of an already exisiting good vault in order to tarnsih the vault's reputation. It can also be done to trick users into interacting with bad vaults.

Proof of Concept (PoC) File

Add this test to DeployVault.t.sol and run forge test --mt test_deploy_twice

    function test_deploy_twice(uint16[2] memory weights_) external {
        vm.assume(weights_[0] > 0);
        vm.assume(weights_[1] > 0);
        address[] memory tokens = getTokens(2);

        uint256[] memory init_balances = new uint256[](2);
        init_balances[0] = 10000 * 10**18;
        init_balances[1] = 5000 * 10**18;

        uint256[] memory weights = new uint256[](2);
        weights[0] = uint256(weights_[0]);
        weights[1] = uint256(weights_[1]);

        approveTokens(address(catFactory), tokens, init_balances);
        t_deploy_volatile(tokens, init_balances, weights);

        approveTokens(address(catFactory), tokens, init_balances);
        t_deploy_volatile(tokens, init_balances, weights); // deploying the same vault again
    }

Poc file attached below.

Proposed fix

Verify that the vault hasn't been deployed before deploying it.

Files:

• DeployVault.t.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmPp6Q72vtoTngvgVBKdwx3Tw5jAe8uYbA6U2FYauYPDWu)

[reednaa]

It might be that the vaults are connected to different vaults on different chains, how would you check against that?

[PlamenTSV]

This is more of a social engineering type of issue, everybody can check for themselves the address of the legit vault they want to use.

[0xisaacc]

Hi @reednaa I think we should verify that the vault hasn't been deployed on that chain before deploying it. We can use this bool in function deploy for it.

isCreatedByFactory[chainInterface][vault] = true;

https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystFactory.sol#L127

If isCreatedByFactory is already set to true on that chain, then it has been deployed and we can revert with an error message e.g, "vault already deployed!"

[reednaa]

Sorry, I don't understand.

[0xisaacc]

It might be that the vaults are connected to different vaults on different chains, how would you check against that?

Let's recap The core problem is that the same vault can be deployed again on the same chain.

• Your point is that the vault can be connected to different vaults on different chains.

• I suggest we start by fixing the core issue, preventing duplicate vaults from existing on the same chain.

• Thiis will also prevent malicious actors from deploying duplicate vaults of the original vault on each chain as only the first vault deployed by the genuine user on each chain will be valid

• That's what I think we should do..

[reednaa]

My argument is:

The vault that is WETH, USDC could be a single pool (not connected to any other vaults) or be in a larger pool, say connected to a vault on Polygon which is WMATIC, USDT. There is no way to know from a single chain.

How would you check the difference between these vaults?

[0xisaacc]

Let's use the vault's name and symbol

string memory name,
string memory symbol,

https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystFactory.sol#L73

Let the name and symbol of each vault be unique to that vault.

[reednaa]

Wouldn't that allow someone to DoS pool creation? And, what if another vault matches the name but not symbol? They could even just add a space at the end to make them seem almost exact.

[0xisaacc]

It seems there are many edge cases around this issue so it would be impossible to suggest just one fix to address all of them. If you have a better fix in mind, let's hear it.

[reednaa]

My "fix" is not fixing. Like many things regarding Catalyst, sane off-chain UIs will show this for a user and as such it is a non-issue.

[0xisaacc]

I suggest we go with my fix then, as both suggestions of:

• Using unique name and symbols for each vailt
• Ensuring that the vault hasn't been deployed before deploying it .

Is a good step in the right direction of securing the protocol and users.

[reednaa]

We have decided to classify this issue as won’t fix. Our decision is based on the following arguments:

• There is no protection granted by fixing the issue
• It increases gas cost
• Provides a vector of DoS.

According to these arguments, the issue is won’t fix.