Add ReentrancyGuard status getter#3714
Conversation
|
Hello @zhiqiangxu I'm not sure about this one. If multiple functions in the contract are nonReentrant, you don't know which one was you are entering from. You are also not sure what path you are entering trough, and if you are not entering twice (either one after the other or one within the other). Can you give us more details about your usecase? |
Your concern makes sense, I now prefer just exposing the guard status and let users decide his own logic? |
|
I'm ok with exposing the guard status but we shouldn't make this function virtual. |
Yeah internal is enough. |
|
Coverage wants you to add a test for the new getter. |
| /** | ||
| * expose _status as internal function, the caller decides what to do with it | ||
| */ | ||
| function _guardStatus() internal view returns (uint256) { |
There was a problem hiding this comment.
Let's call this _reentrancyGuardStatus?
Added a check in an existing nonReentrant function, let's see if Coverage will be happy this time. |
frangio
left a comment
There was a problem hiding this comment.
Realized tests still need work. The coverage report is clearly showing that the getter is not tested.
| bytes4 func = bytes4(keccak256("callback()")); | ||
| attacker.callSender(func); | ||
|
|
||
| require(_reentrancyGuardStatus() == 2, "ReentrancyMock: _reentrancyGuardStatus failed"); |
There was a problem hiding this comment.
I don't think this is getting executed, the attacker line above probably reverts.
Please add a separate test specifically for the guard status, and for the various values it can have.
There was a problem hiding this comment.
Done.
Sorry for late response, busy doing a bunch of other things lol:)
Amxx
left a comment
There was a problem hiding this comment.
Looks mostly good. Need better testing.
I'd do an function that emit the value of the guard ... and this function would be called through 2 public function (one with the modifier, one without)
|
Sorry for the back and forth. I realized the getter was leaking the exact values we use for entered and not entered, and these are values we might want to change in the future, given that they were chosen to optimize gas usage and the optimal values may be different once again in the future. So in order to avoid leaking the exact values I've changed the function to return a boolean and renamed the function accordingly. @zhiqiangxu @Amxx Let me know what you think. |
Cool, I think that's better ! |
|
Any update? |
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2022 OpenZeppelin Contracts Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
Co-authored-by: Francisco Giordano <[email protected]> Co-authored-by: Hadrien Croubois <[email protected]>
This PR adds a entered modifier to ReentrancyGuard contract, it's useful for callbacks.