Fixed bug where domainSeparator was constant bytes32(0) #979
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* feat: add share helpers * fix: add deposit scaling factor * fix: rebase
* fix: slashable window boundaries * test: regression for alm * test: update withdrawal delay not passed reversion * test: burning indices * refactor: switch conditionals * fix: added unit tests * test: assert slashable shares in queue * fix: typos --------- Co-authored-by: Yash Patil <[email protected]>
refactor small cleanup chore: `forge fmt` fix: `getQueuedWithdrawals` + test fix: add constructor back test: `totalQueued` > `withdrawal.strategies.length` test(wip): `completeQueuedWithdrawals` currently failing fix: effectBlock test(wip): @8sunyuan patch fix: one flaky test fix: second flaky test
* feat: initial deploy * feat: slashing patch
* test(wip): todos * fix: dealloc issue * fix: remaining * fix: forktest upgrade issue * test: add `check_Withdrawal_AsShares_State_AfterSlash` * refactor: cleanup * fix: ci * refactor: review changes
ypatil12
force-pushed
the
slashing-magnitudes
branch
from
fe5e397 to
10fb40f
Compare
|
Closing in favor of #990 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the current version, _INITIAL_DOMAIN_SEPARATOR is initialized in the constructor to be constant bytes32(0)
This is because the constructor uses the "gas efficient" domain separator function, but since the INITIAL_CHAIN_ID is set first, the _INITIAL_DOMAIN_SEPARATOR retains its original default value (bytes32(0)).
One potential fix is to simply change the order and set the domain separator before the chain id. The only potential drawback here is if this contract is deployed on ethereum classic (chain id 0), the original bug persists. Because of that edge case I've decided to go with this "bulletproof" fix.
Also note that the
address(this)in the domain separator no longer refers to the proxy contract address, but rather the implementation address. This is because _INITIAL_DOMAIN_SEPARATOR is set in the constructor rather than initializer.