search

code-423n4 / 2022-04-phuture-findings

0 stars 0 forks source link

Asset Manager can update existing _assetAggregator #22

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L60

Vulnerability details

Impact

Asset Manager can update the aggregator of an existing asset thus impacting all function making use of this asset. Ideally if an aggregator is already set for an asset the function should fail

Proof of Concept

  1. Asset Manager call function addAsset to adds an asset X with assetAggregator value as Y
  2. This is being utilized across application
  3. Now Asset Manager calls the same function addAsset with asset X with assetAggregator value as Z
  4. Asset aggregator value for asset X gets changed to Z even though it was already set to Y

Recommended Mitigation Steps

addAsset should only work if assetInfoOf[_asset] value is empty

jn-lp commented 2 years ago

Aggregators often break or are updated to new logic, the manager tracks these changes and sets the value to the current one

moose-code commented 2 years ago

Have to assume that ASSET_MANAGER_ROLE is behaving honestly in the first place otherwise everything falls apart, so this is a centralization issue.

The big question is who is being given the ASSET_MANAGER_ROLE ? This role has the power to rug everyone in every index.

Given this is unclear who is given this role (can't see anything in codebase explicitly on it, no deploy scripts, no documentation on it), one can't know who is given ASSET_MANAGER_ROLE. Given this assumption, this is a valid finding as basically one asset manager could change the oracle for another asset managers index?

Going to give this one to the warden for bringing up a valid point.