Skip to content

Fix Reentrancy Vulnerability in executeTransaction #82

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

Open
seriousfuzzy opened this issue Nov 11, 2024 · 2 comments
Open

Fix Reentrancy Vulnerability in executeTransaction #82

seriousfuzzy opened this issue Nov 11, 2024 · 2 comments

Comments

@seriousfuzzy
Copy link

hello @hackfisher @yrong @hujw77 @WoeOm @xiaoch05 ,
please check this PR.

Changes:

  • Updated executeTransaction to set executed = true before external calls.
  • Used CEI pattern to prevent reentrancy.
  • Added fallback to send for safe Ether transfers.
  • Improved error handling on external calls.

Testing:

  • Added unit tests for reentrancy scenarios.
  • Verified successful and unsuccessful transaction execution behavior.
@hujw77
Copy link
Collaborator

hujw77 commented Nov 12, 2024

Thank you so much for your contribution!

  1. This contract is a fork of the Gnosis MultiSigWallet, and you can also submit this PR to them.
  2. We’ve already switched to using the Gnosis Safe Wallet in the production environment.
  3. The original code already includes reentrancy checks.
  4. This is an older version of the contract, so your modifications may not compile and will need to be adapted for the older solidity compiler.

Given these reasons, we won’t be merging the PR for now.

@seriousfuzzy
Copy link
Author

Thank you.

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

No branches or pull requests

2 participants