jasonxiale - `OlympusTreasury.removeAsset` will revert if `asset.locations.length` is larger than 1

[sherlock-admin2]

jasonxiale

medium

OlympusTreasury.removeAsset will revert if asset.locations.length is larger than 1

Summary

OlympusTreasury.removeAsset is used to remove asset and related data storage, because of an Index out of bounds error when remove locations, the function will always revert if asset.locations.length is larger than 1

Vulnerability Detail

in OlympusTreasury.removeAsset, before poping asset.locations, there is an extra line of code to set asset.locations[i] = asset.locations[len - 1];, if asset.locations.length is larger than 1, the for loop will revert as Index out of bounds when access asset.locations[len - 1]

450     function removeAsset(address asset_) external override permissioned {
451         Asset storage asset = assetData[asset_];
452
453         // Check that asset is approved
454         if (!asset.approved) revert TRSRY_AssetNotApproved(asset_);
455
...
468
469         // Remove locations
470         len = asset.locations.length;
471         for (uint256 i; i < len; ) {
472             asset.locations[i] = asset.locations[len - 1]; <<<---- Here Index out of bounds access
473             asset.locations.pop();
474             unchecked {
475                 ++i;
476             }
477         }
490     }

add the following POC to src/test/modules/TRSRY.v1_1.t.sol and run forge test --mc TRSRYv1_1Test --mt testRevert_removeAssetMultiple

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

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

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

Impact

OlympusTreasury.removeAsset is used to remove asset and related data storage, because of an Index out of bounds error when remove locations, the function will always revert if asset.locations.length is larger than 1

Code Snippet

https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BunniPrice.sol#L110-L166

Tool used

Manual Review

Recommendation

Duplicate of #151