Feat/sunset v2#76
Open
galacticminter wants to merge 55 commits into
Open
Conversation
`error.annotate()` dumps full env (incl. OWNER1_KEY, SAFE_API_KEY) into the error message. Surfaced repeatedly via pre-commit hook output. Replace with key-only error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
L1 MaticX MATIC balance = 0. Legacy MATIC paths (deprecated submit, stakeRewardsAndDistributeFeesMatic) won't materially trigger pre-pause. Any MATIC dust falls through to sweepToCustody after CUSTODY_DELAY. Removes IPolygonMigration import, POLYGON_MIGRATION constant, and the migrate() block. Lean contract, one less trust surface. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
galacticminter
commented
May 12, 2026
…or enhanced clarity and safety
Replace the finalizeTerminalRate require on sweepToCustodyTimestamp with an explicit `sweepToCustodyTimestamp == 0` revert inside sweepToCustody. Same protection against the unset-delay footgun, without coupling finalize ordering to setCustodyDelay. Test updated: footgun guard now asserts sweepToCustody reverts with CustodyDelayNotElapsed when sweepToCustodyTimestamp is 0.
galacticminter
commented
May 15, 2026
| uint256 rate = preFinalizeRate == 0 ? 1 : preFinalizeRate; | ||
| uint256 balanceInMaticX = (_balance * TERMINAL_RATE_PRECISION) / | ||
| rate; | ||
| return (balanceInMaticX, TERMINAL_RATE_PRECISION, rate); |
Contributor
Author
There was a problem hiding this comment.
This is wrong format.
(balanceInMaticx, totalmaticxsupply, totalmaticOnContract)
| if (recallInitiated) { | ||
| uint256 rate = preFinalizeRate == 0 ? 1 : preFinalizeRate; | ||
| uint256 balanceInPOL = (_balance * rate) / TERMINAL_RATE_PRECISION; | ||
| return (balanceInPOL, TERMINAL_RATE_PRECISION, rate); |
Contributor
Author
There was a problem hiding this comment.
This is wrong format.
Contributor
Author
There was a problem hiding this comment.
Please route this to either revert() when recallInitiated but not recallCompelte and to instantUnstake after terminalratelocked please.
Contributor
Author
There was a problem hiding this comment.
Do we still need InstantUnstake after this fn?
Contributor
Author
There was a problem hiding this comment.
@blockgroot would 100% prefer this backwards compatible change. Route this to "instantUnstake" after instantUnstakeEnabled flag please. Revert in other stages before this.
There was a problem hiding this comment.
whenNotPaused modifier on requestWithdraw will block the function execution. moreover instantUnstake does not take any amount param.
There was a problem hiding this comment.
I don't see any downside of keeping them decoupled, we did same setup earlier in BNBx as well and it's a small change on FE to facilitate legacy and new instant claims.
| mapping(address => uint256) public assetRecallNonces; | ||
| bool public recallInitiated; | ||
| uint256 public preFinalizeRate; | ||
| bool public recallClaimsComplete; |
Contributor
Author
There was a problem hiding this comment.
recallComplete*
| address vs = stakeManager.getValidatorContract(validatorIds[i]); | ||
| uint256 nonce = assetRecallNonces[vs]; | ||
| if (nonce != 0) { | ||
| // Claim first, then pop: if the validator reverts (e.g. |
Contributor
Author
There was a problem hiding this comment.
Change comment
| function claimAssetRecallNonces() external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| require(paused(), "Pause first"); | ||
| if (!recallInitiated) revert RecallNotInitiated(); | ||
| if (terminalRateLocked) revert TerminalRateAlreadyLocked(); |
Contributor
Author
There was a problem hiding this comment.
you have to check for recallClaimsComplete right?
There was a problem hiding this comment.
Eventually it's going to revert and also guarded by DEFAULT_ADMIN_ROLE.
also, terminalRateLocked require recallClaimsComplete so this check is going to be redundant.
|
|
||
| uint256 polBal = polToken.balanceOf(address(this)); | ||
| uint256 maticBal = maticToken.balanceOf(address(this)); | ||
| recalledPolBalance = 0; |
Contributor
Author
There was a problem hiding this comment.
Any custody movement (any token), we can stop instant unstake.
Per review stader-labs#2: recalledPolBalance is mathematically redundant. After finalize, terminalRate = balance / supply. Each instantClaim burns m MATICx and transfers m * rate, so supply and balance decrement proportionally — the implied rate is invariant. balanceOf is a single source of truth, no drift, one fewer storage slot. - Remove storage slot - instantClaim: sufficiency check + payout read balanceOf live - pushTerminalRateToL2: reads balanceOf live (idempotent under proportional drain) - sweepToCustody: drop zero-out - AssetRecallCompleted event already takes polBalance (no change); TerminalRatePushedToL2 2nd arg renamed to polBalanceAtPush
Per review stader-labs#3 + stader-labs#4: _convertMaticXToPOL and _convertPOLToMaticX returned (balance, TERMINAL_RATE_PRECISION, rate) in the recall and post-finalize branches, violating the natspec contract of (balance, totalShares, totalPooled). Return derived (totalShares, totalPooled) where totalPooled = totalShares * rate / 1e18. The implied rate (totalPooled / totalShares) equals the locked rate exactly — donation-immune, semantics intact.
Per review stader-labs#6. Storage slot unchanged (same bool), only the public getter selector changes.
Per review stader-labs#7. Call sellVoucher_newPOL first, then read the post-incremented unbondNonces canonically — same pattern as the legacy requestWithdraw path at L308-315. Drops the +1 prediction.
…bs#9) Once the claim phase succeeds (recallComplete = true), block re-entry with RecallAlreadyComplete. Tightens the one-way state machine: initiated -> complete -> finalize. Prior idempotent no-op behavior on post-completion calls is now an explicit revert.
…-labs#10/stader-labs#11/stader-labs#12) sweepToCustody now takes (asset, custody) and moves the full balance of any ERC20. First call (any asset) flips `assetCustodied`, which permanently disables `instantClaim` so the post-custody residue can't be re-entered.
galacticminter
commented
May 16, 2026
| require(paused(), "Pause first"); | ||
| if (!recallInitiated) revert RecallNotInitiated(); | ||
| if (recallComplete) revert RecallAlreadyComplete(); | ||
| if (terminalRateLocked) revert TerminalRateAlreadyLocked(); |
Contributor
Author
There was a problem hiding this comment.
You don't need terminalRateLocked anymore. Dead stmt.
| /// Dust remaining in validators is forfeit (not user funds — terminal rate | ||
| /// is computed from POL balance only). | ||
| function finalizeTerminalRate() external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| require(paused(), "Pause first"); |
Contributor
Author
There was a problem hiding this comment.
We aren't thinking through this migration in a right way. Lots of redundant steps and multidirection flows (primary reason for bugs).
- Pause protocol
- initiate recall - you need to just check for pausing (which is prerequisite) and recallInitiated (so you can't call this again).
- Complete Recall - you need to just check for recallInitiated (so you can call completeRecall) and for completeRecall (so you can't call this again). How can you guarantee that we are not prematurely calling this fn?
- FreezeRate - again check just for previous step and the current step
- PushTOL2
- setInstantRedeemEnabled
- All good?
- Asset Custody
| // During recall (post-bulkUnstake, pre-finalize): serve the | ||
| // pre-recall snapshot so oracle does not drift toward zero as | ||
| // validators unbond. | ||
| if (recallInitiated) { |
Contributor
Author
There was a problem hiding this comment.
Do we truly need this?
Users cannot deposit, withdraw after pausing or even recallInitiated, right?
Contributor
Author
There was a problem hiding this comment.
Same with preFinalizeRate
blockgroot
reviewed
May 18, 2026
blockgroot
reviewed
May 18, 2026
| // oracles cannot be moved by donations or recalled-balance burns. | ||
| // Return derived (shares, pooled) so totalPooled/totalShares ratio | ||
| // equals the locked rate exactly — donation-immune. | ||
| if (terminalRateLocked) { |
blockgroot
reviewed
May 18, 2026
| // Set only after the whole loop completes: if any per-validator | ||
| // claim reverts (unbond not yet matured), the entire tx reverts | ||
| // and this flag stays false so the txn can be retried. | ||
| recallComplete = true; |
blockgroot
reviewed
May 18, 2026
| function setInstantRedeemEnabled( | ||
| bool _enabled | ||
| ) external onlyRole(DEFAULT_ADMIN_ROLE) { | ||
| if (_enabled && !terminalRateLocked) revert TerminalRateNotLocked(); |
blockgroot
reviewed
May 18, 2026
| /// path post-sunset and it always exits the caller in full. Reverts with | ||
| /// `ZeroAmount` if the caller holds no MATICx. | ||
| /// Intentionally not gated by `whenNotPaused`. | ||
| function instantClaim() external nonReentrant { |
Pack 5 bools into single slot; group uints; mapping last. Slim down NatSpec and inline comments to essentials.
- claimAssetRecallNonces: drop redundant terminalRateLocked check; set recallComplete before loop for re-entry safety - finalizeTerminalRate: set terminalRateLocked early - setInstantRedeemEnabled: require terminalRateLocked for any toggle - sweepToCustody: set assetCustodied early
…unset upgrade verification(Bailsec Issue_23)
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
2 participants
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.
No description provided.