Resolve open issues: reentrancy tests, TimeLock Ownable + grace period, Staking invariant - #14
Merged
Merged
Conversation
Vault.withdraw and Staking.unstake both update state before the external call, so a re-entrant call sees a balance that is already spent. Add malicious receivers that re-enter withdraw and unstake from inside their receive(), and assert the attack unwinds and leaves the honest balances and totals untouched. This turns the assumed checks-effects-interactions property into a tested one. Closes #10
TimeLock fixed the admin in the constructor with no way to rotate it. Inherit the existing two-step Ownable mixin instead, so the deployer becomes the admin and the role can be handed over with propose plus accept. The onlyAdmin guard and the constructor admin argument are gone, replaced by Ownable's onlyOwner and its owner. Tests deploy as the admin and cover the full transfer flow. Closes #12
Once eta passed, a queued operation stayed executable forever. Follow the pattern Compound's Timelock uses: add an immutable gracePeriod and revert execute once block.timestamp is past eta plus the grace period, so a stale operation has to be queued again. The constructor takes the grace period, the fuzz test now stays within the window, and a vm.warp test covers the stale case. Closes #11
Staking had a light inline invariant that only checked totalStaked against the balance. Move it into its own test/StakingInvariant.t.sol and give it the same shape as the Vault handler: a bounded actor set, ghost variables, and three invariants (per-actor sum, net flow, and balance versus totalStaked). Drop the old inline version so the staking accounting is exercised the same way Vault is. Closes #13
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Works through the four open issues, one commit each (plus a gas-snapshot refresh).
Add reentrancy tests for Vault and Staking (#10). New
test/Reentrancy.t.solwith malicious receivers that re-enterwithdraw/unstakefrom insidereceive(). The attack unwinds because state is updated before the external call, and the tests assert the honest balances and totals are untouched. Turns the assumed checks-effects-interactions property into a tested one.TimeLock: make admin transferable via the two-step Ownable mixin (#12).
TimeLocknow inherits the existingsrc/Ownable.sol, so the deployer becomes the admin and the role can be handed over with propose plus accept. TheonlyAdminguard and the constructor admin argument are replaced by Ownable'sonlyOwnerandowner, with tests for the full transfer flow.TimeLock: expire stale queued operations with a grace period (#11). Adds an immutable
gracePeriodand revertsexecuteonceblock.timestampis pasteta + gracePeriod, following Compound's Timelock. Covered with avm.warpstale test, and the fuzz test now stays inside the window.Add a dedicated Staking invariant handler like VaultInvariant (#13). Moves the light inline Staking invariant into
test/StakingInvariant.t.soland gives it the same shape as the Vault handler: a bounded actor set, ghost variables, and three invariants (per-actor sum, net flow, balance versus totalStaked).Verified locally:
forge buildclean,forge test59 passed (up from 51),forge fmt --checkclean.Closes #10
Closes #11
Closes #12
Closes #13