Platform: Code4rena
Start Date: 19/04/2022
Pot Size: $30,000 USDC
Total HM: 10
Participants: 43
Period: 3 days
Judges: moose-code, JasoonS
Total Solo HM: 7
Id: 90
League: ETH
Rank: 3/43
Findings: 2
Award: $4,045.80
🌟 Selected for report: 2
🚀 Solo Findings: 2
🌟 Selected for report: csanuragjain
https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ChainlinkPriceOracle.sol#L60
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
addAsset should only work if assetInfoOf[_asset] value is empty
#0 - jn-lp
2022-05-11T14:28:13Z
Aggregators often break or are updated to new logic, the manager tracks these changes and sets the value to the current one
#1 - moose-code
2022-05-24T13:55:55Z
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.
🌟 Selected for report: csanuragjain
https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/ManagedIndex.sol#L35 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TopNMarketCapIndex.sol#L57 https://github.com/code-423n4/2022-04-phuture/blob/main/contracts/TrackedIndex.sol#L45
Initialize function can be called multiple times with same asset. Calling with same asset will make duplicate entries in assets list. Any function reading assets will get impacted and would retrieve duplicate asset
Add a check to fail if assets already contains the passed asset argument. Also add a modifier so that initialize could only be called once
require(!assets.contain(asset), "Asset already exists");
#0 - jn-lp
2022-05-11T13:11:28Z
We require caller of initialize method to be a factory (which is non-upgradable contract), so it can't be called twice
see:
require(msg.sender == factory, "ManagedIndex: FORBIDDEN");
#1 - moose-code
2022-05-24T13:58:53Z
Given the factory contract is not supplied it makes it impossible to know these things and hence siding with the warden for the disclosure.
" to be a factory (which is non-upgradable contract)" i.e. one can't know this if the factory is not supplied or documented.