ready() is not Enough to Assume The Vault is Safe

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xb400710df222077848b27e5e57f5b13cd189bd8c4debe32dd8e4d54c38515881 Severity: high

Description: Description\ "setupMaster" can call 2 functions: "setConnection()" and "finishSetup()". setConnection() connect vaults to create pools. finishSetup() take away setupMaster role hence no new vaults can be connected to pools. As we can see from ready() function, in order for vault to be assumed safe, setupMaster should call finishSetup():

    /**
     * @notice Gives up short-term ownership of the vault. This makes the vault unstoppable.
     * @dev This function should ALWAYS be called before other liquidity providers deposit liquidity.
     * While it is not recommended, the escrow should ensure it is relativly safe trading through it (assuming a minimum output is set).
     */
    function finishSetup() external override {
        require(msg.sender == _setupMaster); // dev: No auth

        _setupMaster = address(0);

        emit FinishSetup();
    }

    /**
     * @notice View function to signal if a vault is safe to use.
     * @dev Checks if the setup master has been set to ZERO_ADDRESS.
     * In other words, has finishSetup been called?
     */
    function ready() external view override returns (bool) {
        // _setupMaster == address(0) ensures the pool is safe. The setup master can drain the pool!
        // _tokenIndexing[0] != address(0) check if the pool has been initialized correctly.
        // The additional check is there to ensure that the initial deployment returns false. 
        return _setupMaster == address(0) && _tokenIndexing[0] != address(0);
    }

Why this is so important? Because setupMaster can drain the pool as mentioned by NatSpec. It can create malicious vaults with malicious tokens and connect them to benign vault and steal the funds from users if users use the vault before finishSetup() called.

What I will argue next is that this is not enough.

Attack Scenario

1. setupMaster creates vaultA
2. setupMaster creates vaultB
3. setupMaster connects vaultA and vaultB
4. setupMaster call finishSetup in vaultA
5. ready() returns true.

Now let's examine if vault is really ready:

Scenario 1:

1. While vaultA is benign, vaultB is malicious because it contains fake tokens.
2. Because vaultA's ready() returns true:
3. Users deposits into it.
4. Malicious deployer use vaultB's malicious tokens to drain funds from vaultA.

Scenario 2:

1. vaultA and vaultB is benign.
2. Because vaultA's ready() returns true:
3. Users deposits into it.
4. Malicious deployer creates malicious vaultC with fake tokens and connects it to the vaultB.
5. Deployer use vaultC to inflate amounts in vaultB
6. Deployer use vaultB's inflated amounts to drain funds from vaultA.

How to prevent:

ready() function should not return just given vault's setupMaster check. It should also check if connected vaults' setupMaster also called finishSetup() or not.

[reednaa]

All vaults in a pool should be ready before the pool is ready. This is invalid under the common sense clause.

[kosedogus]

Just like you said:

All vaults in a pool should be ready before the pool is ready.

But the "ready()" function does not check this. It checks just the current vault and reports its boolean result. Hence what you are saying and what the code is doing are in complete contradiction.

If we want to approach it with common sense, what we can expect from our common sense can only be: relying on "ready()" functions return value before using vault. But although this is what common sense tell us, it is not the case. Even if "ready()" function in a vault returns "true", pool can be unsafe to use because of the reasons provided in the issue.

I want to propose the following solution for this:

1- Create a storage variable (boolean) indicating all connected vaults are ready. You can name it "finalized" for example. 2- Create a function that uses a relayer to check other vaults' position (their ready() functions' return). This function will set "finalized" variable to true if all connected vaults and also those vaults' connected vaults (2nd degree connection) are ready. (This can go more far, there might be 3rd degree connections or maybe more). 3- Instead of relying on "ready()" function's return, use "finalized" variable to be sure that vaults are safe to use. 4- Although optional, I also want to strongly suggest creating "isFinalized" modifier and using this modifier in all deposits and swaps.

Regarding 4th point above: If we look at Balancer's implementation we can see that Balancer checks if finalize() called (same as "finishSetup()") in swaps/deposits. So in the 4th point I tried to take inspiration from it to make the protocol even safer because I believe "ready()" is already an inspiration from that. But please don't try to focus on that point in order to invalidate the issue. Of course if you want to continue to use view functions as a check, it is your decision (which I don't think is optimal). But not implementing first 3 steps will make those attack vectors which I mentioned in the main issue easily usable and users won't be safe against them.

Best regards.

[reednaa]

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

• It significantly increases gas costs for users because we have to read an additional storage variable for each action.
• There are instances where it could be preferable to leave vaults unfinished.
• It very significantly increases the gas cost associated with creating a pool.
• It increases the overall complexity of the system
• It adds additional code to the contracts that would require us to reduce the number of optimiser runs which would further increase gas costs for interacting with the contract.
• Off-chain parties have a simple time reading the ready state and connections of a vault.

According to these arguments, the issue has been classified as won't fix.