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

Fix reentrancy vulnerability #30

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

0xPanku
Copy link

@0xPanku 0xPanku commented Jan 24, 2022

Fix re-entry vulnerability on split payment using call();
The first addr could drain all the funds.

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Fix re-entry vulnerability on split payment using call();
The first addr could drain all the funds.

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
@liarco
Copy link
Contributor

liarco commented Jan 24, 2022

Thank you @0xPanku for this contribution, I'm gonna make a review and take action ASAP.

@liarco
Copy link
Contributor

liarco commented Jan 25, 2022

Hi @0xPanku, please correct me if I'm wrong, but from what I see the re-entrancy attack can only be performed by a malicious owner, because the withdraw method has the onlyOwner modifier and that is run before any code in the method itself. (So the fix is technically correct, but a malicious owner has infinite ways of scamming the team if that's his real intention)

If that's the case, we absolutely think this fix is valuable, but we prefer including it in our next contract release so we won't confuse people following along from the YouTube videos since they have line-by-line references there.

Anyway thank you very much for your time, we really appreciate this kind of contributions.

@0xPanku
Copy link
Author

0xPanku commented Jan 26, 2022

Hi @liarco, you are absolutely right about your analysis.
As people tend to copy/paste the code, it may be good to apply the patch in a future release but, yes your are right there is no rush.
thanks

@liarco
Copy link
Contributor

liarco commented Jan 26, 2022

You are welcome and by the way, thank you very much for pointing this out. This kind of contributions from the community are what make open-source projects grow the right way! :)

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.

2 participants