xiaoming90 - removeVault did not remove the vault from _vaultsByType mapping

[sherlock-admin]

xiaoming90

medium

removeVault did not remove the vault from _vaultsByType mapping

Summary

The removeVault function did not remove the vault from _vaultsByType mapping. As a result, any internal contract or external protocols integrating with Tokemak that rely on the LMPVaultRegistry.listVaultsForType might break as incorrect vault information is returned.

Vulnerability Detail

When a new vault is added, they are added to the _vaultsByAsset and _vaultsByType mappings. However, when a vault is removed, they are only removed from the _vaultsByAsset mapping, not from the _vaultsByType mapping.

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRegistry.sol#L46

File: LMPVaultRegistry.sol
46:     function addVault(address vaultAddress) external onlyUpdater {
47:         Errors.verifyNotZero(vaultAddress, "vaultAddress");
48: 
49:         ILMPVault vault = ILMPVault(vaultAddress);
50: 
51:         address asset = vault.asset();
52:         bytes32 vaultType = vault.vaultType();
53: 
54:         if (!_vaults.add(vaultAddress)) revert VaultAlreadyExists(vaultAddress);
55:         //slither-disable-next-line unused-return
56:         if (!_assets.contains(asset)) _assets.add(asset);
57: 
58:         if (!_vaultsByAsset[asset].add(vaultAddress)) revert VaultAlreadyExists(vaultAddress);
59:         if (!_vaultsByType[vaultType].add(vaultAddress)) revert VaultAlreadyExists(vaultAddress);
60: 
61:         emit VaultAdded(asset, vaultAddress);
62:     }
63: 
64:     function removeVault(address vaultAddress) external onlyUpdater {
65:         Errors.verifyNotZero(vaultAddress, "vaultAddress");
66: 
67:         // remove from vaults list
68:         if (!_vaults.remove(vaultAddress)) revert VaultNotFound(vaultAddress);
69: 
70:         address asset = ILMPVault(vaultAddress).asset();
71: 
72:         // remove from assets list if this was the last vault for that asset
73:         if (_vaultsByAsset[asset].length() == 1) {
74:             //slither-disable-next-line unused-return
75:             _assets.remove(asset);
76:         }
77: 
78:         // remove from vaultsByAsset mapping
79:         if (!_vaultsByAsset[asset].remove(vaultAddress)) revert VaultNotFound(vaultAddress);
80: 
81:         emit VaultRemoved(asset, vaultAddress);
82:     }

Any internal contract or external protocols integrating with Tokemak that relies on the LMPVaultRegistry.listVaultsForType will receive the incorrect vault information.

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRegistry.sol#L102

File: LMPVaultRegistry.sol
102:     function listVaultsForType(bytes32 _vaultType) external view returns (address[] memory) {
103:         return _vaultsByType[_vaultType].values();
104:     }

Impact

Any internal contract or external protocols integrating with Tokemak that relies on the LMPVaultRegistry.listVaultsForType might break as incorrect vault information is returned.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRegistry.sol#L46

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVaultRegistry.sol#L102

Tool used

Manual Review

Recommendation

Consider the following change to remove the vault from the _vaultsByType mapping.

function removeVault(address vaultAddress) external onlyUpdater {
    Errors.verifyNotZero(vaultAddress, "vaultAddress");

    // remove from vaults list
    if (!_vaults.remove(vaultAddress)) revert VaultNotFound(vaultAddress);

    address asset = ILMPVault(vaultAddress).asset();

    // remove from assets list if this was the last vault for that asset
    if (_vaultsByAsset[asset].length() == 1) {
        //slither-disable-next-line unused-return
        _assets.remove(asset);
    }

    // remove from vaultsByAsset mapping
    if (!_vaultsByAsset[asset].remove(vaultAddress)) revert VaultNotFound(vaultAddress);
    
+    // remove from _vaultsByType mapping
+    if (!_vaultsByType[asset].remove(vaultAddress)) revert VaultNotFound(vaultAddress);

    emit VaultRemoved(asset, vaultAddress);
}

Duplicate of #674