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(contract): init Reentrancy Guard #18

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

PierrickGT
Copy link
Contributor

@aodhgan
Copy link
Contributor

aodhgan commented Jun 29, 2021

What is the default behaviour of the reentrancy guard when its not initialized? Just default to 0?

ACK

@PierrickGT
Copy link
Contributor Author

PierrickGT commented Jun 30, 2021

What is the default behaviour of the reentrancy guard when its not initialized? Just default to 0?

ACK

Looking at the code, I feel like it would behave the same way than when initialized.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/security/ReentrancyGuardUpgradeable.sol#L44

By initializing it, _status is then equal to _NOT_ENTERED, so 1. But without initializing it, _status would be equal to 0, which wouldn't be a problem cause once the function called, it would still set _status to _ENTERED which is equal to 2.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/91e0b106804c3f2ca7379577c680e4233e2be09c/contracts/security/ReentrancyGuardUpgradeable.sol#L59

@PierrickGT PierrickGT changed the base branch from main to fixes/c4-audit June 30, 2021 13:17
@PierrickGT PierrickGT force-pushed the fixes/0xrajeev-60 branch from 4069db9 to e9a8de8 Compare July 5, 2021 08:32
@PierrickGT PierrickGT merged commit faa315e into fixes/c4-audit Jul 5, 2021
@PierrickGT PierrickGT deleted the fixes/0xrajeev-60 branch July 5, 2021 08:37
@asselstine
Copy link
Contributor

@PierrickGT @aodhgan. Likely gas costs.

@kamescg
Copy link
Contributor

kamescg commented Jul 8, 2021

To my knowledge reverting back to 0 would have been cheaper, since no storage is required for reverting to the default.

https://eips.ethereum.org/EIPS/eip-2200

If a contract with empty storage sets slot 0 to 1, then back to 0, it will be charged 20000 + 200 - 19800 = 400 gas.

When original value is 0, we want to prove that:
Case I: If the final value ends up still being 0, we want to charge 200 * N gases, because no disk write is needed.
Case II: If the final value ends up being a non-zero value, we want to charge 20000 + 200 * (N-1) gas, because it requires writing this slot to disk.

@kamescg
Copy link
Contributor

kamescg commented Jul 9, 2021

@PierrickGT @asselstinesSimilar to pooltogether/pooltogether-yearnv2-yield-source#8 (review) may want to remove transferOwnership from initialize to adhere with other PT contracts, even thought contract doesn't include additional function for setting config params?

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.

4 participants