Add reentrancy guard #11
Conversation
There was a problem hiding this comment.
Looks great.
Sorry, I hope there was no miss communication from my side. Since we reevaluate this later, it would have been fine for me to switch that all out later, and only come up with the nice testing - that probably required some time to write - when we actually wanna go for it.
Personally, I kind of prefer having it in now 😛. IMO, its better to be safe now, forget about the issue later, and then deploy a slightly less gas efficient contract than not include it now, forget to get back and check this later, and deploy a contract with a potential bug that can be exploited. |
|
I also prefer the piece of mind of not leaving the code in a potentially unsafe state. The test did take more than expected to write however. 🙈 |
This PR makes order creation and deletion non reentrant using OpenZeppelin's dedicated contract.
This allows us to send ETH to users without worrying about reentrancy attacks without worrying about keeping all existing function reentrancy safe.
The drawback is a very high extra execution cost of about 2500 gas (details).
The code that is being replaced by this PR is reentrancy safe but a user could lose funds if creating an order from a smart contract that has a complicated receive function.
Context: discussion in #7, especially this thread and this thread.
Before auditing starts I plan to remove the reentrancy guard by auditing the contract's reentrancy risks ourselves.
Test plan
Unit tests.