fix(payment): bind single-node payment to on-chain recipient + price floor (F2/F5 free storage)#101
Conversation
There was a problem hiding this comment.
Pull request overview
Security fix for findings F2/F5 (free storage) in the single-node payment path. The previous code delegated acceptance to SingleNodePayment::verify (median-based) and to validate_local_recipient (which only required this node's rewards address to appear in some quote), letting an attacker either underprice with 1-atto self-signed quotes or register an on-chain completedPayments record for a victim-addressed quote hash while routing the actual ANT to their own wallet. The new verify_evm_payment requires a single quote Q to simultaneously (a) pay this node, (b) meet a live per-record price floor, and (c) have an on-chain completedPayments(Q.hash) whose amount >= 3 * Q.price and rewardsAddress == first16(local_rewards_address).
Changes:
- Rewrites the single-node branch of
PaymentVerifier::verify_evm_paymentto enforce the on-chain recipient binding and a price floor, bounded to ≤7 candidate RPCs, fail-closed on RPC error. - Adds
PriceFloorProviderand aprice_floor: Option<PriceFloorProvider>field onPaymentVerifierConfig; wires it in production (node.rs,devnet.rs) by sharing oneArc<QuotingMetricsTracker>between quote generator and verifier;QuoteGenerator::newnow takesimpl Into<Arc<QuotingMetricsTracker>>. - New offline PoC tests (
tests/poc_f2_f5_price_floor.rs) and live-Anvil e2e tests (tests/e2e/f2_f5_pay_yourself.rs) covering underpricing, pay-yourself decoy, flip-control, and honest payments.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/payment/verifier.rs | Replaces median-based single-node check with per-candidate recipient + price-floor + on-chain bytes16 recipient binding. |
| src/payment/mod.rs | Re-exports new PriceFloorProvider. |
| src/payment/quote.rs | QuoteGenerator now stores Arc<QuotingMetricsTracker> so the verifier shares the live metrics. |
| src/node.rs | Wires the shared metrics tracker and a live PriceFloorProvider into the production verifier. |
| src/devnet.rs | Same wiring for the devnet harness. |
| src/storage/handler.rs | Adds price_floor: None to the test config. |
| tests/poc_f2_f5_price_floor.rs | New offline regression tests for F2/F5 (pre-RPC gate). |
| tests/e2e/f2_f5_pay_yourself.rs | New live-Anvil tests exercising the real payForQuotes pay-yourself attack and an honest positive control. |
| tests/e2e/mod.rs | Registers the new e2e module. |
| tests/e2e/testnet.rs | Adds price_floor: None to the test verifier config. |
| tests/e2e/data_types/chunk.rs | Same. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The median quote's price must be at least | ||
| /// `local_price / PRICE_FLOOR_TOLERANCE_DIVISOR`, where `local_price` is what | ||
| /// this node would itself quote right now (`calculate_price(records_stored)`). | ||
| /// | ||
| /// Why a divisor rather than exact equality: a single-node proof carries 7 | ||
| /// close-group quotes and the *median* is checked. Close-group peers | ||
| /// legitimately differ in `records_stored` (and therefore price), and a | ||
| /// quote can be up to `QUOTE_MAX_AGE_SECS` old (price grows monotonically | ||
| /// with this node's stored count, so our own floor only rises after a quote | ||
| /// was issued). A divisor of 4 tolerates a 4× legitimate spread between this | ||
| /// node's current price and an honest median — far more than real | ||
| /// close-group variance — while still rejecting the F2/F5 attack, where the | ||
| /// attacker's self-signed median is priced at 1 atto / baseline, i.e. many | ||
| /// orders of magnitude below `local_price` on any non-empty node. |
| }))), | ||
| }) | ||
| } | ||
|
|
…ent + price floor (F2/F5)
The single-node payment path had no DHT/identity binding (unlike merkle)
and delegated the decision to SingleNodePayment::verify, which accepts if
ANY quote tied at the median price was paid 3x on-chain. Combined with
validate_local_recipient only checking .any(), and with
PaymentVaultV2.payForQuotes being unauthenticated (caller sets
{quoteHash, rewardsAddress, amount} independently and the ANT goes to the
caller-chosen address), an attacker could:
* underprice: submit 7 self-signed 1-atto quotes; and/or
* pay-yourself: register an on-chain completedPayments record for a
victim-addressed quote's hash while sending the ANT to their OWN
wallet.
Either way the node stored data while earning nothing — free storage.
Fix: verify_evm_payment no longer calls SingleNodePayment::verify. It
enforces a sound invariant — accept only if there exists a quote Q with
ALL of, on the SAME quote:
(a) Q.rewards_address == this node's local_rewards_address,
(b) Q.price >= price_floor (calculate_price(records_stored)/TOL, wired
live in production via a shared Arc<QuotingMetricsTracker>),
(c) on-chain completedPayments(Q.hash).amount >= 3 * Q.price, AND
(d) on-chain completedPayments(Q.hash).rewardsAddress ==
first16(local_rewards_address) — the trusted on-chain record, not
the attacker-controlled quote object.
(d) is the decisive check that closes pay-yourself: the contract stores
the recipient it actually paid (bytes16, matching getFirst16Bytes); the
verifier now requires it to be THIS node. bytes16 truncation leaves
128-bit preimage resistance (an EVM address is keccak256(pubkey)[12..],
not attacker-structured), cryptographically infeasible to forge.
PaymentVerifierConfig gains an optional PriceFloorProvider; production
(node.rs, devnet.rs) wires it to the same metrics tracker the quote
generator prices from. validate_quote_structure still enforces exactly
CLOSE_GROUP_SIZE quotes; merkle path untouched; fail-closed on RPC
error; candidate loop bounded to <=7.
Proof: tests/poc_f2_f5_price_floor.rs (offline, 4 tests: underpriced &
pay-yourself-decoy rejected pre-RPC, flip without binding, fair payment
not pre-rejected) and tests/e2e/f2_f5_pay_yourself.rs (live Anvil: the
REAL pay-yourself payForQuotes tx is rejected; honest payment to the
node accepted). The flip was verified by neutering the recipient
binding — the e2e attack then returns PaymentVerified, i.e. the pre-fix
free-storage. Three rounds of multi-agent adversarial review; all
blockers resolved. 460 lib tests pass; e2e compiles; cfd clean.
…, doc Per review-wave findings on PR WithAutonomi#101: 1. Hydrate `records_stored` from the LMDB row count on startup so the price floor reflects this node's actual disk state immediately. Prior to this, every restart reset the in-memory counter to 0 and the floor to `baseline/4` until live PUTs dragged it back up — weakening the F2/F5 underpricing defence across restarts (the recipient binding still defeated pay-yourself unconditionally, but the doc claim "live to records_stored" was stronger than the code delivered). 2. Move tests/poc_f2_f5_price_floor.rs into tests/e2e/ so the registered `e2e` test target picks it up. Previously the file was a free-standing integration test that CI never ran (CI only invokes `--lib` and `--test e2e`). Also widen the post-RPC assertions to accept either the "paid 0" message (RPC reachable) or the "lookup failed" message (offline CI), since both prove the proof passed the pre-RPC gate and only died at the on-chain step — which is what the test is testing. 3. Update the stale doc on PRICE_FLOOR_TOLERANCE_DIVISOR: the new code filters every candidate per-quote and no longer uses median selection.
9947154 to
8dcf26b
Compare
…docs Second-round reviewers flagged two doc strings that still described the floor as a "median-priced quote" check — left over from the original SingleNodePayment::verify delegation. The new code filters every candidate per-quote and never selects a median, so those doc lines were misleading. PriceFloorProvider's doc and the price_floor field's doc now describe per-candidate enforcement.
| fn verifier_with_floor(victim_rewards: RewardsAddress, floor_atto: u128) -> PaymentVerifier { | ||
| PaymentVerifier::new(PaymentVerifierConfig { | ||
| evm: EvmVerifierConfig::default(), | ||
| cache_capacity: 64, | ||
| local_rewards_address: victim_rewards, | ||
| price_floor: Some(PriceFloorProvider::new(Arc::new(move || { | ||
| Amount::from(floor_atto) | ||
| }))), | ||
| }) |
…ayment rustdoc Per round-2 reviewers on PR WithAutonomi#101: [SHOULD-FIX] `verify_evm_payment`'s rustdoc still listed step 7 as "the median-priced quote was paid at least 3x its price on-chain" — the same category of stale wording the prior commit set out to remove. Rewritten to describe the actual per-candidate (a)/(b)/(c)/(d) binding the code enforces: recipient match, price floor, on-chain amount, on-chain recipient. [NIT] The in-function invariant comment used (a)/(b)/(c) where (c) covered both amount and recipient, while node.rs and the new rustdoc used (a)/(b)/(c)/(d) splitting them. Unified everywhere on (a)=recipient, (b)=floor, (c)=on-chain amount >= 3x, (d)=on-chain recipient match. No code-behaviour change; docs/comments only.
dirvine
left a comment
There was a problem hiding this comment.
Review: PR #101 — F2/F5 free storage fix
Architecture & Soundness
The fix is structurally rigorous. The four-condition invariant (a-d) on the same quote correctly closes both attacker primitives:
- Underpricing → (b) price floor kills 1-atto self-signed quotes.
- Pay-yourself → (d) on-chain recipient binding kills redirected payments; condition (a) already ensures the quote pays this node, so the attacker cannot use a victim-addressed quote while paying themselves.
Removing the SingleNodePayment::verify delegation (which trusted the attacker-supplied median) and replacing with per-candidate enforcement is the right architectural change.
Security analysis of bytes16 truncation
I verified the preimage argument: EVM addresses are keccak256(pubkey)[12..]. An attacker cannot freely structure the 16 bytes; they must find a pubkey whose hash suffix matches a specific target. This is ~2^128 preimage work, not ~2^64 birthday-collision work. The analysis in the PR description is correct.
Implementation
- PriceFloorProvider sharing via Arc is clean; the Into<Arc<...>> bound on QuoteGenerator::new is a nice API touch for test compatibility.
- Fail-closed on RPC error is correct.
- Candidate loop bounded to <=7.
- Startup hydration from storage.current_chunks() — a nice hardening detail.
- Tests: offline PoCs + live Anvil e2e with flip verification = thorough.
Residual notes
- The price floor tolerance divisor of 4 is a judgement call. Not blocking.
Verdict: LGTM — sound, well-tested, merge-ready.
Summary
Security fix for F2/F5 (Critical — free storage). The single-node payment path had no DHT/identity binding (unlike the merkle path) and delegated the decision to
SingleNodePayment::verify, which accepts if any quote tied at the median price was paid 3× on-chain. Combined withvalidate_local_recipientonly checking.any(), and withPaymentVaultV2.payForQuotesbeing unauthenticated (the caller sets{quoteHash, rewardsAddress, amount}independently and the ANT is sent to the caller-chosen address), an attacker could:completedPaymentsrecord for a victim-addressed quote's hash while sending the ANT to their own wallet.Either way the node stored data while earning nothing — free storage.
Fix — sound invariant
verify_evm_paymentno longer callsSingleNodePayment::verify. It accepts only if there exists a quoteQwith all of the following, on the same quote:Q.rewards_address == this node's local_rewards_addressQ.price >= price_floor—calculate_price(records_stored)/TOL, wired live in production via a sharedArc<QuotingMetricsTracker>(the same tracker the quote generator prices from)completedPayments(Q.hash).amount >= 3 * Q.pricecompletedPayments(Q.hash).rewardsAddress == first16(local_rewards_address)— the trusted on-chain record, not the attacker-controlled quote object(d) is the decisive check that closes pay-yourself: the contract stores the recipient it actually paid (
bytes16, matchinggetFirst16Bytes); the verifier now requires that to be this node. Thebytes16truncation leaves 128-bit preimage resistance — an EVM address iskeccak256(pubkey)[12..], not an attacker-structuredhigh16 ++ low4, so forging a colliding address is ~2^128 work (cryptographically infeasible), not 2^32.PaymentVerifierConfiggains an optionalPriceFloorProvider; production (node.rs,devnet.rs) wires it to the metrics tracker.validate_quote_structurestill enforces exactlyCLOSE_GROUP_SIZEquotes; the merkle path is untouched; the on-chain check is fail-closed on RPC error; the candidate loop is bounded to ≤7.Proof
tests/poc_f2_f5_price_floor.rs— 4 offline deterministic tests: underpriced and pay-yourself-decoy proofs rejected at the pre-RPC recipient+floor gate; flip (without the binding the proof passes the gate and reaches the on-chain step); fair payment not pre-rejected.tests/e2e/f2_f5_pay_yourself.rs— live Anvil with the realPaymentVaultV2: the real pay-yourselfpayForQuotestransaction (attacker pays 3× to their own wallet for a victim-addressed quote hash) is rejected; an honest 3× payment to the node's address is accepted (positive control).The flip was verified by neutering the recipient binding: the e2e pay-yourself attack then returns
PaymentVerified— i.e. the pre-fix free-storage acceptance, confirming the vulnerability was real and that the on-chain recipient binding closes it.Validation
--features test-utils).cfdclean (fmt + clippy, no warnings/errors).bytes16truncation does not reopen the attack.Notes / out of scope
bytes16recipient truncation is the contract's design (PaymentVaultV2); the node-side check matches it exactly and 128-bit resistance is sufficient.