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/issue 3 #81

Draft
wants to merge 7 commits into
base: fix-review
Choose a base branch
from
Draft

Fix/issue 3 #81

wants to merge 7 commits into from

Conversation

spengrah
Copy link
Member

@spengrah spengrah commented Dec 2, 2024

This PR addresses Sherlock issue 3, which outlines a reentrancy vulnerability that allows an attacker to bypass certain protections and, for example, change the Safe's fallbackHandler.

The fix is to modify the reentrancy guard logic to enforce that HSG.checkTransaction and HSG.checkAfterExecution can only be called in that explicit order.

Together with the existing logic enforcing that HSG.checkTransaction can only be called from within Safe.execTransaction, we now enforce that

a) HSG.checkTransaction MUST be called exactly once per outer call to Safe.execTransaction, and
b) HSG.checkAfterExecution is always comparing the existing Safe state to the original/correct snapshot of the Safe state (since it MUST be called after HSG.checkTransaction and HSG.checkTransaction cannot be called after itself)

This PR updates some of the transient variable names to better describe their intent.

Copy link
Contributor

@gershido gershido left a comment

Choose a reason for hiding this comment

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

LGTM

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