fix(vault): keep OZ _totalSupply in step with rebased balances (audit H01) - #289
Conversation
📝 WalkthroughWalkthroughThis PR synchronizes OpenZeppelin’s raw ChangesRaw total supply synchronization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CHANGELOG.md`:
- Around line 17-18: Update the V4 release note around migrateAccount to replace
LibERC20Storage.setUnderlyingTotalSupply with the actual helper
LibERC20Storage.applyBalanceDeltaToTotalSupply, preserving the rest of the
changelog text.
In `@foundry.toml`:
- Line 25: Update the comment adjacent to optimizer_runs in foundry.toml to
describe the current 2000 setting and its measured EIP-170 runtime-size margin,
removing the stale 5000 claim while leaving the configuration unchanged.
🪄 Autofix
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 Plus
Run ID: 0a25adac-1bc8-4414-a131-d69bf18e366a
⛔ Files ignored due to path filters (12)
src/generated/candidate/ST0xOrchestrator.pointers.solis excluded by!**/generated/**src/generated/candidate/ST0xOrchestratorBeaconSetDeployer.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxCorporateActionsFacet.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxOffchainAssetReceiptVaultAuthorizerV1.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxOffchainAssetReceiptVaultPaymentMintAuthorizerV1.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxReceipt.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxReceiptVault.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxUnifiedDeployer.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxWrappedTokenVault.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxWrappedTokenVaultBeacon.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxWrappedTokenVaultBeaconSetDeployer.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (9)
CHANGELOG.mdfoundry.tomlsrc/concrete/StoxReceiptVault.solsrc/lib/LibERC20Storage.soltest/src/concrete/StoxReceiptVault.rawTotalSupply.t.soltest/src/concrete/TestStoxReceiptVault.soltest/src/lib/LibERC20Storage.t.soltest/src/lib/LibProdDeployV4.t.soltest/src/lib/TestERC20.sol
|
Reviewed 7f15d5f: APPROVE Independently verified beyond the PR's own claims:
Two non-blocking notes:
|
…SLOT The +2 offset from the ERC-7201 root was derived inline in both underlyingTotalSupply and setUnderlyingTotalSupply, and re-derived a third time in LibTotalSupplyHarness. Name it once as a file-level constant next to ERC20_STORAGE_LOCATION and use it for every read and write of the accumulator. The harness now writes through LibERC20Storage.setUnderlyingTotalSupply — its inline assembly predated the H01 fix reintroducing the setter, and its comment claiming the library exposes no setter was stale. Candidate pointers regenerated (BuildPointers + fmt, converged): the folded slot constant shifts StoxReceiptVault and StoxCorporateActionsFacet bytecode, cascading to ST0xOrchestrator and the three deployers. Frozen 0_1_1 snapshots untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed 4f01c7f: APPROVE Delta since 7f15d5f: names the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/LibERC20Storage.sol (1)
127-127: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a delta-aware addition.
applyBalanceDeltaToTotalSupplyaddsnewBalancebefore subtractingoldBalance. If the split multiplier is valid and the final supply fits, the intermediatesupply + newBalancecan still overflow and revert. Branch on the delta sign or add unsigned arithmetic before applying the change.🤖 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 `@src/lib/LibERC20Storage.sol` at line 127, Update applyBalanceDeltaToTotalSupply so it applies the balance change using delta-aware unsigned arithmetic: subtract oldBalance from newBalance before adding when the delta is positive, and subtract the difference when it is negative, avoiding the intermediate supply + newBalance overflow while preserving the final total supply.
🤖 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.
Outside diff comments:
In `@src/lib/LibERC20Storage.sol`:
- Line 127: Update applyBalanceDeltaToTotalSupply so it applies the balance
change using delta-aware unsigned arithmetic: subtract oldBalance from
newBalance before adding when the delta is positive, and subtract the difference
when it is negative, avoiding the intermediate supply + newBalance overflow
while preserving the final total supply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b58a235-3541-4834-b552-fa5207d07a95
⛔ Files ignored due to path filters (6)
src/generated/candidate/ST0xOrchestrator.pointers.solis excluded by!**/generated/**src/generated/candidate/ST0xOrchestratorBeaconSetDeployer.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxCorporateActionsFacet.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxOffchainAssetReceiptVaultBeaconSetDeployer.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxReceiptVault.pointers.solis excluded by!**/generated/**src/generated/candidate/StoxUnifiedDeployer.pointers.solis excluded by!**/generated/**
📒 Files selected for processing (6)
CHANGELOG.mdfoundry.tomlsrc/lib/LibERC20Storage.soltest/script/20260619-deploy-v4-authoriser-clone.t.soltest/src/concrete/upgrade/V3UpgradeShadowFork.t.soltest/src/lib/LibTotalSupplyHarness.sol
|
Reviewed 4f01c7f: APPROVE |
2 participants
Fixes H01 from the Protofire
st0x.deploy 5.0report (July 2026, audited ated767bf2, tagsol-v0.1.14) — the only open finding in that report. Bothinformationals (I01, I02) were already fixed at
aba8db0/87ce4d1.Two commits: the fix itself, then
optimizer_runs 5000 → 2000so it fits underEIP-170. Full suite is green — 657 passing, and the only failures are fork tests
needing
BASE_RPC_URL, which CI supplies.rainix-sol-staticpasses (slither: 0results,
forge fmt --checkclean).The bug
StoxReceiptVaultkeeps two records of supply:totalSupply()— the rebase-awareLibTotalSupply.effectiveTotalSupply()_totalSupplyslot — only ever written byERC20Upgradeable._updateA stock split rewrites balances but never the raw slot:
migrateAccountwrites straight to storage via
LibERC20Storage.setUnderlyingBalance, bypassing_mint/_burn(noTransfer, no supply write). The two records drift.The raw slot looks like dead weight, but OZ still uses it — burn subtracts
unchecked, mint adds checked. So once balances have been inflated by a
split, a burn of the inflated amount subtracts more than the raw slot holds, the
slot wraps to ~
2**256, and from that point every mint reverts withPanic(0x11).The report's worked example, reproduced verbatim as a test: Alice mints 1000, a
2:1 split completes, Alice redeems 1002 — the raw slot goes
1000 - 1002andwraps to
2**256 - 2. The vault holds 998 shares and can never issue more than1 more wei.
What makes it nasty is that nothing surfaces it.
totalSupply(),balanceOf()and every event keep returning correct, mutually consistent values. No funds are
lost and transfers/redemptions keep working — but new issuance is permanently
capped, and the slot has no write path other than mint/burn, so recovery would
require a beacon implementation upgrade.
Production status — latent, not yet triggered
Checked all 29 live receipt vaults on Base:
totalSupply()equals the raw sloton every one, so no split has ever been applied in production and nothing is
corrupted today. That also means this must land before the first stock split
is scheduled on any live vault.
The fix
Preserve OZ's own
_totalSupply == Σ _balancesinvariant instead of letting therebase break it — this is what the auditors recommended.
migrateAccountnowapplies the same delta to the raw slot that it applied to the balance, via a new
LibERC20Storage.applyBalanceDeltaToTotalSupply.Reported supply is unchanged:
totalSupply()is stillLibTotalSupply.effectiveTotalSupply()and the per-cursor pot accounting inonAccountMigratedis untouched. The raw slot now tracks the sum of storedbalances, which is a different quantity from the rebase-aware supply during
partial migration — they converge once every holder has migrated. Every OZ code
path now operates on valid state, so burn's unchecked subtraction and mint's
checked addition are both safe.
Scope is confined to the ERC-20 side.
StoxReceiptextendsReceiptwith noERC1155Supply, so there is no raw supply slot on the receipt side, andStoxWrappedTokenVaultnever writes storage directly.Tests
Written before the fix and confirmed failing against unfixed code — the raw slot
wrapped to exactly
2**256 - 2and the follow-up mint panicked with0x11,matching the report.
New
StoxReceiptVaultRawTotalSupplyTest(5 tests):testH01RedeemAcrossSplitDoesNotWrapRawTotalSupply— the report's exact tabletestH01MintAfterRedeemAcrossSplitStillSucceeds— the permanent-issuance-cap impacttestRawTotalSupplyFollowsReverseSplit— the shrinking directiontestRawTotalSupplyMatchesStoredBalancesDuringPartialMigration— invariant holds mid-migrationtestFuzzRawTotalSupplyInvariantAcrossSplitAndBurn— arbitrary holdings/split/burnPlus 3 new
LibERC20Storagetests pinning the+2slot offset in the writedirection (
underlyingTotalSupplyalone only pinned it for reads) and provingthe write does not disturb any
_balancesslot. 18 tests pass in total.Sizing — resolved by lowering
optimizer_runsStoxReceiptVaulthad a 6-byte runtime margin before this change (24,570 of24,576 at
optimizer_runs = 5000). The smallest correct form of the fix costs~148 bytes. Measured runtime sizes with the fix applied:
optimizer_runsRemedies attempted and measured
Adopted:
optimizer_runs = 2000. The vault lands at 24,037 with 539 bytesspare and every production contract is comfortably under the limit. Pointers
regenerate cleanly (5 passes to converge, 14 generated files).
Two in-vault alternatives were measured first and rejected:
migrateAccountreturn the balance pair so
_updateapplies one combined delta costs morethan it saves: 24,809, i.e. 233 over. The return-value plumbing outweighs the
removed duplicate.
uncheckedarithmetic — 18 bytes. Sound (two's-complement makessupply + new - oldexact whenever the true result is representable, which itis by construction) but only reaches 24,700, still 124 over. Not kept: it does
not solve the problem and the checked form is easier to audit.
So the floor for the fix is ~124–142 bytes against 6 available — the optimizer
was the only lever that clears it without a structural redesign.
What the optimizer change does and does not affect
It changes the compiled bytecode, and therefore the deterministic Zoltu address,
of every contract — not just the vault.
No user-facing address moves. Every per-token contract is a
BeaconProxy(confirmed on Base: beacon slot populated, implementation slot empty), with
beacons owned by the Safe at
0xe70d821f…. Token addresses, beacon addressesand roles are all unaffected.
upgrade. Three singletons must be deployed and pointed at in lockstep:
StoxReceiptVaultStoxCorporateActionsFacetLibProdDeployCurrent.STOX_CORPORATE_ACTIONS_FACETinto itsfallback(); that address moved, so a vault built from this source delegatecalls into empty code until the facet is redeployedStoxReceiptST0xOrchestrator's vault-logic version lock compares live beacon implementations againstLibProdDeployCurrent; the receipt's address moved even though its source did not, so the orchestrator halts mint/burn until a new receipt impl is deployed and its beacon repointedThe facet coupling was previously invisible: the facet's bytecode had been
unchanged since 0.1.1, so its candidate and
0_1_1addresses coincided. OnlyCI's fork tests surfaced it — worth knowing before the deploy is planned.
Pin bookkeeping. The
testDeployAddress*assertions forStoxReceipt,StoxWrappedTokenVault,StoxWrappedTokenVaultBeaconand both authorizers arere-pointed from the frozen
_0_1_1pins to_CANDIDATE, matching whatStoxReceiptVaultalready did.0_1_1stays as the frozen historical record ofwhat is live on Base;
testFrozenRedeploy*keeps proving those snapshotsredeploy reproducibly independent of the current optimizer setting, and the
on-chain fork codehash tests still compare live code against the unchanged
0_1_1pins. No new release tag is cut here — that is a release-time action.What is genuinely given up. A fresh build of current source no longer
reproduces the previously released artifacts; verifying those against the repo
means using the frozen snapshot rather than a fresh build. Deploying the stack to
a new chain must likewise use the frozen creation code to keep addresses
identical to Base.
StoxCrossChainParitycompares implementation addressesacross chains by reading them on-chain, so existing parity is unaffected.
Gas. Runtime gas rises across all contracts at 2000 runs.
.gas-snapshotisconsequently stale; it is not gate-checked by CI (
rainix-sol-testis justforge test -vvv) and should be regenerated in CI where the fork-test RPCsecrets are available.
Worth noting regardless: a 6-byte margin meant any vault change was blocked,
not just this one.
Fork-test fixtures
CI's fork tests caught two fixtures that silently depended on current source
reproducing the 0.1.1 bytecode — an assumption the optimizer change breaks.
Both are fixed at the root rather than re-pinned to new constants:
V3UpgradeShadowForkplanted the corporate-actions facet at a hardcoded_0_1_1address while planting a freshly-compiled vault that pins_CANDIDATE. It now plants the facet atLibProdDeployCurrent.STOX_CORPORATE_ACTIONS_FACET— the same source thevault reads — so the two cannot drift apart again.
20260619-deploy-v4-authoriser-cloneetched a freshly-compiled authoriserat the pinned address, with NatSpec asserting its codehash "matches the
LibProdDeployV4pin by construction". It now etches the frozenRUNTIME_CODE_0_1_1snapshot, which is what the script's codehash guardactually checks against. The production guard is unchanged.
CI status
Green —
test,static,legalandgit-cleanall pass.Locally, with a Base fork against a public endpoint, 739 pass and the only
failures are RPC infrastructure (other chains' env vars, plus public-endpoint
rate limits) — zero assertion or revert mismatches.
rainix-sol-static(slitheracross 135 contracts, 0 results, plus
forge fmt --check) andrainix-sol-legalboth pass locally too.Summary by CodeRabbit
Bug Fixes
Deployment & Reliability