OlympusTreasury.removeAsset can fail

Summary

removeAsset will fail if asset.locations.length > 1

Vulnerability Detail

function has following piecec of code:

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

After first iteration assets.locations.length will decrease by 1 and last element would have index len-2 and acess to len-1 will revert (len is initial length). This code copy pasted from other functions when there is break after first location remove. Like:

 if (assets[i] == asset_) {
      assets[i] = assets[len - 1];
      assets.pop();
      break;
  }

But here it's not the case.

Impact

Dos of remove asset function

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

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

Duplicate of #151

sherlock-audit / 2023-11-olympus-judging

evilakela - OlympusTreasury.removeAsset can fail #204