fix(invariants): reject null vault authoriser unconditionally in migration invariant - #246
fix(invariants): reject null vault authoriser unconditionally in migration invariant#246thedavidmeister wants to merge 3 commits into
Conversation
…ation invariant assertUniformAuthoriserMigration passed a vault whose authorizer() is address(0): the unhydrated STOX_PROD_AUTHORISER_V4_CLONE pin is still address(0), so a null authoriser satisfied actual == post and read as already-migrated before the deadline (and after it). A bricked, ungated vault — exactly what the cron bundle exists to catch — kept cron green. The null rejection lives in the authoriser leg (the semantic layer that knows an authoriser must never be null), not in the generic LibMigrationInvariant helper, whose bytes32/uint256 overloads serve migrations where zero can be a legitimate state. It fires before the window check and independent of the deadline, surfacing the offending vault via the typed ReceiptVaultNullAuthoriser error. Closes #245 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesNull authoriser invariant
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thedavidmeister
added
the
ai:ready
|
🤖 ai:vetter |
|
Design decision: keep the caller-level null-authoriser check (this PR), not a zero- Considered moving the guard into
Rejecting a zero |
Merging main brings LibProdDeployV4 from src/lib to src/generated, which this branch's new test still imported from the old path — a moved file breaks the build without ever showing as a merge conflict, so the PR read mergeable while being uncompilable. Its header also described the V4 clone pin as unhydrated address(0), which is what made the original fail-open reachable: a null authoriser collided with the zero pin and read as already migrated. The pin carries a real address now, so that collision is gone and a null already reverts — but as MigrationStateDrift, which names the wrong fault. Rejecting null unconditionally keeps the specific error correct whatever the pin holds, including a future chain's added unhydrated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/src/lib/LibTokenInvariantsMigration.t.sol`:
- Around line 70-75: The null-case NatSpec in
test/src/lib/LibTokenInvariantsMigration.t.sol lines 70-75 must describe the
zero-post collision as a historical regression scenario rather than current
production-pin behavior; update the comments around the null-authorizer test
accordingly. In lines 87-92, remove the assertion that all-zero values satisfy
the current live post pin, while leaving the test logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 167ada61-4a30-41b8-8438-5481b121bde3
📒 Files selected for processing (2)
src/lib/LibTokenInvariants.soltest/src/lib/LibTokenInvariantsMigration.t.sol
| /// @notice A vault whose `authorizer()` is `address(0)` trips | ||
| /// `ReceiptVaultNullAuthoriser` before the deadline, under the | ||
| /// production pins whose `post` is still `address(0)`. Pins the | ||
| /// fail-closed reading of a null authoriser: without the unconditional | ||
| /// rejection, `actual == post` (`0 == 0`) would accept the bricked | ||
| /// vault as "already migrated" and the bundle would pass. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct stale V4-pin assertions in the null-case NatSpec.
The supplied PR context says LibProdDeployV4.STOX_PROD_AUTHORISER_V4_CLONE is now non-zero. These comments incorrectly claim the live-pin calls use post == address(0) and would otherwise pass.
test/src/lib/LibTokenInvariantsMigration.t.sol#L70-L75: describe the zero-post collision as the historical regression scenario, not the current production-pin state.test/src/lib/LibTokenInvariantsMigration.t.sol#L87-L92: remove the claim that all-zero values would satisfy the current livepostpin.
📍 Affects 1 file
test/src/lib/LibTokenInvariantsMigration.t.sol#L70-L75(this comment)test/src/lib/LibTokenInvariantsMigration.t.sol#L87-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/src/lib/LibTokenInvariantsMigration.t.sol` around lines 70 - 75, The
null-case NatSpec in test/src/lib/LibTokenInvariantsMigration.t.sol lines 70-75
must describe the zero-post collision as a historical regression scenario rather
than current production-pin behavior; update the comments around the
null-authorizer test accordingly. In lines 87-92, remove the assertion that
all-zero values satisfy the current live post pin, while leaving the test logic
unchanged.
|
Closing: the defect this PR fixes no longer exists. The premise was that With a non-zero post pin, Worth recording why the green CI here was not evidence the change still worked as described: The residual concern is not the null check itself but that it is now only incidentally true: it holds because the pin happens to be non-zero, and this repo sat in the fail-open state for weeks. A future migration that introduces an unhydrated post pin reinstates the same hole. That is a pin-drift hazard rather than a live bug, and it belongs on #245 rather than in a PR whose justification has expired. Not deleting the branch. |
2 participants
What
LibTokenInvariants.assertUniformAuthoriserMigration— and via itLibInvariants.assertAll, the production invariant bundle the cron enforces — failed open on a null (address(0)) vault authoriser. The V4 post pin (LibProdDeployV4.STOX_PROD_AUTHORISER_V4_CLONE) is still unhydratedaddress(0), so for a vault whoseauthorizer()isaddress(0)the window guard's boolean derivation reads:actual != pre→0 != V3→ trueactual != post→0 != 0→ falsetrue && false→ false → no revert → a bricked, ungated vault passes as "already migrated"This PR rejects
actual == address(0)unconditionally — before the migration window is consulted and independent of the deadline — with a new typed errorReceiptVaultNullAuthoriser(address vault)that surfaces the offending vault, declared alongside the siblingReceiptVault*invariant errors.Layering
The rejection lives in
assertUniformAuthoriserMigration(the semantic authoriser leg), not in the genericLibMigrationInvariant.assertMigration: that helper is a reusable dual-state window primitive withbytes32/uint256overloads serving migrations where zero can be a legitimate value on either side. Only the authoriser leg knows that a production vault is never legitimately ungated, so that is where "null is never acceptable" belongs. Legitimate window behaviour is untouched: realpreand realpostboth pass before the deadline, onlypostat/after it.Touched files are the operator-tooling invariant layer (
src/lib/LibTokenInvariants.sol) plus tests — no deployed vault source, no generated deploy libs, no bytecode change.QA
testNullAuthoriserRevertsBeforeDeadline,testNullAuthoriserRevertsAfterDeadline(in newtest/src/lib/LibTokenInvariantsMigration.t.sol, using the CURRENT production pins: pre = real V3 authoriser, post =STOX_PROD_AUTHORISER_V4_CLONE=address(0), deadline =V4_SWAP_DEADLINE) — each fails on base: with the zero check removed (base behaviour), both fail with "next call did not revert as expected", i.e. the fail-open passes-without-revert was reproduced and proven. Window-preservation teststestWindowAcceptsPreBeforeDeadline/testWindowAcceptsPostBeforeDeadline/testWindowAcceptsPostAfterDeadline/testWindowRejectsPreAfterDeadline(non-zero sentinels) pass on base and on the fix by design — they pin that the null rejection does not narrow legitimate window behaviour and kill over-correction mutants.testNullAuthoriserRevertsBeforeDeadline+testNullAuthoriserRevertsAfterDeadlinefail (killed)block.timestamp < deadline→testNullAuthoriserRevertsAfterDeadlinefails (killed)block.timestamp >= deadline→testNullAuthoriserRevertsBeforeDeadlinefails (killed)address(0)instead of the offending vault → both null tests fail on exact revert-data mismatch (killed)actual != pre && actual != postwithpost == 0collapsing to acceptactual == 0) plus migration semantics — a null authoriser is never a legitimate state on either side of the swap — derived independently of the implementation.productionReceiptVaults()exhaustiveness guard is a related gap the issue lists, not the ask — explicitly out of scope here (separate engineering change).Closes #245
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests