Arabadzhiev - `OlympusTreasury`

[sherlock-admin]

Arabadzhiev

medium

OlympusTreasury

Summary

When a category group is removed, the categories and asset categorizations linked to it remain as they are, potentially leading to unexpected behavior

Vulnerability Detail

With the current implementation of the OlympusTreasury::removeCategoryGroup function, the only action that takes place within it is the removal of the specified group from the categoryGroups array. The problem here is that some functions rely on the categoryToGroup mapping to get the group of a given category and to also verify that the given category actually exists in the first place. One such function is OlympusTreasury::getAssetsByCategory.

    function getAssetsByCategory(
        Category category_
    ) public view override returns (address[] memory) {
        // Get category group
        CategoryGroup group = categoryToGroup[category_];
        if (fromCategoryGroup(group) == bytes32(0))
            revert TRSRY_InvalidParams(0, abi.encode(category_));

As it can be seen within the first lines of it, it gets the group of the specified category from the categoryToGroup mapping and then uses that value to verify the category exists through checking that the group != bytes32(0). However, as it can be seen in removeCategoryGroup, that mapping is not being cleared at all, meaning that all functions that are using it will function as if the category group was never removed.

    function addCategory(Category category_, CategoryGroup group_) external override permissioned {
        // Check if the category group exists
        if (!_categoryGroupExists(group_)) revert TRSRY_CategoryGroupDoesNotExist(group_);

And as it can be seen in the OlympusTreasury::addCategory function, a category can not be added to an unexisting category group. This means that there is an invariant being held - A category can not exist without an existing category group pointing to it. Since removeCategoryGroup does not respect that invariant, any policies that rely on that invariant being held might end up not functioning properly.

Additionally, when OlympusTreasury::removeAsset is called on an asset that has been linked to a category group that has been removed though removeCategoryGroup, the categorization mapping won't be completely cleared from that asset, since it uses the categoryGroups array (which does not contain removed groups within it) to clear the mapping.

        // Remove categorization
        len = categoryGroups.length;
        for (uint256 i; i < len; ) {
            categorization[asset_][categoryGroups[i]] = toCategory(bytes32(0));
            unchecked {
                ++i;
            }
        }

This will also likely lead to unexpected situations if a removed asset and category group are to be re-added at some point in the future.

Impact

Downstream policies using the OlympusTreasury module might behave unexpectedly

Code Snippet

OlympusTreasury.sol#L567-L583

Tool used

Manual Review

Recommendation

Extend the logic of the OlympusTreasury::removeCategoryGroup function, so that it also clears the categories and asset categorizations linked to the category group being removed

Duplicate of #158