locations_ array may contain duplicate members

Summary

The locations_ array contains duplicate members, which will cause the return value calculation of some functions to be incorrect.

Vulnerability Detail

[OlympusTreasury.sol#addAsset](https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/TRSRY/OlympusTreasury.sol#L415-L444)

    function addAsset(
        address asset_,
        address[] calldata locations_
    ) external override permissioned {
        Asset storage asset = assetData[asset_];

        // Ensure asset is not already added
        if (asset.approved) revert TRSRY_AssetAlreadyApproved(asset_);

        // Check that asset is a contract
        if (asset_.code.length == 0) revert TRSRY_AssetNotContract(asset_);

        // Set asset as approved and add to array
        asset.approved = true;
        assets.push(asset_);

        // Validate balance locations and store
        uint256 len = locations_.length;
        for (uint256 i; i < len; ) {
            if (locations_[i] == address(0))
                revert TRSRY_InvalidParams(1, abi.encode(locations_[i]));
            asset.locations.push(locations_[i]);
            unchecked {
                ++i;
            }
        }

        // Initialize cache with current value
        (asset.lastBalance, asset.updatedAt) = _getCurrentBalance(asset_);
    }

When adding location in function addAsset, it does not check whether location is repeated.

But when adding location in other functions, it will check whether the location is repeated.

[OlympusTreasury.sol#addAssetLocation](https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/TRSRY/OlympusTreasury.sol#L498-L520)

    function addAssetLocation(address asset_, address location_) external override permissioned {
        Asset storage asset = assetData[asset_];

        // Check that asset is approved
        if (!asset.approved) revert TRSRY_AssetNotApproved(asset_);

        // Check that the location is not the zero address
        if (location_ == address(0)) revert TRSRY_InvalidParams(1, abi.encode(location_));

        // Check that location is not already added
        uint256 len = asset.locations.length;
        for (uint256 i; i < len; ) {
            if (asset.locations[i] == location_)
                revert TRSRY_InvalidParams(1, abi.encode(location_));
            unchecked {
                ++i;
            }
        }

        // Add location to array
        asset.locations.push(location_);
    }

In the addAssetLocation function, the for loop will check whether the location already exists. If it does, an error will be revert.

Since this type of function is called by a trusted account, this kind of incorrect input will cause low at most. However, according to the response from the project team, I consider this vulnerability to be a Medium level, which should meet the requirements. The project team’s response is as follows (can be found in discord ):

Jem: I think that if there is a duplicate or invalid address (zero), then it should be flagged. Mistakes happen. Best to catch them early

Impact

For some functions that use location, calculations will be repeated, resulting in the final error result being returned, for example: getLocationsByCategory

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/TRSRY/OlympusTreasury.sol#L415-L444

Tool used

Manual Review

Recommendation

Add repeated judgment of location in the addAsset function.

Duplicate of #78

sherlock-audit / 2023-11-olympus-judging

cu5t0mPe0 - locations_ array may contain duplicate members #119