calling finishSetup() before completing setup can render the vault as useless.

[hats-bug-reporter]

Github username: @https://github.com/betharavikiran
Twitter username: @ravikiranweb3
Submission hash (on-chain): 0xb15909f47f8166e8c7bebc8d24d6ac012d4a993e55d3ee89ddd1ea6d01e93b4e
Severity: low

Description:
Description
SetupMaster is a temporary entitlement provided to an account to configure the vault. The role of setupMaster
is to configure the connections for the vault. Calling finishSetup() will revoke the temporary entitlement and
the vault becomes unstoppable.

The vulnerability is the call to finishSetup() before calling setConnection() will revoke the entitlements
and the vault will not be functional if connections were not set.

The finishSetup can be called by setupMaster once and lose the entitlements permanently.

function finishSetup() external override {
        require(msg.sender == _setupMaster); // dev: No auth

        _setupMaster = address(0);

        emit FinishSetup();
    }

The setConnection can be called only by setupMaster, if there connections are not set, key functionality will
not work.

function setConnection(
        bytes32 channelId,
        bytes calldata toVault,
        bool state
    ) external override {
        require(msg.sender == _setupMaster); // dev: No auth
        require(toVault.length == 65);  // dev: Vault addresses are uint8 + 64 bytes.

        _vaultConnection[channelId][toVault] = state;

        emit SetConnection(channelId, toVault, state);
    }

All the below listed function on both Vault templates will not work rendering the vault as useless.

The problem exists for both types of vaults.

• sendAsset
• sendAssetFixedUnit
• receiveAsset
• sendLiquidity
• receiveLiquidity
• releaseUnderwriteAsset

Attack Scenario\

• Deploy a new vault using the factory's deploy function.
• Call finishSetup() function on the vault.

The vault will not work as expected.

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

In the setup() function of the vault , nominate the chains that will be configured for the vault.

Before revoking the entitlements, the nominimated
chain connections should be configured.

When calling finishSetup(), the logic checks to ensure that configured chain related connections are configured. If not, the finishSetup will revert allowing the temporary admin to configure the vault properly.

[reednaa]

Given the function name finishSetup I personally believe it is pretty clear that you should only call it after you have set the connections.
The vault are very close to their maximum size and as such we can't add a lot logic to these checks. Furthermore, the setup functions are stack limited and we would have to convert the setup into a struct which would further increase gas cost, likewise, storing the connections to be setup would also increase gas cost significantly. (potential many zero to non-zero just to set to zero again).

What if the user misinput a connection in the beginning, would that render the vault invalid? Say they specified a connection to Polygon, BSC, and Base but afterwards decided to only set to Polygon and Base?

There is no fix to this issue that doesn't create new problems and the function is clearly doing its intended purpose.

[betharavikiran]

What is the time window to call finishSetup() in that case ?
Unless this function is called, the vault is not ready to be used.

so, can he continue to decide for long time ? It has to be planned and executed almost immediately after deployment.
Otherwise, the vault will not be ready to use due to the below.

But, yes, suggestion above does not cover the scenario you described above.

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);
}

[reednaa]

What is the time window to call finishSetup() in that case ?

There is no time window. Someone could technically run the vault forever without calling finishSetup. This would let them modify the connections if they wanted. Within a certain (trusted) group of people this might be desired.

[betharavikiran]

Got it. The below function will indicate that the vault is not ready or safe to use. Is that ok? /** * @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); }

…

On Wed, 7 Feb, 2024, 6:13 pm Alexander, ***@***.***> wrote: What is the time window to call finishSetup() in that case ? There is no time window. Someone could technically run the vault forever without calling finishSetup. This would let them modify the connections if they wanted. Within a certain (trusted) group of people this might be desired. — Reply to this email directly, view it on GitHub <#83 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACTGZVOYWBVYJJXMDGL5SGDYSNZILAVCNFSM6AAAAABC2PMEQOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZRHE3TAOJXGI> . You are receiving this because you commented.Message ID: <hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/83/1931970972 @github.com>

[reednaa]

It will indicate it for each specific vault. To figure out if a pool is final, you have to check all connected vaults.

[betharavikiran]

Yeah, basically that means until finishSetup() is called, those vaults will not be returning as ready. Anyway, if there is any significance of ready(), then review this issue from that context, else you can consider the reported issue as invalid like it was marked. thank you

…

On Wed, Feb 7, 2024 at 7:42 PM Alexander ***@***.***> wrote: It will indicate it for each specific vault. To figure out if a pool is final, you have to check all connected vaults. — Reply to this email directly, view it on GitHub <#83 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACTGZVLOEREEKLKGSR6V2ALYSODWFAVCNFSM6AAAAABC2PMEQOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZSGEZTIMZWGY> . You are receiving this because you commented.Message ID: <hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/issues/83/1932134366 @github.com>