addAsset allows a location to be accounted for more than once
Summary
addAsset allows a location to be added multiple times.
This inflates lastBalance as tokens from that location are be accounted for more than once.
Vulnerability Detail
When calling addAsset a permissioned party can make the OlympusTreasury begin to account an asset on certain locations. The issue is the function does not sanity check whether a same location is added more than once during the asset registration:
uint256 len = locations_.length;
for (uint256 i; i < len; ) {
if (locations_[i] == address(0))
revert TRSRY_InvalidParams(1, abi.encode(locations_[i]));
asset.locations.push(locations_[i]);
unchecked {
++i;
}
}
This is problematic because after the locations processing, the function calls getCurrentBalance to determine the asset's latestBalance:
As getCurrentBalance iterates through all the registered locations without sanity checks, it will account for one single balance more than once, returning wrong values as current balance:
uint256 len = asset.locations.length;
for (uint256 i; i < len; ) {
balance += token.balanceOf(asset.locations[i]);
unchecked {
++i;
}
}
Impact
The new asset added will return wrong balances since it's addition to the Treasury.
Make sure to sanity check whether each of the locations added to the asset is not already inside the locations array, as done similarly at the addAssetLocation function:
uint256 len = asset.locations.length;
for (uint256 i; i < len; ) {
if (asset.locations[i] == location_)
revert TRSRY_InvalidParams(1, abi.encode(location_));
unchecked {
++i;
}
}
jovi
medium
addAsset allows a location to be accounted for more than once
Summary
addAsset allows a location to be added multiple times. This inflates lastBalance as tokens from that location are be accounted for more than once.
Vulnerability Detail
When calling addAsset a permissioned party can make the OlympusTreasury begin to account an asset on certain locations. The issue is the function does not sanity check whether a same location is added more than once during the asset registration:
This is problematic because after the locations processing, the function calls getCurrentBalance to determine the asset's latestBalance:
As getCurrentBalance iterates through all the registered locations without sanity checks, it will account for one single balance more than once, returning wrong values as current balance:
Impact
The new asset added will return wrong balances since it's addition to the Treasury.
Code Snippet
https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/TRSRY/OlympusTreasury.sol#L415 https://github.com/sherlock-audit/2023-11-olympus/blob/main/bophades/src/modules/TRSRY/OlympusTreasury.sol#L315
Tool used
Manual Review
Recommendation
Make sure to sanity check whether each of the locations added to the asset is not already inside the locations array, as done similarly at the addAssetLocation function:
Duplicate of #78