bin2chen - removeAsset() when locations.length>1 will revert

[sherlock-admin]

bin2chen

medium

removeAsset() when locations.length>1 will revert

Summary

In removeAsset(), the implementation of deleting locations is incorrect. If locations.length > 1, it will revert out-of-bounds.

Vulnerability Detail

In removeAsset(), deleting asset will clear locations. The code is as follows:

    function removeAsset(address asset_) external override permissioned {
...

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

The above code loops pop(), the size of the array will become smaller and smaller but it always uses asset.locations[len - 1], which will cause out-of-bounds.

POC

add to TRSRY.v1_1.t.sol

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

        // 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);
    } 

forge test -vv --match-test testCorrectness_removeAsset_two_locations

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_two_locations() (gas: 204563)

Impact

when locations.length>1 unable to properly delete asset.

Code Snippet

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

Tool used

Manual Review

Recommendation

    function removeAsset(address asset_) external override permissioned {
...

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

[0xJem]

Technically correct.

Low severity. The issue would not affect the treasury valuation (which is the purpose of all of this), and removeAsset() is a permissioned function called by a whitelisted admin (via the policy).

[nevillehuang]

Hi @0xJem, to me the watsons highlighted a valid point that can break core functionality of removeAsset as long as locations.length is greater than 1, which would constitute medium severity. I think it is intended that there will be more than one location for an asset, unless I am missing something. Open to hearing your opinion.

[0xrusowsky]

https://github.com/OlympusDAO/bophades/pull/248

[IAm0x52]

Escalate

Agree with sponsor that this should be low. Although removeAsset won't work and that is inconvenient. Admin can easily remove each location prior using removeAssetLocation() then calling removeAsset after.

[sherlock-admin2]

Escalate

Agree with sponsor that this should be low. Although removeAsset won't work and that is inconvenient. Admin can easily remove each location prior using removeAssetLocation() then calling removeAsset after.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Agree with @IAm0x52, should be invalid.

[ChechetkinVV]

Agree with @IAm0x52, should be invalid.

Although it is possible to pre-remove locations before calling the removeAsset function, it should be noted that administration will most likely take place through voting on the proposal. https://docs.olympusdao.finance/main/technical/overview/#ownership-model-the-executor

Thus, after an unsuccessful call to the removeAsset function, a vote will be required to remove the locations. I cannot agree that this is a simple inconvenience, because there is a lot more work to be done than just calling one additional function.

This is solely my assumption, since all module management processes are carried out through the kernel, and this is outside the scope of this contest.

[IAm0x52]

Fix looks good. Solidity already clears this by default. Confirmed with a new test

[Czar102]

I agree, this is a low severity issue. Planning to accept the escalation.

[Czar102]

Result: Low Has duplicates

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• IAm0x52: accepted