Unexpected error when assets removal at the OlympusTreasury

Summary

There is the removeAsset function at the OlympusTreasury contract which will revert when removing several locations because they are out of bounds of the asset.locations array.

Vulnerability Detail

The removeAsset function removes locations from assetData[asset_].locations with the pop() function. It reduces the length of the array which is cached outside of the loop.

        // Remove locations
        len = asset.locations.length;
        for (uint256 i; i < len; ) {
            asset.locations[i] = asset.locations[len - 1];
            asset.locations.pop();
            unchecked {
                ++i;
            }
        }

There is a test case for the TRSRYv1_1Test which throws the array out-of-bounds access error but should not:

    function testCorrectness_removeAsset_AssetConfigured_manyLocations() public {
        address[] memory locations = new address[](3);
        locations[0] = address(1);
        locations[1] = address(2);
        locations[2] = address(3);

        // Add an asset
        vm.prank(godmode);
        TRSRY.addAsset(address(reserve), locations);

        // Remove the asset
        vm.prank(godmode);
        TRSRY.removeAsset(address(reserve));

        // Verify asset data
        TRSRYv1_1.Asset memory asset = TRSRY.getAssetData(address(reserve));
        assertEq(asset.approved, false);
        assertEq(asset.lastBalance, 0);
        assertEq(asset.locations.length, 0);

        // Verify asset list
        address[] memory assets = TRSRY.getAssets();
        assertEq(assets.length, 0);
    }

The test result is:

Running 1 test for src/test/modules/TRSRY.v1_1.t.sol:TRSRYv1_1Test
[FAIL. Reason: panic: array out-of-bounds access (0x32)] testCorrectness_removeAsset_AssetConfigured_manyLocations() (gas: 232017)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.28s

Impact

The issue breaks the assets removal functionality of the OlympusTreasury contract.

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/TRSRY/OlympusTreasury.sol#L469-L477

Tool used

Manual Review

Recommendation

Consider removal the asset.locations[i] = asset.locations[len - 1]; expression:

        // Remove locations
        len = asset.locations.length;
        for (uint256 i; i < len; ) {
-           asset.locations[i] = asset.locations[len - 1];
            asset.locations.pop();
            unchecked {
                ++i;
            }
        }