search

HashLips / hashlips_nft_contract

A simple NFT smart contract that works with the rest of the HashLips ecosystem.
MIT License
834 stars 674 forks source link

Fix reentrancy vulnerability #30

Open 0xPanku opened 2 years ago

0xPanku commented 2 years ago

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 commented 2 years ago

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

liarco commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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! :)