Blocking of Default Vault Implementation in changeImplementationForVault Function

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/Core.sol#L183
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/entities/CoreLib.sol#L157
https://github.com/code-423n4/2024-07-karak/blob/f5e52fdcb4c20c4318d532a9f08f7876e9afb321/src/entities/CoreLib.sol#L118

Vulnerability details

Vulnerability Details:

The changeImplementationForVault function allows the owner to change the implementation of a specific vault. The only two requirements are that the newVaultImpl must be an allowlisted implementation, and the previous vault cannot be the zero address.

    /// @notice Allows the owner to change the implementation of a specific vault
    /// @param vault: The address of the vault to change the implementation of
    /// @param newVaultImpl: The address of the new vault implementation. Must be a allowlisted implementation
    function changeImplementationForVault(address vault, address newVaultImpl) external onlyOwner {
        CoreLib.Storage storage self = _self();
        if (!self.isVaultImplAllowlisted(newVaultImpl)) revert VaultImplNotAllowlisted();
        // Don't let the admin change the implementation from address(0) to something else
        // bypassing the deployVault flow
        if (self.vaultToImplMap[vault] == address(0)) revert VaultNotAChildVault();

        self.vaultToImplMap[vault] = newVaultImpl;
        emit UpgradedVault(newVaultImpl, vault);
    }

The isVaultImplAllowlisted function checks the implementation against the allowlistedVaultImpl mapping and the current default vault implementation, which is self.vaultImpl. If the new implementation is neither of these, it returns false, causing the changeImplementationForVault function to revert.

    function isVaultImplAllowlisted(Storage storage self, address implementation) internal view returns (bool) {
        return self.allowlistedVaultImpl[implementation] || implementation == self.vaultImpl;
    }

The problem is that this implementation blocks the changeImplementationForVault function from setting the implementation to the default DEFAULT_VAULT_IMPLEMENTATION_FLAG. As seen in the deployVaults function, this is a valid implementation and allows changing all standard vaults to a new implementation.

Therefore, if the protocol wants to change a vault's implementation to the default vault implementation, it cannot do so and would have to manually update it every time the default is changed.

Impact:

The current implementation restricts the ability to update vaults to the default vault implementation using the changeImplementationForVault function. This requires manual updates to each vault whenever the default implementation is changed.

Tools Used:

• Manual analysis

Recommendation:

Modify the changeImplementationForVault function to allow it to set the new implementation to the default vault implementation flag.

Assessed type

Other

[c4-judge]

MiloTruck changed the severity to QA (Quality Assurance)

[MiloTruck]

This is low severity at best.

The impact here is if the owner has called changeImplementationForVault() to override a vault's implementation to a custom address before, it cannot be automatically updated with changeStandardImplementation(). changeImplementationForVault() can still be called to update the vault's implementation when needed.

Given that vault upgrades are not expected to occur often, this impact does not warrant medium severity.

[c4-judge]

MiloTruck marked the issue as grade-b

[devdks25]

Fix: https://github.com/karak-network/karak-restaking/pull/384