Skip to content

Commit ebb59e4

Browse files
Benalex8797Benalex8797
committed
fixed issues
1 parent 3cad0cb commit ebb59e4

34 files changed

Lines changed: 6064 additions & 36 deletions

‎.kiro/specs/deterministic-id-hardening/.config.kiro‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"specId": "b4f1a2e9-7c3d-4e8f-b5a6-1d9c0f2e3b47", "workflowType": "requirements-first", "specType": "bugfix"}

‎.kiro/specs/deterministic-id-hardening/bugfix.md‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
# Bugfix Requirements Document
2+
3+
## Introduction
4+
5+
Several monotonic counters used to generate on-chain IDs lack overflow guards, and the
6+
globally sequential nature of those counters creates a latent cross-tenant collision risk.
7+
The affected identifiers are:
8+
9+
| ID | Contract / layer | Type | Risk |
10+
|---|---|---|---|
11+
| `escrow_id` | `contracts/escrow/src/lib.rs` | `u64` | Wrap at 2⁶⁴ (remote but unguarded) |
12+
| `card_id` | `contracts/virtual-card/src/lib.rs` | `u32` | Wrap at ~4.3 billion (near-term concern) |
13+
| `tx_id` (TxCounter) | `contracts/virtual-card/src/lib.rs` | `u32` | Same as card_id |
14+
| `channel_id` | `contracts/payment-channel/src/lib.rs` | `u64` | Wrap at 2⁶⁴ (remote but unguarded) |
15+
| `SubscriptionCounter` | `contracts/src/subscription_registry.rs` | `u64` | Wrap at 2⁶⁴ |
16+
| `sequenceNumber` (off-chain) | `backend/src/services/payment-channel-service.ts` | JS `number` | Safe to 2⁵³ but unguarded |
17+
18+
Additionally, all counters except `SubscriptionCounter` are **globally sequential across
19+
tenants** — there is no per-user namespace baked into the counter, so access control relies
20+
entirely on address/RLS checks rather than on the ID structure itself.
21+
22+
The `sub_id` in `subscription_renewal` is caller-supplied and not validated for uniqueness
23+
at the contract level, placing the uniqueness burden on callers.
24+
25+
---
26+
27+
## Bug Analysis
28+
29+
### Current Behavior (Defect)
30+
31+
**1.1 — No overflow guard on u32 `CardCounter`**
32+
WHEN the `virtual-card` contract's `CardCounter` reaches `u32::MAX` (4,294,967,295) AND
33+
`issue_card` is called THEN the counter silently wraps to 0 and a new card is assigned
34+
`card_id = 1`, colliding with the first card ever issued and overwriting its storage entry.
35+
36+
**1.2 — No overflow guard on u32 `TxCounter`**
37+
WHEN the `virtual-card` contract's `TxCounter` reaches `u32::MAX` AND `process_payment`
38+
is called THEN the transaction counter wraps silently, and the returned `tx_id` aliases a
39+
previous transaction's identifier, breaking audit integrity.
40+
41+
**1.3 — No overflow guard on u64 `EscrowCount`**
42+
WHEN `EscrowCount` reaches `u64::MAX` AND `create_escrow` is called THEN the counter wraps
43+
to 0, colliding `escrow_id = 1` with the original first escrow and potentially allowing a
44+
new payer to overwrite an existing escrow storage entry.
45+
46+
**1.4 — No overflow guard on u64 `ChannelCount`**
47+
WHEN `ChannelCount` reaches `u64::MAX` AND `open_channel` is called THEN the same silent
48+
wrap occurs as in 1.3, overwriting the existing channel at id = 1.
49+
50+
**1.5 — No overflow guard on u64 `SubscriptionCounter`**
51+
WHEN `SubscriptionCounter` reaches `u64::MAX` AND `create_subscription` is called THEN the
52+
counter wraps, producing a subscription ID whose first 8 bytes are zero — potentially
53+
colliding with the genesis subscription for that user.
54+
55+
**1.6 — Off-chain `sequenceNumber` is an unguarded JS `number`**
56+
WHEN `applyOffChainRenewal` is called on a channel whose `sequenceNumber` has reached
57+
`Number.MAX_SAFE_INTEGER` (2⁵³ − 1) THEN `sequenceNumber + 1` produces an incorrect
58+
result due to IEEE 754 precision loss, silently corrupting channel state.
59+
60+
**1.7 — Globally sequential counters create cross-tenant collision surface**
61+
WHEN all users share a single global counter AND that counter is the sole basis for an ID
62+
THEN any future bug in access-control logic (contract or RLS) maps directly to a collision
63+
between tenants' records. The escrow, card, channel, and tx counters all exhibit this.
64+
65+
**1.8 — `sub_id` uniqueness is caller-enforced, not contract-enforced**
66+
WHEN the `subscription_renewal` contract receives an `init_sub` call with a `sub_id` that
67+
already exists in persistent storage THEN the contract overwrites the existing subscription
68+
data without error, because no uniqueness check is performed on entry.
69+
70+
---
71+
72+
### Expected Behavior (Correct)
73+
74+
**2.1 — `CardCounter` and `TxCounter` saturate at u32::MAX**
75+
WHEN `issue_card` or `process_payment` is called AND the relevant counter equals `u32::MAX`
76+
THEN the contract SHALL panic with a descriptive error (e.g., `CardLimitReached`,
77+
`TxLimitReached`) before writing any new state, so no wrap and no collision occurs.
78+
79+
**2.2 — `EscrowCount` and `ChannelCount` saturate at u64::MAX**
80+
WHEN `create_escrow` or `open_channel` is called AND the relevant counter equals `u64::MAX`
81+
THEN the contract SHALL panic with a descriptive error before assigning a new ID, preventing
82+
silent wraparound.
83+
84+
**2.3 — `SubscriptionCounter` saturates at u64::MAX**
85+
WHEN `create_subscription` is called AND `SubscriptionCounter` equals `u64::MAX` THEN the
86+
contract SHALL panic with `SubscriptionLimitReached` before generating a new ID.
87+
88+
**2.4 — Off-chain `sequenceNumber` guards against unsafe integer range**
89+
WHEN `applyOffChainRenewal` is called AND the current `sequenceNumber >= Number.MAX_SAFE_INTEGER`
90+
THEN the service SHALL throw a typed error (`SequenceOverflowError`) and refuse to apply the
91+
state update, rather than silently producing a corrupted sequence number.
92+
93+
**2.5 — `card_id` u32 → u64 upgrade path is evaluated and documented**
94+
WHEN the `virtual-card` contract is next upgraded THEN the engineering team SHALL have a
95+
documented decision on whether to widen `card_id` from `u32` to `u64` (to align with
96+
`escrow_id` and `channel_id`), including an analysis of on-chain storage impact and any
97+
migration path for existing card records.
98+
99+
**2.6 — `sub_id` uniqueness enforced at the contract level**
100+
WHEN `init_sub` is called with a `sub_id` that already exists in the contract's persistent
101+
storage THEN the contract SHALL panic with `DuplicateSubscriptionId` rather than silently
102+
overwriting the existing subscription record.
103+
104+
**2.7 — Overflow guards are covered by uniqueness tests**
105+
WHEN the test suite runs THEN there SHALL be at least one test per counter that:
106+
(a) sets the counter to its maximum value (e.g., `u32::MAX` or `u64::MAX`),
107+
(b) invokes the creation function, and
108+
(c) asserts that the call panics with the expected overflow error rather than succeeding.
109+
110+
---
111+
112+
### Unchanged Behavior (Regression Prevention)
113+
114+
**3.1 — Normal ID creation is unaffected**
115+
WHEN a counter is below its maximum value THEN `create_escrow`, `issue_card`,
116+
`process_payment`, `open_channel`, and `create_subscription` SHALL CONTINUE TO assign the
117+
next sequential ID and succeed as before.
118+
119+
**3.2 — `submit_state` stale-sequence guard is preserved**
120+
WHEN `submit_state` is called on a payment channel THEN the existing check
121+
`sequence_number <= channel.sequence → Error::StaleState` SHALL CONTINUE TO function
122+
independently of the new `ChannelCount` overflow guard.
123+
124+
**3.3 — Renewal cycle deduplication is preserved**
125+
WHEN the `subscription_renewal` contract's `CycleKey` deduplication detects a duplicate
126+
`cycle_id` THEN that `DuplicateRenewalRejected` path SHALL CONTINUE TO function independently
127+
of the new `sub_id` uniqueness check.
128+
129+
**3.4 — RLS and address-based access control is preserved**
130+
WHEN access-control checks (Soroban `require_auth()`, RLS policies) pass THEN they SHALL
131+
CONTINUE TO be the primary runtime enforcement mechanism. The overflow guards and ID
132+
uniqueness checks are defence-in-depth additions, not replacements.
133+
134+
**3.5 — `SubscriptionCounter` hybrid ID format is preserved**
135+
WHEN `SubscriptionCounter` is below `u64::MAX` THEN the hybrid 8-byte-counter +
136+
24-byte-user-hash ID format SHALL CONTINUE TO be generated identically.
137+
138+
---
139+
140+
## Bug Condition Pseudocode
141+
142+
```pascal
143+
FUNCTION isOverflowCondition(counter, maxVal)
144+
INPUT: counter of numeric type, maxVal of same type
145+
OUTPUT: boolean
146+
RETURN counter >= maxVal
147+
END FUNCTION
148+
```
149+
150+
```pascal
151+
// Property: Fix Checking — any counter at max must be rejected
152+
FOR ALL C WHERE isOverflowCondition(C.value, C.typeMax) DO
153+
result ← contractCreateFn(C)
154+
ASSERT result IS Error WITH kind IN {
155+
CardLimitReached, TxLimitReached,
156+
EscrowLimitReached, ChannelLimitReached,
157+
SubscriptionLimitReached
158+
}
159+
END FOR
160+
```
161+
162+
```pascal
163+
// Property: Preservation — counters below max continue to succeed
164+
FOR ALL C WHERE NOT isOverflowCondition(C.value, C.typeMax) DO
165+
ASSERT contractCreateFn_before(C) = contractCreateFn_after(C) // behavior unchanged
166+
END FOR
167+
```
168+
169+
```pascal
170+
// Property: sub_id uniqueness
171+
FOR ALL S WHERE persistentStorage.contains(sub_id S) DO
172+
result ← init_sub(S)
173+
ASSERT result IS Error WITH kind = DuplicateSubscriptionId
174+
END FOR
175+
```
176+
177+
---
178+
179+
## Acceptance Criteria Summary
180+
181+
| # | Criterion |
182+
|---|---|
183+
| AC-1 | `CardCounter` and `TxCounter` (u32) saturate — calling at `u32::MAX` panics with a named error |
184+
| AC-2 | `EscrowCount` and `ChannelCount` (u64) saturate — calling at `u64::MAX` panics with a named error |
185+
| AC-3 | `SubscriptionCounter` (u64) saturates — calling at `u64::MAX` panics with a named error |
186+
| AC-4 | Off-chain `sequenceNumber` throws `SequenceOverflowError` when `>= Number.MAX_SAFE_INTEGER` |
187+
| AC-5 | `card_id` u32 → u64 upgrade path is documented (decision record, not necessarily implemented) |
188+
| AC-6 | `init_sub` rejects duplicate `sub_id` with `DuplicateSubscriptionId` |
189+
| AC-7 | At least one overflow boundary test exists per counter, asserting the correct panic/error |
190+
| AC-8 | All existing tests continue to pass (no regressions) |

‎.kiro/specs/x402-replay-protection/.config.kiro‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
{"specId": "23f543ef-d521-419b-918f-1969002a1a87", "workflowType": "requirements-first", "specType": "bugfix"}

‎.kiro/specs/x402-replay-protection/bugfix.md‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
# Bugfix Requirements Document
2+
3+
## Introduction
4+
5+
x402-gated agent endpoints accept any structurally valid `PAYMENT-SIGNATURE` header without
6+
checking whether the proof has been used before. An attacker who captures a legitimate 402
7+
receipt (containing a nonce and an expiry timestamp) can replay it an unlimited number of
8+
times against the same endpoint, bypassing the payment requirement entirely. This fix
9+
introduces a server-side nonce store with TTL so that each payment proof can only be
10+
accepted once within its validity window.
11+
12+
## Bug Analysis
13+
14+
### Current Behavior (Defect)
15+
16+
1.1 WHEN a request arrives at an x402-gated endpoint with a `PAYMENT-SIGNATURE` header
17+
whose nonce has already been used within the proof's validity window THEN the system
18+
grants access to the protected resource without rejecting the replayed proof.
19+
20+
1.2 WHEN a captured `PAYMENT-SIGNATURE` (nonce + expiry) is submitted by any client after
21+
the original legitimate use THEN the system processes the request as if it were a fresh,
22+
valid payment proof.
23+
24+
1.3 WHEN no nonce store exists THEN the system has no mechanism to detect or reject
25+
previously seen payment proofs, leaving all x402-gated endpoints vulnerable to replay
26+
attacks.
27+
28+
### Expected Behavior (Correct)
29+
30+
2.1 WHEN a request arrives at an x402-gated endpoint with a `PAYMENT-SIGNATURE` header
31+
whose nonce has already been recorded in the nonce store THEN the system SHALL reject
32+
the request with HTTP 402 and an error indicating the payment proof has already been used.
33+
34+
2.2 WHEN a request arrives at an x402-gated endpoint with a `PAYMENT-SIGNATURE` header
35+
containing a nonce that has not been seen before and has not expired THEN the system
36+
SHALL record the nonce in the nonce store with a TTL equal to the proof's expiry window
37+
and grant access to the protected resource.
38+
39+
2.3 WHEN a request arrives at an x402-gated endpoint with a `PAYMENT-SIGNATURE` header
40+
whose embedded expiry timestamp is in the past THEN the system SHALL reject the request
41+
with HTTP 402 and an error indicating the payment proof has expired, without recording
42+
the nonce.
43+
44+
2.4 WHEN a `PAYMENT-SIGNATURE` header is absent on a request to an x402-gated endpoint
45+
THEN the system SHALL reject the request with HTTP 402 and a `PAYMENT-REQUIRED` header
46+
describing the accepted payment schemes.
47+
48+
2.5 WHEN the nonce store TTL for a recorded nonce expires THEN the system SHALL evict the
49+
nonce entry so that storage does not grow unboundedly.
50+
51+
### Unchanged Behavior (Regression Prevention)
52+
53+
3.1 WHEN a request arrives at a non-x402-gated endpoint THEN the system SHALL CONTINUE TO
54+
process the request using its existing authentication and authorization logic, unaffected
55+
by the nonce store.
56+
57+
3.2 WHEN a valid, first-use `PAYMENT-SIGNATURE` is submitted to an x402-gated endpoint
58+
THEN the system SHALL CONTINUE TO grant access and return the expected resource response
59+
with HTTP 200.
60+
61+
3.3 WHEN an x402-gated endpoint receives a request with an invalid cryptographic signature
62+
in the `PAYMENT-SIGNATURE` header (but a fresh nonce) THEN the system SHALL CONTINUE TO
63+
reject it with HTTP 402 due to signature invalidity, not replay.
64+
65+
3.4 WHEN the existing `IdempotencyService` deduplicates client-initiated API operations THEN
66+
that behavior SHALL CONTINUE TO function independently and is not replaced by the nonce
67+
store introduced by this fix.
68+
69+
3.5 WHEN the `PAYMENT-REQUIRED` header is returned on a 402 response THEN the system SHALL
70+
CONTINUE TO include `maxTimeoutSeconds` in the accepted payment requirements, and the
71+
documented replay-protection window SHALL match that value.
72+
73+
---
74+
75+
## Bug Condition Pseudocode
76+
77+
```pascal
78+
FUNCTION isBugCondition(X)
79+
INPUT: X of type PaymentProof { nonce: string, expiresAt: timestamp }
80+
OUTPUT: boolean
81+
82+
// Returns true when the proof triggers the replay vulnerability
83+
RETURN nonceStore.contains(X.nonce) OR X.expiresAt < now()
84+
END FUNCTION
85+
```
86+
87+
```pascal
88+
// Property: Fix Checking — replayed proof must be rejected
89+
FOR ALL X WHERE isBugCondition(X) DO
90+
result ← x402Middleware'(X)
91+
ASSERT result.status = 402
92+
ASSERT result.error IN { "payment proof already used", "payment proof expired" }
93+
END FOR
94+
```
95+
96+
```pascal
97+
// Property: Preservation Checking — fresh valid proofs must still succeed
98+
FOR ALL X WHERE NOT isBugCondition(X) DO
99+
ASSERT x402Middleware(X) = x402Middleware'(X) // behavior unchanged
100+
END FOR
101+
```

0 commit comments

Comments
 (0)