Submodules can be installed but never removed causing bloat and potential DOS over time

Summary

Submodules can be installed and upgraded but they can never be uninstalled. This leads to unavoidable bloat as submodules are added and retired but can never be removed.

Vulnerability Detail

Submodules.sol#L81-L132

SubKeycode[] public submodules;

/// @notice Mapping of SubKeycode to Submodule address.
mapping(SubKeycode => Submodule) public getSubmoduleForKeycode;

function installSubmodule(Submodule newSubmodule_) external permissioned {
    ...
}

function upgradeSubmodule(Submodule newSubmodule_) external permissioned {
    // Validate new submodule and get its subkeycode
    SubKeycode subKeycode = _validateSubmodule(newSubmodule_);

    // Get the existing submodule, ensure that it's not zero and not the same as the new submodule
    // If this reverts due to no submodule being installed, then the new submodule should be installed via installSubmodule
    Submodule oldSubmodule = getSubmoduleForKeycode[subKeycode];
    if (oldSubmodule == Submodule(address(0)) || oldSubmodule == newSubmodule_)
        revert Module_InvalidSubmoduleUpgrade(subKeycode);

    // Update submodule in module
    getSubmoduleForKeycode[subKeycode] = newSubmodule_;

    // Initialize the submodule
    newSubmodule_.INIT();
}

function execOnSubmodule(
    SubKeycode subKeycode_,
    bytes memory callData_
) external permissioned returns (bytes memory) {
    ...
}

function getSubmodules() external view returns (SubKeycode[] memory) {
    return submodules;
}

In the ModulesWithSubmodules contract there are only two methods for modifying submodules, installSubmodule and upgradeSubmodule, neither of which have the ability to remove a module. This means there is no way to uninstall submodules. This can lead to substantial bloat over time. This is especially relevant to the SPPLY module. The treasury and supply are highly diversified and assets/allocation are updated/removed on a very frequent basis. Each time an asset is retired the submodule representing can never be removed from the module. Overtime this will continue to grow and ultimately may DOS the entire RBS system.

Impact

Modules bloated with retired submodules can lead to key system DOS

Code Snippet

Submodules.sol#L70-L156

Tool used

Manual Review

Recommendation

Create a function that allows for the removal of old, unnecessary submodules.

sherlock-audit / 2023-11-olympus-judging

0x52 - Submodules can be installed but never removed causing bloat and potential DOS over time #153