KupiaSec - `removeAsset` reverts in `OlympusTreasury`.

[sherlock-admin2]

KupiaSec

medium

removeAsset reverts in OlympusTreasury.

Summary

removeAsset reverts in OlympusTreasury because it is incorrectly implemented.

Vulnerability Detail

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

Removing locations from an asset is implemented weirdly, in for loop, it accesses last element of original array even though there is pop for every iteration, thus it reverts at 2nd iteration.

Here's a test case that verifies the function reverts:

function testAuditRemoveAsset() 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));
}

Result of running test:

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

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Impact

Admin is not able to remove assets.

Code Snippet

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

Tool used

Manual Review

Recommendation

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

Duplicate of #151