fix(#444): a maintenance-rate raise is no longer retroactive - #464
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 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 |
`last_fee_slot` is anchored at ACCOUNT CREATION and advanced only by SyncMaintenanceFee. While the rate is 0 nobody cranks, so it stays pinned at creation for the market's whole life — and the moment `marketauth` raises the rate, the next PERMISSIONLESS crank bills `new_rate x (the account's entire age)`, including every slot the rate was 0, capped only by capital. A small-looking rate takes up to 100% of a flat account, and `UpdateMaintenanceFeePerSlot` + `SyncMaintenanceFee` bundle atomically so the victim never gets a turn. That defeats #428's safety argument, which Live-gated the rate change on the assumption a user can withdraw before a hostile fee bites. They cannot: the charge is retroactive AND instantaneous. ── WHY THIS IS IN THE WRAPPER, AFTER A FIRST ATTEMPT IN THE ENGINE FAILED ────── My first attempt capped `dt` inside `sync_account_fee_to_slot_not_atomic` and broke `v16_spec_tests.rs`. spec.md §981 fixes what the ENGINE does with a given anchor: it "charges recurring wrapper-owned fees exactly once over `[last_fee_slot_i, anchor]`" and advances `last_fee_slot_i = anchor`. The retroactivity is SPECIFIED. But CHOOSING the anchor is the wrapper's job, and §981's only constraint on it is "Live anchors must be `<= current_slot`". A smaller anchor is fully compliant. So the fix is two spec-compliant engine calls instead of one: [last_fee_slot, checkpoint] at the PREVIOUS rate <- what was actually owed [checkpoint, now] at the CURRENT rate <- ordinary accrual `UpdateMaintenanceFeePerSlot` records `(slot, previous_rate)` when — and only when — the rate actually changes, so a no-op re-assert cannot move the checkpoint and silently forgive owed fees. STATE: two fields on `AssetOracleProfileV16`, asset 0's copy holding the market-wide value, following the existing precedent that asset 0's profile carries market-wide oracle state the config mirrors. `ASSET_ORACLE_PROFILE_LEN` 408 -> 432, still inside the fixed 512-byte slot (80 spare), so `MARKET_ASSET_SLOT_LEN` is unchanged and NO offset moves. This is why it is not in `WrapperConfigV16`: `WRAPPER_CONFIG_LEN` is exactly 576 and growing it shifts `MARKET_GROUP_OFF`, bricking every deployed market. Zero means "never changed", which is what a deployed market reads after an in-place upgrade — correct, since a market whose rate never changed has no retroactive window. ── (B) HAS NO SAFE PLACE TO SIT, AND THAT IS A FINDING ───────────────────────── The decision asked for a `max_accrual_dt_slots` per-crank bound as belt and braces. I implemented it. It cannot go anywhere useful once (A) exists: * on the POST-checkpoint leg it under-collects ordinary accrual from any account that simply has not been cranked lately — legitimately owed fees, not hostile ones. Six existing tests assert those exact amounts and were right to; all six failed with the cap in place. * on the PRE-checkpoint leg it is WORSE than useless: the leg stops short of the checkpoint, and the second call then bills the remaining pre-change window at the NEW rate — re-creating the exact confiscation (A) prevents. The new test fails with the account drained to zero. With (A) there is no unbounded HOSTILE window left. A cap would only defer legitimately-owed fees, so it is deliberately absent and the reasoning is in-source. VERIFIED new test `..._raising_the_maintenance_rate_is_not_retroactive` — rate 0 for ~900 slots, then raised and cranked immediately (the bundled attack). Only the 10 post-raise slots are billed: capital 950. NEGATIVE CONTROL with the checkpoint leg disabled, that test FAILS with capital 0 — the account fully drained. Restored, 950. full CI suite 605 passed / 21 failed — "OK: failing set matches the allowlist exactly". The six accrual tests pass UNTOUCHED. STACKED ON #420 (prog#463): both grow `AssetOracleProfileV16`, so this branch is based on that one to avoid a conflict. Merge #463 first. UPSTREAM: `sync_account_fee_to_slot_not_atomic` exists at `av` (tip 8eb7142a) with the same retroactive shape, so this is not a divergence we introduced — and it is worth filing upstream, since spec.md is Toly's and upstream's own engine has the same exposure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D
dcccrypto
force-pushed
the
fix/444-fee-checkpoint-and-cap
branch
from
September 4, 2026 00:32
f9d40b9 to
8b54714
Compare
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
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.
Decision C — both halves. Half (A) is implemented; half (B) turns out to have no safe place to sit, and that's a finding rather than an omission. Detail below.
The bug
last_fee_slotis anchored at account creation and advanced only bySyncMaintenanceFee. While the rate is 0 nobody cranks, so it stays pinned at creation for the market's whole life — and the momentmarketauthraises the rate, the next permissionless crank billsnew_rate × (the account's entire age), including every slot the rate was 0, capped only by capital.A small-looking rate takes up to 100% of a flat account, and the rate change and the crank bundle atomically so the victim never gets a turn. That defeats #428's safety argument, which Live-gated the change assuming a user could withdraw first.
My first attempt was in the engine, and the spec rejected it
I capped
dtinsidesync_account_fee_to_slot_not_atomicand brokev16_spec_tests.rs:The retroactivity is specified. But §981 governs what the engine does with a given anchor — and choosing the anchor is the wrapper's job, constrained only by "Live anchors must be
<= current_slot". A smaller anchor is fully compliant.So the fix is two spec-compliant engine calls instead of one:
UpdateMaintenanceFeePerSlotrecords(slot, previous_rate)when — and only when — the rate actually changes, so a no-op re-assert can't move the checkpoint and silently forgive owed fees.State: two fields on
AssetOracleProfileV16, asset 0's copy holding the market-wide value (the existing precedent: asset 0's profile carries market-wide oracle state the config mirrors).ASSET_ORACLE_PROFILE_LEN408 → 432, still inside the fixed 512-byte slot, so no offset moves. It isn't inWrapperConfigV16because that's exactly 576 and growing it shiftsMARKET_GROUP_OFF, bricking every deployed market.(B) has no safe place to sit
The decision asked for a
max_accrual_dt_slotsper-crank bound as belt and braces. I implemented it. Once (A) exists it cannot go anywhere useful:With (A) there's no unbounded hostile window left. A cap would only defer legitimately-owed fees, so it's deliberately absent, with the reasoning in-source rather than in a commit message someone would have to find.
Verified
Upstream
sync_account_fee_to_slot_not_atomicexists atav(tip8eb7142a) with the same retroactive shape — not a divergence we introduced, and worth filing upstream, sincespec.mdis Toly's and upstream's own engine carries the same exposure.🤖 Generated with Claude Code
https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D