Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion abis/SuperPaymaster.json

Large diffs are not rendered by default.

23 changes: 16 additions & 7 deletions contracts/src/paymasters/superpaymaster/v3/SuperPaymaster.sol
Original file line number Diff line number Diff line change
Expand Up @@ -1353,13 +1353,22 @@ contract SuperPaymaster is BasePaymasterUpgradeable, ReentrancyGuard, ISuperPaym
// Pending Debt Recovery
// ====================================

/// @notice Retry recording a pending debt that failed during postOp
/// @param token The xPNTs token address
/// @param user The user address
function retryPendingDebt(address token, address user) external onlyOwner nonReentrant {
uint256 amount = pendingDebts[token][user];
if (amount == 0) revert NoPendingDebt();
delete pendingDebts[token][user];
/// @notice Retry recording a pending debt that failed during postOp.
/// @dev H-01: takes an explicit `amount` so a pending balance larger than the
/// token's per-tx limit (`maxSingleTxLimit`) can be drained in chunks —
/// call repeatedly with `amount <= maxSingleTxLimit` until empty. Previously
/// it always retried the full balance, which reverted (and stayed stuck)
/// whenever the accumulated debt exceeded that limit. The remainder stays in
/// `pendingDebts` for the next call. Pass `amount == 0` to attempt the full
/// balance in one shot (works when it is within the limit).
/// @param token The xPNTs token address
/// @param user The user address
/// @param amount aPNTs to record this call; clamped to the pending balance.
function retryPendingDebt(address token, address user, uint256 amount) external onlyOwner nonReentrant {
uint256 pending = pendingDebts[token][user];
if (pending == 0) revert NoPendingDebt();
if (amount == 0 || amount > pending) amount = pending;
pendingDebts[token][user] = pending - amount;
IxPNTsToken(token).recordDebt(user, amount);
emit PendingDebtRetried(token, user, amount);
}
Expand Down
29 changes: 29 additions & 0 deletions contracts/test/v3/SuperPaymaster_BurnRestore.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,35 @@ contract SuperPaymaster_BurnRestore_Test is Test {
assertGt(paymaster.pendingDebts(address(xpnts), user1), 0, "pendingDebts must be non-zero");
}

// ── H-01: chunked retryPendingDebt drains a balance over multiple calls ──────
function test_RetryPendingDebt_Chunked() public {
// 1. Accumulate a pending debt (both burn + recordDebt fail in postOp).
xpnts.setRecordDebtFail(true);
bytes memory ctx = _runValidate();
vm.prank(address(entryPoint));
paymaster.postOp(IPaymaster.PostOpMode.opSucceeded, ctx, MAX_COST, 0);
uint256 pending = paymaster.pendingDebts(address(xpnts), user1);
assertGt(pending, 1, "setup: pending must be > 1");

// 2. recordDebt works again; drain in a chunk smaller than the balance.
xpnts.setRecordDebtFail(false);
address owner = paymaster.owner();
uint256 chunk = pending / 2;
vm.prank(owner);
paymaster.retryPendingDebt(address(xpnts), user1, chunk);
assertEq(paymaster.pendingDebts(address(xpnts), user1), pending - chunk, "chunk 1 leaves remainder");

// 3. amount == 0 drains the full remainder.
vm.prank(owner);
paymaster.retryPendingDebt(address(xpnts), user1, 0);
assertEq(paymaster.pendingDebts(address(xpnts), user1), 0, "fully drained");

// 4. retrying an empty balance reverts.
vm.prank(owner);
vm.expectRevert(SuperPaymaster.NoPendingDebt.selector);
paymaster.retryPendingDebt(address(xpnts), user1, 0);
}

// ── Test 4: Two consecutive ops → different burns (not replay) ────────────

function test_PostOp_TwoOps_NoDuplicateReplay() public {
Expand Down
43 changes: 43 additions & 0 deletions docs/operations/pending-debt-runbook.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Operations runbook — pending debt retry (H-01)

## What `pendingDebts` is

`SuperPaymaster.pendingDebts[token][user]` is a last-resort accumulator. During
`postOp`, `_recordDebt` tries, in order: `burnFromWithOpHash` (pull from the user's
xPNTs balance) → `recordDebtWithOpHash` (book the debt) → and only if BOTH revert does
the amount land in `pendingDebts`. In normal operation this is always 0. It is non-zero
only in exceptional cases (a single op whose aPNTs cost exceeds the token's
`maxSingleTxLimit`, an emergency-disabled token, or a temporarily unreachable token).

## The H-01 issue (fixed)

`recordDebt` reverts when `amount > maxSingleTxLimit` (5000 aPNTs). The old
`retryPendingDebt(token, user)` always retried the **full** pending balance, so once the
accumulator exceeded that limit the retry reverted forever — the debt could only be
written off via `clearPendingDebt` (the protocol eats it).

## The fix — chunked retry

```solidity
function retryPendingDebt(address token, address user, uint256 amount) external onlyOwner
```

`amount` is recorded this call and clamped to the pending balance; the remainder stays
in `pendingDebts`. So a balance larger than `maxSingleTxLimit` is drained over multiple
calls instead of writing it off.

## Management-backend / operational rule

To drain a stuck pending debt:

1. Read `pendingDebts(token, user)` and the token's `maxSingleTxLimit()` (5000 aPNTs default).
2. If `pending <= maxSingleTxLimit`: call `retryPendingDebt(token, user, 0)` once (0 = full balance).
3. If `pending > maxSingleTxLimit`: call `retryPendingDebt(token, user, maxSingleTxLimit)`
**repeatedly** until `pendingDebts(token, user) == 0`. Each call books one chunk as
real user debt.
4. Only use `clearPendingDebt(token, user)` (write-off, protocol absorbs the loss) when
the debt is genuinely unrecoverable (token permanently unreachable / user gone).

**Backend implementation note:** the retry loop must be idempotent and re-read
`pendingDebts` after each tx (the value shrinks per call). Prefer recovering the debt
(step 3) over writing it off (step 4); reserve write-off for unrecoverable cases and log it.
49 changes: 49 additions & 0 deletions docs/security/2026-05-31-pk-audit-findings.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,52 @@ Each fix ships with a regression test that **fails on current code and passes af
- `PoC_C02_UnsignedDrain.t.sol` ✅ faithful (real xPNTs auto-allowance)
- `PoC_C03_RecipientRedirect.t.sol` ✅ faithful (real EIP-3009 sig)
- `PoC_C04_ForcedPostOpOOG.t.sol` ✅ faithful (real EntryPoint handleOps; replaces the removed dead-code PoC)

---

## Final resolution (2026-06-01) — fixes landed + per-fix Codex challenge review

| Finding | Status | Commit / note |
|---|---|---|
| **C-04** | ✅ FIXED (3 Codex rounds clean) | `MIN_POST_OP_GAS = 200_000` floor in validate + dryRun; dead `postOpReverted` branch removed. 200k measured (postOp ~141k), not Codex's 250k over-estimate. |
| **C-01** | ✅ FIXED | `b9c13af7`. Balance-aware `_creditExceeded`: pay-from-balance → allowed; else `getDebt + pendingDebts + charge <= getCreditLimit`. **TOCTOU (balance moved between validate and postOp) ACCEPTED by jhf** as a bounded one-op overrun — recovery via mint auto-repayment (`xPNTsToken._update`, mint-only = B1) + manual `repayDebt`. exchangeRate read from `xPNTsToken.exchangeRate()` (consistent with AirAccount #10). |
| **C-02** | ✅ FIXED — Codex approved mainnet | `d7df0c3e`. `settleX402PaymentDirect` requires payer EIP-712 `X402PaymentAuthorization(from,to,asset,amount,maxFee,validBefore,nonce)`, domain `verifyingContract = SP proxy`, `SignatureCheckerLib` (EOA + ERC-1271). |
| **C-03** | ✅ FIXED — Codex approved mainnet | `d7df0c3e`. `settleX402Payment` binds recipient via `nonce = keccak256(to, salt)` — reuses the payer's EIP-3009 signature, no second sig. |
| **L-01** | 📝 ACCEPTED as a feature (not fixed) | Codex flagged: X402 auth not bound to `msg.sender` → another approved facilitator can front-run the fee (payer funds 100% safe; only fee attribution). **jhf decision: keep it — decentralized facilitator competition is the intended model; you sign once, whoever serves first earns.** `facilitator` deliberately NOT bound into the signature (preserves flexible routing). |
| **I-01** | ↪ off-chain WYSIWYS, see below | On-chain nonce/recipient binding is tamper-evident; the residual "malicious UI" risk is solved off-chain (passkey-bound signing + clear-signing display). Converges with AirAccount KMS #16. |

### Verification
- 969 forge tests pass / 0 failed. SuperPaymaster 24,093 bytes (EIP-170 OK, 483 spare).
- PoC_C02/PoC_C03 rewritten as fix regressions (no-sig drain reverts; redirect reverts; valid-sig happy paths pass).
- AirAccount KMS EIP-712 digest is standard `keccak256(0x1901 || domainSeparator || hashStruct)` — **compatible with SP `_verifyX402Auth`**.

### Cross-repo dependencies
- **aastar-sdk #39** (filed): client must sign `X402PaymentAuthorization` (direct) + derive EIP-3009 `nonce = keccak256(to, salt)` (USDC). Canonical x402 integration lives in the SDK; in-repo `packages/x402-facilitator-node` is deprecated.
- **AirAccount KMS #16** (their repo): host-compromise → arbitrary `sign_typed_data` would let a forged X402 authorization pass SP's on-chain check for AirAccount (ERC-1271) users. SP's C-02 gate is necessary but end-to-end security for AirAccount accounts ALSO needs KMS passkey-bound JWT issuance (their fix A). **On-chain fix + KMS fix together = complete.**

---

## KMS integration consistency — CONFIRMED OK (2026-06-01, AirAccount #16 / PR #20)

AirAccount closed KMS Issue #16 (PR #20, 5 rounds): removed the JWT signing oracle
(JwtHmacSign/JwtSignPayload as external TA commands), folded JWT issuance into
`create_agent_key` (WebAuthn-gated), added TA passkey auth to `sign_typed_data`, and
made the TA own the JWT `iat`. **Impact on SuperPaymaster x402: none on the interface,
positive on end-to-end security.** The dual-payer x402 design maps cleanly onto two
SEPARATE TA command paths — confirmed by AirAccount:

| x402 payer | TA command | Auth | Passkey per payment? |
|---|---|---|---|
| Human user (manual EIP-712) | `sign_typed_data` (cmd 17) | passkey, user present | yes (by design) |
| Human grants a session key | `sign_grant_session` / `sign_p256_grant_session` (PR #19) | passkey once | once, at delegation |
| **Autonomous agent (x402 micropayment)** | `sign_agent_user_op` (cmd 12) | **JWT, no passkey** | **no** |

Agent x402 micropayments go through `sign_agent_user_op` (JWT auth) — #16's passkey
gating on `sign_typed_data` does NOT touch this path. Lifecycle: `create_agent_key`
(passkey once) → `grant_session` (passkey once) → every x402 via `sign_agent_user_op`
(JWT, 0 passkey). So C-02's `_verifyX402Auth` (ERC-1271) accepts both: human-signed
(passkey path) and agent-signed (session-key path) authorizations, with no contract change.

- Signature format unchanged (standard EIP-712 digest); our on-chain verification is auth-method-agnostic.
- #16 resolves the C-02 ↔ KMS dependency flagged in I-01 (host compromise can no longer forge a human's X402 authorization).
- **Open coordination item (low):** confirm `sign_grant_session` / `sign_p256_grant_session` (PR #19) are themselves passkey-gated — that is the human-delegation moment and should require user presence. If not, flag to AirAccount.
17 changes: 11 additions & 6 deletions script/gasless-tests/test-group-B5-dry-run-pending-debt.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const SP_DRY_ABI = [
// Pending debt tracking
"function pendingDebts(address token, address user) view returns (uint256)",
// Owner-only recovery functions
"function retryPendingDebt(address token, address user) external",
"function retryPendingDebt(address token, address user, uint256 amount) external",
"function clearPendingDebt(address token, address user) external",
];

Expand Down Expand Up @@ -172,18 +172,23 @@ async function main() {
printSkip('No pending debts — retryPendingDebt not applicable');
} else {
try {
const r = await sendTxSafe(sp, 'retryPendingDebt', [config.aPNTs, deployerAddr], 'retryPendingDebt');
// H-01: 3rd arg is the chunk to record this call; 0 = full pending balance
// (clamped to the balance). Use repeated calls with amount <= maxSingleTxLimit
// to drain a balance larger than the per-tx limit.
const r = await sendTxSafe(sp, 'retryPendingDebt', [config.aPNTs, deployerAddr, 0n], 'retryPendingDebt');
if (r) {
const debtAfter = await sp.pendingDebts(config.aPNTs, deployerAddr);
printKeyValue('pendingDebt after retry', ethers.formatEther(debtAfter));
if (debtAfter < pendingDebt) {
printSuccess(`pendingDebt decreased after retryPendingDebt (${ethers.formatEther(pendingDebt)} → ${ethers.formatEther(debtAfter)})`);
pendingDebt = debtAfter;
} else {
// retryPendingDebt deletes the mapping then calls xPNTs.recordDebt.
// If recordDebt succeeds, pendingDebts[token][user] is now 0.
// If recordDebt reverts, retryPendingDebt itself reverts (no partial state).
printInfo(`pendingDebt unchanged after retry — xPNTs.recordDebt may have also reverted`);
// retryPendingDebt records `amount` (here the full balance) via xPNTs.recordDebt,
// leaving the remainder in pendingDebts. With amount=0 and recordDebt succeeding,
// pendingDebts[token][user] is now 0. If recordDebt reverts, the call reverts
// (no partial state) — e.g. the balance exceeds maxSingleTxLimit, so pass a
// chunk <= the limit instead.
printInfo(`pendingDebt unchanged after retry — xPNTs.recordDebt may have reverted (try a chunk <= maxSingleTxLimit)`);
}
}
} catch (e) {
Expand Down
Loading