Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate asset can be added #23

Open
code423n4 opened this issue Apr 20, 2022 · 2 comments
Open

Duplicate asset can be added #23

code423n4 opened this issue Apr 20, 2022 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

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

Proof of Concept

  1. Observe that initialize function can be called multiple times
  2. Admin calls initialize function with asset X
  3. asset X gets added in assets object
  4. Admin again calls initialize function with asset X
  5. asset X again gets added in assets object making duplicate entries

Recommended Mitigation Steps

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");
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Apr 20, 2022
code423n4 added a commit that referenced this issue Apr 20, 2022
@olivermehr olivermehr added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label May 2, 2022
@jn-lp
Copy link
Collaborator

jn-lp commented May 11, 2022

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");

@moose-code
Copy link
Collaborator

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.

@moose-code moose-code removed the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants