Dust accounting: per-(user, token) credit ledger realized on the next token move (#2673)#2702
Open
thedavidmeister wants to merge 9 commits into
Open
Dust accounting: per-(user, token) credit ledger realized on the next token move (#2673)#2702thedavidmeister wants to merge 9 commits into
thedavidmeister wants to merge 9 commits into
Conversation
Lossy float->fixed-decimal token moves leave a sub-base-unit remainder: `pullTokens` rounds the transfer UP (the orderbook receives the excess), `pushTokens` rounds it DOWN (the orderbook keeps the shortfall). Both residues are real tokens the orderbook already holds on behalf of the user. Rather than stranding them as orphaned dust, book the residue into a new `sDustCredit[user][token]` ledger (token units) and realize it into the next move for the same `(user, token)`: - pull reduces the amount pulled by any standing credit, so a whole base unit of accumulated credit is recovered instead of re-pulled; - push increases the amount pushed by any standing credit, so an accumulated whole base unit is paid out instead of re-truncated. Each conversion stays conservative (pull rounds up, push rounds down) but the remainder is now exactly accounted instead of lost. The credit is always non-negative and under one base unit, and is always backed by tokens the orderbook provably holds for that user, so realizing it never dips into a vault balance: the solvency invariant (real balance >= sum of vault balances + tracked credits) is preserved. To keep RaindexV6 under EIP-170, factor the duplicated TOFU decimals-read that opened both pull/push into a shared `_tokenDecimals` helper. Closes #2673 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 33 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (17)
✨ 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 |
These tests were written by independent skeptic reviewers during the #2702 solvency verification to attack the per-(user, token) dust-credit ledger. They are committed here verbatim (tests only — no source changes) so the adversarial coverage and the regression they uncovered are preserved on the PR branch. - test/concrete/raindex/RaindexV6.dustCredit.solvencyAttack.t.sol Passing coverage. Cross-account pooled solvency, same-tx arrive-and-leave on one token, large-amount push (credit-not-inflated), and cumulative never-over-pay. All green against the current bytecode. - test/concrete/raindex/RaindexV6.dustCredit.conservationAttack.t.sol Passing coverage. Round-trip/boundary conservation across decimals (0..60), int224 boundary, high-decimals credit-not-dropped, and a wide fuzz. All green. - test/concrete/raindex/RaindexV6.dustCredit.precisionAttack.t.sol CURRENTLY-FAILING regression gate. Two tests fail against #2702's current (buggy) code and document a solvency shortfall that appears at very large amounts (above ~1e50 token units): testTwoPullSolvencySlack shows the orderbook taking in fewer base units than it books as owed (a 1e68-base-unit obligation backed by 1e68 - 8), and testPullMagnitudeScanConservation shows movedTotal != netRequested + credit once the exponent gap drops the standing credit. These are intentionally red — they pin the regression and must not be made green by touching source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… Float ceiling (#2673) `pullTokens` computed `effective = amount - credit` then rounded the fixed-decimal conversion UP only when `toFixedDecimalLossy` reported the conversion lossy. Near the Float int224 coefficient ceiling (~1e50 whole tokens) two effects compound: `amount.sub(credit)` drops the sub-base-unit credit, and the conversion of the resulting `effective` reports a spuriously-lossless value that truncates DOWN by more than one base unit. The round-up never fires, so the orderbook pulled in fewer base units than the vault obligation it booked - an untracked shortfall that broke solvency (real balance < sum of vault balances + credits) at high magnitudes. Fix: alongside the credit-reduced `effective`, also convert the full requested `amount` to base units (the conservative obligation a zero-credit pull would take). The standing credit can only ever reduce the pull by under one base unit, so when the rounded pull lands two or more base units below the obligation the credit was dropped; clamp the pull up to the obligation so the orderbook is never short. A clamped pull moved exactly `amount` in base units (the standing credit was sub-resolution beside it and could not reduce the pull), so the standing credit carries forward unchanged; the normal path keeps the original `pulled - effective` credit. RaindexV6 deployed runtime is 24,566 bytes (10 under the EIP-170 24,576 limit). The new precisionAttack regression gates (testTwoPullSolvencySlack, testPullMagnitudeScanConservation) and the conservation/solvency fuzz suites pass; the bug-characterizing precisionAttack diagnostics are updated to assert the now-solvent behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…recision The dust-credit ledger adds an sDustCredit Float slot that pullTokens and pushTokens always SLOAD and SSTORE, and whose realization shifts the exact base-unit transfer amounts when a single address is both depositor/taker/owner. Both effects are legitimate new behavior; this updates the test expectations to match it. No source change is required. Read/write-count assertions (each Float slot access is 2 SLOADs; the unconditional store is 1 SSTORE): - deposit testDepositMany: reads 5 -> 7 (+2 dust SLOAD in pullTokens), writes pinned to 4 (+1 dust SSTORE). - withdraw testWithdrawEmptyVault: reads 6 -> 8, writes 3 -> 4 (the noop still calls pushTokens, which reads and writes the dust slot). - deposit.entask checkReentrancyRW: reads 5 -> 7, writes 3 -> 4. - withdraw.entask checkReentrancyRW: reads 6 -> 8, writes 3 -> 4. takeOrder.precision KnownBad07-11 and KnownBad01BothVaultIdZero: investigated and confirmed a boundary shift, NOT a precision regression. checkPrecision reuses address(this) as depositor, taker, and order owner, so the lossy seed deposit (or taker-payment pull) over-pulls by ceil-exact and books it as dust credit for that (account, token); the matching push then realizes it, sending floor(exact + credit) == ceil instead of the truncated floor(exact). The Float accounting (totalTakerInput/Output equality and the output vault ending at zero) is unchanged and conservation holds: the taker receives back exactly the sub-unit dust it over-paid. The transfer mocks now expect the ceil amount in those coupled cases. forge test --match-path 'test/concrete/raindex/*' is green (0 failures) with pointers regenerated locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts: # src/concrete/raindex/RaindexV6.sol
The dust-credit ledger changes RaindexV6 bytecode, so its CREATE2 address moves; the three arb takers embed that address and re-emit in turn. BuildPointers run to a fixpoint (RaindexV6, GenericPoolArbOrderTaker, RouteProcessorArbOrderTaker, GenericPoolFlashBorrower all settle; SubParser and RouteProcessor4 unchanged). The next-version (0.1.10) deploy-constant suite is pre-pinned in LibRaindexDeploy so the publish lands without a red main. build.sh regenerated the test_fixtures RaindexV6 abi+bytecode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Descoped from the current Protofire formal audit (maintainer decision). The dust credit ledger — new per-(user,token) storage slot + lossy-Float residue handling + near-ceiling solvency clamp on the core pullTokens/pushTokens settlement path — is a larger change deferred to a future audit/deploy round; it will NOT be part of the bytecode being audited now. Keeping this PR open for that later round (will need a fresh rebase + pointer/constant regen + deploy at that time). |
…mpt] Each dustCredit test file had a Harness contract and a Test contract in the same file, violating the rainix-sol-single-contract static check. Extract each harness (and the PrecisionAttackMutableDecimalsToken helper) into its own .sol file and import it from the test file. Co-Authored-By: Claude <noreply@anthropic.com>
forge fmt reformats multi-line import statements to curly-brace style
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.
Closes #2673.
Approach (per maintainer decision)
Every lossy float->fixed-decimal token move leaves a sub-base-unit remainder.
pullTokensrounds the transfer up (the orderbook receives the excess) andpushTokensrounds it down (the orderbook keeps the shortfall). In both cases the residue is real tokens the orderbook already holds on behalf ofaccount. Instead of stranding it as orphaned dust (audit L01), it is booked into a new per-(user, token)ledger and realized into the next move for that same pair.Storage
The credit is denominated in token units (a
Float), so it is independent of the token's decimals. It is always0 <= credit < 1 base unit.pullTokens(rounds up)effective = amount - credit— only pull the part ofamountthe standing credit does not already cover.effective > 0: convert to base units, round up on loss,safeTransferFromthat many.effective <= 0: the credit covers the whole pull, so no tokens move (toFixedDecimalLossyalso rejects negatives).= pulled - effective, wherepulledis the actually-transferred base units back as aFloat(pulled = 0when nothing moved). This is the over-pull when tokens moved (non-negative, under one base unit because rounding adds at most one), or the leftover credit when none did.pushTokens(rounds down)effective = amount + credit— the orderbook may send what it owes this move plus the standing credit.safeTransferthat many.= effective - pushed, the shortfall truncated away (non-negative, under one base unit).Invariant-preservation argument
Let
Rbe the orderbook's real token balance (base units, as aFloatvia the decimals round-trip),C = sDustCredit[user][token], andnetRequestedthe sum of requested pull amounts minus push amounts for that pair (the exact economic value the vault accounting moved). The contract maintains, per(user, token):Per move, with
tthe transferred base units (+ton pull,-ton push):t + C_old = amount + C_newby construction (C_new = pulled - effective = t - (amount - C_old)), soΔR = t = amount + ΔC = Δ(netRequested) + ΔC.t = effective - C_new = (amount + C_old) - C_new, soΔR = -t = -amount + ΔC = Δ(netRequested) + ΔC.Both give
ΔR = Δ(netRequested) + ΔC, i.e.R - C = netRequestedis invariant — every base unit that physically moved is accounted for by the requested economic value plus the standing credit. No dust is created or destroyed.Solvency.
Cis always>= 0and< 1 base unit, and every credit arises from tokens the orderbook over-held (pull rounded up) or under-paid (push rounded down) — i.e. tokens it already holds for that user. Realizing a credit only ever releases that provable excess, sois preserved and a move can never over-credit or dip into a vault balance. The pull/push rounding direction is unchanged (still pull-up / push-down per move); only the residue handling changed from "strand" to "ledger".
Precision boundary (for review). The credit is derived by measuring the actually transferred base units and rounding them back to a
Float(fromFixedDecimalLossyPacked), which is lossless for any transfer up toint256.maxbase units — far beyond realistic token economics at any supported decimals. Above that threshold the round-trip can drop low digits; the resulting credit would be biased downward (slightly smaller / even slightly negative), which is conservative — it favours the protocol and pulls more / pushes less on the next move, never over-crediting. Flagging this as the one spot worth a reviewer's eye, per the "starting point for REVIEW" framing.Tests
test/concrete/raindex/RaindexV6.dustCredit.t.solexercises the new behaviour against a freshly-compiledRaindexV6harness (so the logic is directly validated and mutation-testable):testConservationAndSolvencyInvariantfuzzes arbitrary pull/push sequences and assertsrealDelta == netRequested + creditafter every move, plus0 <= credit < 1 base unit.testPullRealizesCreditIntoNextPull,testPushRealizesCreditIntoNextPush,testPushAccumulatesSubUnitToWholeUnit,testPullCoveredEntirelyByCredit,testPushCreditAddsExtraUnit.testLosslessMoveLeavesCreditZero,testCreditIsolatedPerUser,testDustCreditStartsZero.The pre-existing
RaindexV6.vaultBalanceRevert.t.solrounding tests (pull-up / push-down, zero-skips-transfer, sub-unit boundaries) still pass: they start from an empty ledger, where the new logic is identical to the old per-move rounding.Deploy / size
RaindexV6runtime is 24,509 bytes (67 under the EIP-170 24,576 limit) after this change. The dust-credit logic costs bytecode, so the duplicated TOFU decimals-read block that opened bothpullTokens/pushTokensis factored into one_tokenDecimalshelper to make room;optimizer_runsis left at 800.needsDeploy = true: this is a deployed-bytecode change. The Solidity tests etch the committed (stale)src/generated/RaindexV6.pointers.solruntime code, so the standard etched-harness suites will not see the new behaviour until the contract is redeployed and pointers regenerated — an expected red that is itself the mutation proof. The newRaindexV6.dustCredit.t.soltests instead deploy a freshly compiled harness (matching the existingRaindexV6.vaultBalanceRevert.t.solpattern), so the new logic is validated and mutation-tested here even before deploy. Per the task scope, this PR does not regenerate pointers, pin constants, or deploy.🤖 Generated with Claude Code