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

Remove _merkleStreamers mapping #218

Merged
merged 1 commit into from
Nov 26, 2023

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Nov 20, 2023

The Merkle streamer contract inherits Adminable contract, which allows the admin to transfer its ownership to someone else. If that happens, the _merkleStreamers will include incorrect data about the campaign's admin. To prevent this, I removed both the mapping and the getter function.

fix: remove getMerkleStreamers function
test: update test accordingly
@PaulRBerg
Copy link
Member

Thanks @andreivladbrg, reviewing now. For future PRs, it would be helpful to mention the audit finding this PR addresses:

https://cantina.xyz/ai/6d6a47ff-5279-4502-8705-d837b159a31e/findings/6daf4dc9-65fb-4bde-87a8-bf7f42c02b30

Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@PaulRBerg PaulRBerg merged commit 13f0b9e into staging Nov 26, 2023
6 of 7 checks passed
@PaulRBerg PaulRBerg deleted the fix/remove-merkle-streamers-mapping branch November 26, 2023 15:07
@PaulRBerg
Copy link
Member

Cc @razgraf @gavriliumircea, this PR entails some changes in the subgraph.

@razgraf
Copy link
Member

razgraf commented Nov 26, 2023

Cc @razgraf @gavriliumircea, this PR entails some changes in the subgraph.

Could you go a bit into detail as to which changes may affect the subgraph?

I don't see anything altering event signatures. Aside from maybe updating the ABIs to remove that getter (which we aren't using) is there something else we should consider?

@PaulRBerg
Copy link
Member

PaulRBerg commented Nov 26, 2023

Could you go a bit into detail as to which changes may affect the subgraph?

Sorry, meant to say that you should now obtain the addresses from the subgraph, not from the contract. Bad wording on my part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants