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 ReentrancyGuard for Proxy Pattern #2171

Merged
merged 5 commits into from
May 12, 2020

Conversation

BrendanChou
Copy link
Contributor

@BrendanChou BrendanChou commented Apr 8, 2020

Allows the ReentrancyGuard to be used in contracts that are proxied. Fixes two problems:

  1. The ReentrancyGuard was not able to be used by new contracts that use the proxy pattern.
  2. The ReentrancyGuard was not able to be used by old contracts that use the proxy pattern and want to move to new implementation contracts that use this new ReentrancyGuard.

In both cases, the "corrupted" value of the _notEntered slot (zero in the case of new contracts, non-zero in the case of existing contracts) is now fixed in the first call to any nonReentrant function, making it backwards-compatible with any previous version of ReentrancyGuard

Another added benefit is that this should decrease the gas costs of this function as uint256 values use the entire value of the slot and thus do not have to be checked by bytecode that the storage slot is returning a valid boolean value upon each SLOAD or SSTORE

I invite anyone to take-on and clean-up this PR as necessary

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Thanks for this suggestion!

First of all we want to make clear that this library is not intended for use behind proxies, because it has constructors and we make use of storage in ways that don't work with proxies, and also because we make changes that can result in storage incompatibilities and corrupted upgrades. For use with proxies and upgrades, we maintain the separate @openzeppelin/contracts-ethereum-package where we do take all of these precautions.

That said, this PR is interesting because of the improvement in gas costs. I ran some tests and indeed this implementation is significantly cheaper (by ~1800 gas). As you mentioned, the improvement comes from using uint256 instead of bool. Not only because the value isn't masked when read from storage, but also because when booleans are written to storage the entire slot is first read from storage again in order to modify only the relevant bytes. 🤯

contracts/utils/ReentrancyGuard.sol Show resolved Hide resolved
contracts/utils/ReentrancyGuard.sol Outdated Show resolved Hide resolved
@nventuro
Copy link
Contributor

nventuro commented Apr 17, 2020

It's a sad state of affairs that using the right tools provided by the language, bool or even enum, result in code much less efficient than the hacky approach.

@frangio
Copy link
Contributor

frangio commented Apr 17, 2020

@BrendanChou Let us know if you want to work on this or if we should take over.

@BrendanChou
Copy link
Contributor Author

@frangio go ahead and take it over. I'm less familiar with the standards you all have for code quality and linting

@stale
Copy link

stale bot commented May 9, 2020

Hi all!
This Pull Request has not had any recent activity, is it still relevant? If so, what is blocking it? Is there anything we can do to help move it forward?
Thanks!

@stale stale bot added the stale label May 9, 2020
@frangio
Copy link
Contributor

frangio commented May 12, 2020

Not stale, we'll pick this up soon.

@stale stale bot removed the stale label May 12, 2020
@nventuro nventuro requested a review from frangio May 12, 2020 19:43
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM

@nventuro nventuro merged commit cfa9ad9 into OpenZeppelin:master May 12, 2020
@BrendanChou BrendanChou deleted the patch-1 branch May 14, 2020 19:07
lakefishingman522 added a commit to lakefishingman522/Vinium-lending that referenced this pull request Aug 17, 2023
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