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 reentrancy guard #23

Merged
merged 2 commits into from
Sep 16, 2022
Merged

Remove reentrancy guard #23

merged 2 commits into from
Sep 16, 2022

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Sep 16, 2022

Removes reentrancy checks in preparation for the audit.

We introduced reentracy checks after this discussion.

There is no plan of implementing the order update function before the audit or before deploying the first version of the ETH flow contract. The order update function and its extra reentrancy risks was the main motivations for the reentrancy guard, so there is no reason to keep the guard until development of a new contract version with this feature starts.

Note that reentrancy between createOrder and deleteOrder does not help an attacker. No instructions after the reentrancy entry point read or write the storage of the contract, so calling a function while reentering would be equivalent to calling a function after the call to deleteOrder is finished.

Test plan

Existing tests.

@fedgiac fedgiac requested a review from a team September 16, 2022 12:36
@fedgiac fedgiac marked this pull request as ready for review September 16, 2022 12:39
@josojo josojo merged commit 5659dfb into main Sep 16, 2022
@josojo josojo deleted the remove-reentrancy-check branch September 16, 2022 17:26
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