fix: dao hardfork activation check underflow #776
Conversation
|
agostbiro
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
agostbiro
changed the title
| && drift <= DAO_FORCE_EXTRA_DATA_RANGE | ||
| // Prevent overflow by checking that activation block is smaller | ||
| && dao_hardfork_activation_block <= header.number | ||
| && header.number - dao_hardfork_activation_block <= DAO_FORCE_EXTRA_DATA_RANGE |
There was a problem hiding this comment.
Bikeshedding: maybe replacing these two conditions with header.number <= dao_hardfork_activation_block + DAO_FORCE_EXTRA_DATA_RANGE is more concise and more self-explanatory. YMMV though, feel free to ignore.
There was a problem hiding this comment.
Not sure that's correct tbh. The condition should only activate if the block number is in the range [1_920_000, 1_920_009]
The operation
underflows if for example
header.numberis 1 anddao_hardfork_activation_blockis the actual hardfork activation (1_920_000).This panics in debug mode and wraps around in release mode silently. The wrapping doesn't cause any problems, because the subsequent check
drift <= DAO_FORCE_EXTRA_DATA_RANGEis false if it wrapped around.Still, it's not great to have a panic here in debug mode, so I fixed it by refactoring the drift check. Since there is no user impact, I don't think we need a changelog item.
We haven't caught this before, because we run Hardhat integration tests in release mode.