Skip to content

Commit 2bfa50f

Browse files
tmigoneMoonBoi9001claude
authored
TRST-H-2 to H-6: Consolidated fixes from Trust Security audit (#1010)
* fix(service): TRST-H-2 -- reject V2 receipts with non-zero collection_id prefix The AllocationEligible check only examines the trailing 20 bytes of a 32-byte collection_id, while storage persists all 32 bytes. The tap-agent reconstructs collection_id with zero-padded prefix, so receipts with non-zero prefix bytes pass validation but are invisible to RAV aggregation, enabling theft of service. Reject any V2 receipt where collection_id[0..12] is not all zeros. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-3 -- add collector filter to V2 escrow account query The V2 escrow query did not filter by collector, allowing an attacker to inflate a payer's perceived balance by depositing to a fake collector contract. Adds a required graph_tally_collector_address config field and passes it as a collector filter in the GraphQL query. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-4 -- add pagination and balance filter to V2 escrow account query The V2 escrow accounts GraphQL query omitted pagination parameters, defaulting to first: 100. An attacker could deposit 1 wei from hundreds of fake payer addresses to crowd out legitimate payers, blinding the indexer to all real balances and signer mappings. Add skip/first pagination with a loop that accumulates all pages, pin to block hash from the first page for consistent reads, filter out accounts below 0.1 GRT at the subgraph level, and cap nested signers at 1000 per payer. Log page and account counts after pagination and warn when pages exceed 5 or any payer hits the signer cap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tap-agent): TRST-H-5 -- apply rav_request_timeout to gRPC Endpoint The rav_request_timeout config field was defined and validated but never applied to the tonic Endpoint. A malicious aggregator could hang the AggregateReceipts RPC indefinitely, permanently blocking the SenderAllocation actor and bypassing sender denial checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tap-agent): TRST-H-6 -- use strict signer set for RAV signature verification The tap-agent was using a single escrow accounts watcher with thawing signers included for both receipt processing and RAV verification. A payer could sign RAVs with a thawing signer, then revoke it before the indexer redeems. The RAV becomes unredeemable on-chain but the tap-agent has already removed receipt values from its funds-at-risk tracker. Create a second escrow accounts watcher with reject_thawing_signers=true and thread it through to TapAgentContext for RAV signature verification. Receipt processing continues using the permissive watcher. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-3 -- use scalar collector filter in V2 escrow query The collector_ nested relationship filter is rejected by the live subgraph. Switch to the scalar collector: $collector form, which is confirmed working against the deployed network subgraph. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tap-agent): TRST-H-4 -- persist signers locally and replace startup panic The nested signers sub-query in the V2 escrow account query caps at first: 1000. A malicious payer can authorize enough signers to crowd out a legitimate one, causing get_pending_sender_allocation_id_v2() to panic via .expect() and crash the entire tap-agent process. Add a tap_escrow_signers cache table that is upserted on every escrow accounts poll. When a signer lookup against the live subgraph data fails, both the startup path and the receipt notification handler fall back to the local cache. If neither source resolves the signer, skip it with a warning and increment tap_signer_lookup_failures_total. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-4 -- use cursor-based pagination for V2 escrow query Graph Node caps skip at 5000, so the previous skip-based pagination would fail after 25 pages (5000 / 200 page size). Replace with cursor-based pagination using id_gt, which has no upper bound. This matches the existing pattern used by allocations.query.graphql. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(tap-agent): TRST-H-6 -- pass both escrow watchers through TapAgentContext TapAgentContext previously received only the strict escrow watcher (excluding thawing signers), but receipt retrieval and deletion also used it. This caused receipts from thawing signers to become invisible to the TAP manager. Now the context carries both watchers: the full set for receipt operations and the strict set for RAV signature verification only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-4 -- replace signer cache with nested signer pagination The database cache (tap_escrow_signers) was only consulted in two places in sender_accounts_manager, while all downstream consumers still used the raw EscrowAccounts watcher. This left crowded-out signers invisible for fee calculations, RAV aggregation, and cleanup. Replace the cache with cursor-based follow-up pagination so EscrowAccounts is complete at the source. Also makes the 0.1 GRT balance filter configurable via escrow_min_balance_grt_wei. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-4 -- default to unlimited signer pagination The previous default of max_signers_per_payer = 1000 silently truncated signers beyond the cap, enabling the same free-queries attack as the original vulnerability at a marginally higher cost to the attacker. Default to 0 (no limit) so all signers are fetched and no receipts are orphaned. A warning fires when any payer exceeds 1000 signers. The cap remains configurable for operators who accept the trade-off. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(monitor): TRST-H-4 -- tighten signer pagination cap and ordering Three follow-ups to 28f3b10: - router.rs fallback default was still 1000 when no network subgraph is configured; align with the unlimited (0) default introduced in 28f3b10. - Nested signers selection in the V2 escrow query lacked explicit ordering, so the 1000-item page cap returned a non-deterministic prefix and the follow-up SignersByPayerQuery cursor could skip or duplicate signers. Add orderBy: id, orderDirection: asc. - The max_signers_per_payer truncation ran after the "fewer than 1000 returned, skip pagination" shortcut, so an operator cap below 1000 was silently ignored when the initial query already exceeded it (e.g. cap=200, returned=500). Swap the two checks so the cap is enforced first. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: fix missing fields Signed-off-by: Tomás Migone <tomas@edgeandnode.com> --------- Signed-off-by: Tomás Migone <tomas@edgeandnode.com> Co-authored-by: MoonBoi9001 <samuelametcalfe@yahoo.co.uk> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9fa1471 commit 2bfa50f

23 files changed

Lines changed: 664 additions & 86 deletions

crates/config/default_values.toml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ port = 7300
55
syncing_interval_secs = 60
66
recently_closed_allocation_buffer_secs = 3600
77
max_data_staleness_mins = 30
8+
escrow_min_balance_grt_wei = "100000000000000000"
9+
max_signers_per_payer = 0
810

911
[subgraphs.escrow]
1012
syncing_interval_secs = 60
@@ -46,6 +48,8 @@ chain_id = 1337
4648
receipts_verifier_address_v2 = "0x3333333333333333333333333333333333333333"
4749
# SubgraphService contract (required).
4850
subgraph_service_address = "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9"
51+
# GraphTallyCollector contract (required for V2 escrow filtering).
52+
graph_tally_collector_address = "0x4444444444444444444444444444444444444444"
4953

5054
[horizon]
5155
# Enable Horizon support and detection

crates/config/maximal-config-example.toml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,13 @@ recently_closed_allocation_buffer_secs = 3600
8383
# This protects against Gateway routing queries to indexers that are significantly behind.
8484
# Set to 0 to disable staleness checking.
8585
max_data_staleness_mins = 30
86+
# Minimum escrow balance (GRT wei) for V2 escrow queries. Filters dust deposits
87+
# to raise the cost of crowding attacks. Default: 0.1 GRT.
88+
escrow_min_balance_grt_wei = "100000000000000000"
89+
# Maximum signers to fetch per payer. 0 = no limit (recommended).
90+
# A positive value caps pagination but allows attackers to crowd out legitimate
91+
# signers, orphaning receipts and enabling free queries.
92+
max_signers_per_payer = 0
8693

8794
[subgraphs.escrow]
8895
# NOTE: It is heavily recomended to use both `query_url` and `deployment_id`,
@@ -106,6 +113,7 @@ chain_id = 1337
106113
# Horizon addresses; required when [horizon].enabled = true
107114
receipts_verifier_address_v2 = "0x3333333333333333333333333333333333333333"
108115
subgraph_service_address = "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9"
116+
graph_tally_collector_address = "0x4444444444444444444444444444444444444444"
109117

110118
##############################################
111119
# Specific configurations to indexer-service #

crates/config/minimal-config-example.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ chain_id = 1337
6161
# Horizon addresses; required when [horizon].enabled = true
6262
receipts_verifier_address_v2 = "0x3333333333333333333333333333333333333333"
6363
subgraph_service_address = "0xcf7ed3acca5a467e9e704c703e8d87f634fb0fc9"
64+
graph_tally_collector_address = "0x4444444444444444444444444444444444444444"
6465

6566
########################################
6667
# Specific configurations to tap-agent #

crates/config/src/config.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,12 +498,32 @@ pub struct NetworkSubgraphConfig {
498498
/// Default: 30 (minutes)
499499
#[serde(default = "default_max_data_staleness_mins")]
500500
pub max_data_staleness_mins: u64,
501+
502+
/// Minimum escrow balance (GRT wei) for the V2 escrow query. Filters dust
503+
/// deposits to raise the cost of crowding attacks. Default: 0.1 GRT.
504+
#[serde(default = "default_escrow_min_balance_grt_wei")]
505+
pub escrow_min_balance_grt_wei: String,
506+
507+
/// Maximum signers to fetch per payer. 0 = no limit (recommended).
508+
/// Setting a positive value caps pagination but allows attackers to crowd out
509+
/// legitimate signers, orphaning receipts and enabling free queries.
510+
/// Default: 0.
511+
#[serde(default = "default_max_signers_per_payer")]
512+
pub max_signers_per_payer: usize,
501513
}
502514

503515
fn default_max_data_staleness_mins() -> u64 {
504516
30
505517
}
506518

519+
fn default_escrow_min_balance_grt_wei() -> String {
520+
"100000000000000000".to_string() // 0.1 GRT
521+
}
522+
523+
fn default_max_signers_per_payer() -> usize {
524+
0
525+
}
526+
507527
#[derive(Debug, Deserialize)]
508528
#[cfg_attr(test, derive(PartialEq))]
509529
pub struct EscrowSubgraphConfig {
@@ -546,6 +566,8 @@ pub struct BlockchainConfig {
546566
pub receipts_verifier_address_v2: Address,
547567
/// Address of the SubgraphService contract used for Horizon operations
548568
pub subgraph_service_address: Option<Address>,
569+
/// Address of the GraphTallyCollector contract used to filter V2 escrow account queries.
570+
pub graph_tally_collector_address: Address,
549571
}
550572

551573
impl BlockchainConfig {

crates/monitor/src/escrow_accounts.rs

Lines changed: 184 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -101,13 +101,6 @@ impl EscrowAccounts {
101101
pub fn contains_signer(&self, signer: &Address) -> bool {
102102
self.signers_to_senders.contains_key(signer)
103103
}
104-
105-
pub fn get_all_mappings(&self) -> Vec<(Address, Address)> {
106-
self.signers_to_senders
107-
.iter()
108-
.map(|(signer, sender)| (*signer, *sender))
109-
.collect()
110-
}
111104
}
112105

113106
pub type EscrowAccountsWatcher = Receiver<EscrowAccounts>;
@@ -135,9 +128,19 @@ pub async fn escrow_accounts_v2(
135128
indexer_address: Address,
136129
interval: Duration,
137130
reject_thawing_signers: bool,
131+
graph_tally_collector_address: Address,
132+
min_balance_grt_wei: String,
133+
max_signers_per_payer: usize,
138134
) -> Result<EscrowAccountsWatcher, anyhow::Error> {
139135
indexer_watcher::new_watcher(interval, move || {
140-
get_escrow_accounts_v2(escrow_subgraph, indexer_address, reject_thawing_signers)
136+
get_escrow_accounts_v2(
137+
escrow_subgraph,
138+
indexer_address,
139+
reject_thawing_signers,
140+
graph_tally_collector_address,
141+
min_balance_grt_wei.clone(),
142+
max_signers_per_payer,
143+
)
141144
})
142145
.await
143146
}
@@ -146,45 +149,180 @@ async fn get_escrow_accounts_v2(
146149
escrow_subgraph: &'static SubgraphClient,
147150
indexer_address: Address,
148151
reject_thawing_signers: bool,
152+
graph_tally_collector_address: Address,
153+
min_balance_grt_wei: String,
154+
max_signers_per_payer: usize,
149155
) -> anyhow::Result<EscrowAccounts> {
150156
tracing::trace!(
151157
indexer_address = ?indexer_address,
152158
reject_thawing_signers,
153159
"Loading V2 escrow accounts for indexer"
154160
);
155-
// Query V2 escrow accounts from the network subgraph which tracks PaymentsEscrow
156-
// and GraphTallyCollector contract events.
157161

158162
use indexer_query::network_escrow_account_v2::{
159-
self as network_escrow_account_v2, NetworkEscrowAccountQueryV2,
163+
self as network_escrow_account_v2, Block_height, NetworkEscrowAccountQueryV2,
160164
};
165+
use indexer_query::signers_by_payer::{self as signers_by_payer, SignersByPayerQuery};
161166

162-
let response = escrow_subgraph
163-
.query::<NetworkEscrowAccountQueryV2, _>(network_escrow_account_v2::Variables {
164-
receiver: format!("{indexer_address:x?}"),
165-
thaw_end_timestamp: if reject_thawing_signers {
166-
U256::ZERO.to_string()
167-
} else {
168-
U256::MAX.to_string()
169-
},
170-
})
171-
.await?;
167+
let page_size: i64 = 200;
168+
let mut last: Option<String> = None;
169+
let mut block_hash: Option<String> = None;
170+
let mut all_accounts = vec![];
172171

173-
// V2 TAP receipts use different field names (payer/service_provider) but the underlying
174-
// escrow account model is identical to V1. Both V1 and V2 receipts reference the same
175-
// sender addresses and the same escrow relationships.
176-
//
177-
// V1 queries the TAP subgraph while V2 queries the network subgraph, but both return
178-
// the same escrow account structure for processing.
179-
//
180-
// V2 receipt flow:
181-
// 1. V2 receipt contains payer address (equivalent to V1 sender)
182-
// 2. Receipt is signed by a signer authorized by the payer
183-
// 3. Escrow accounts map: signer -> payer (sender) -> balance
184-
// 4. Service provider (indexer) receives payments from payer's escrow
172+
let thaw_end_timestamp = if reject_thawing_signers {
173+
U256::ZERO.to_string()
174+
} else {
175+
U256::MAX.to_string()
176+
};
185177

186-
let senders_balances: HashMap<Address, U256> = response
187-
.payments_escrow_accounts
178+
let mut pages_fetched: u64 = 0;
179+
180+
loop {
181+
let response = escrow_subgraph
182+
.query::<NetworkEscrowAccountQueryV2, _>(network_escrow_account_v2::Variables {
183+
receiver: format!("{indexer_address:x?}"),
184+
collector: format!("{graph_tally_collector_address:x?}"),
185+
thaw_end_timestamp: thaw_end_timestamp.clone(),
186+
first: page_size,
187+
last: last.unwrap_or_default(),
188+
min_balance: min_balance_grt_wei.clone(),
189+
block: block_hash.as_ref().map(|h| Block_height {
190+
hash: Some(h.clone()),
191+
number: None,
192+
number_gte: None,
193+
}),
194+
})
195+
.await?;
196+
197+
let page_len = response.payments_escrow_accounts.len();
198+
pages_fetched += 1;
199+
200+
// Pin to the block hash from the first page for consistent reads across pages
201+
if block_hash.is_none() {
202+
block_hash = response.meta.as_ref().and_then(|m| m.block.hash.clone());
203+
}
204+
205+
// Paginate additional signers for any payer that hit the 1000-per-payer nested cap
206+
let mut response = response;
207+
for account in &mut response.payments_escrow_accounts {
208+
if max_signers_per_payer > 0 && account.payer.signers.len() >= max_signers_per_payer {
209+
tracing::warn!(
210+
payer = %account.payer.id,
211+
signers = account.payer.signers.len(),
212+
max = max_signers_per_payer,
213+
"Payer signers already at or above max_signers_per_payer cap; skipping follow-up pagination"
214+
);
215+
account.payer.signers.truncate(max_signers_per_payer);
216+
continue;
217+
}
218+
219+
if account.payer.signers.len() < 1000 {
220+
continue;
221+
}
222+
223+
let mut signer_cursor = account
224+
.payer
225+
.signers
226+
.last()
227+
.map(|s| s.id.clone())
228+
.unwrap_or_default();
229+
230+
tracing::info!(
231+
payer = %account.payer.id,
232+
initial_signers = account.payer.signers.len(),
233+
"Payer hit nested signer cap; starting follow-up pagination"
234+
);
235+
236+
loop {
237+
let signer_response = escrow_subgraph
238+
.query::<SignersByPayerQuery, _>(signers_by_payer::Variables {
239+
payer: account.payer.id.clone(),
240+
first: 1000,
241+
last: signer_cursor.clone(),
242+
thaw_end_timestamp: thaw_end_timestamp.clone(),
243+
block: block_hash.as_ref().map(|h| signers_by_payer::Block_height {
244+
hash: Some(h.clone()),
245+
number: None,
246+
number_gte: None,
247+
}),
248+
})
249+
.await?;
250+
251+
let page_len = signer_response.signers.len();
252+
253+
for signer in &signer_response.signers {
254+
account.payer.signers.push(
255+
network_escrow_account_v2::NetworkEscrowAccountQueryV2PaymentsEscrowAccountsPayerSigners {
256+
id: signer.id.clone(),
257+
},
258+
);
259+
}
260+
261+
if page_len < 1000
262+
|| (max_signers_per_payer > 0
263+
&& account.payer.signers.len() >= max_signers_per_payer)
264+
{
265+
break;
266+
}
267+
268+
signer_cursor = signer_response
269+
.signers
270+
.last()
271+
.map(|s| s.id.clone())
272+
.unwrap_or_default();
273+
}
274+
275+
if max_signers_per_payer > 0 && account.payer.signers.len() >= max_signers_per_payer {
276+
tracing::warn!(
277+
payer = %account.payer.id,
278+
signers = account.payer.signers.len(),
279+
max = max_signers_per_payer,
280+
"Payer signers capped at max_signers_per_payer"
281+
);
282+
account.payer.signers.truncate(max_signers_per_payer);
283+
}
284+
285+
if account.payer.signers.len() > 1000 {
286+
tracing::warn!(
287+
payer = %account.payer.id,
288+
signers = account.payer.signers.len(),
289+
"Payer has an unusual number of signers which may indicate a signer-flooding attack; consider adding to deny list"
290+
);
291+
}
292+
293+
tracing::info!(
294+
payer = %account.payer.id,
295+
total_signers = account.payer.signers.len(),
296+
"Signer follow-up pagination complete"
297+
);
298+
}
299+
300+
last = response
301+
.payments_escrow_accounts
302+
.last()
303+
.map(|a| a.id.to_string());
304+
all_accounts.extend(response.payments_escrow_accounts);
305+
306+
if (page_len as i64) < page_size {
307+
break;
308+
}
309+
}
310+
311+
tracing::info!(
312+
pages = pages_fetched,
313+
accounts = all_accounts.len(),
314+
"V2 escrow account pagination complete"
315+
);
316+
317+
if pages_fetched > 5 {
318+
tracing::warn!(
319+
pages = pages_fetched,
320+
accounts = all_accounts.len(),
321+
"Unusually high number of V2 escrow accounts; this may indicate a dust-deposit crowding attack"
322+
);
323+
}
324+
325+
let senders_balances: HashMap<Address, U256> = all_accounts
188326
.iter()
189327
.map(|account| {
190328
let balance = U256::checked_sub(
@@ -203,8 +341,7 @@ async fn get_escrow_accounts_v2(
203341
})
204342
.collect::<Result<HashMap<_, _>, anyhow::Error>>()?;
205343

206-
let senders_to_signers = response
207-
.payments_escrow_accounts
344+
let senders_to_signers = all_accounts
208345
.into_iter()
209346
.map(|account| {
210347
let payer = Address::from_str(&account.payer.id)?;
@@ -218,7 +355,13 @@ async fn get_escrow_accounts_v2(
218355
})
219356
.collect::<Result<HashMap<_, _>, anyhow::Error>>()?;
220357

221-
let escrow_accounts = EscrowAccounts::new(senders_balances.clone(), senders_to_signers.clone());
358+
let escrow_accounts = EscrowAccounts::new(senders_balances, senders_to_signers);
359+
360+
tracing::info!(
361+
senders = escrow_accounts.get_senders().len(),
362+
signers = escrow_accounts.signer_count(),
363+
"V2 escrow accounts loaded"
364+
);
222365

223366
Ok(escrow_accounts)
224367
}
@@ -405,6 +548,9 @@ mod tests {
405548
test_assets::INDEXER_ADDRESS,
406549
Duration::from_secs(60),
407550
true,
551+
Address::ZERO, // collector address; mock ignores query variables
552+
"100000000000000000".to_string(),
553+
0,
408554
)
409555
.await
410556
.unwrap();

0 commit comments

Comments
 (0)