Skip to content

fix(p0-12b): per-xPNTs community-controlled facilitator whitelist#110

Open
jhfnetboy wants to merge 5 commits intofix/p0-12a-x402-asset-whitelistfrom
fix/p0-12b-facilitator-whitelist
Open

fix(p0-12b): per-xPNTs community-controlled facilitator whitelist#110
jhfnetboy wants to merge 5 commits intofix/p0-12a-x402-asset-whitelistfrom
fix/p0-12b-facilitator-whitelist

Conversation

@jhfnetboy
Copy link
Copy Markdown
Member

⚠️ Stacked PR

Base 是 `fix/p0-12a-x402-asset-whitelist`(PR #N+5)。完整 stack: main → P0-13 → P0-12a → P0-12b。GitHub 只展示这条 commit 的 delta。

P0-12b (D4 新设计)

D4 决策:每个 xPNTs 由社区 multisig 指定 trusted facilitators。当前 `autoApprovedSpenders` 是单一全局映射 —— 跨社区 blast radius 太大。社区 A 不应被强迫 trust 社区 B 的 facilitator。

Defense

  • `xPNTsToken`: `approvedFacilitators` mapping + `addApprovedFacilitator` / `removeApprovedFacilitator`(onlyCommunityOwner)+ event
  • `xPNTsFactory.deployxPNTsToken` 接受可选 `address[] initialFacilitators` 参数(保持 ABI 兼容:原方法仍存在)
  • `SuperPaymaster.settleX402PaymentDirect` 加 `require(IXPNTsToken(asset).approvedFacilitators(msg.sender), "facilitator not approved")`

`autoApprovedSpenders` 与 `approvedFacilitators` 是不同概念,两者并存 —— 前者是 ERC20 transfer firewall,后者是 settle 调用权限白名单。

⚠️ 部署 runbook

D4 用户决策:社区部署默认自动 add AAStar facilitator —— community 自己显式 add。AAStar 提供一个可选默认。

Tests

新测试(spender 不在白名单 revert / 在白名单接受 / add/remove only community / settle 强制 `isXPNTs && approvedFacilitator`)+ 现有不破坏

Spec

`docs/security/2026-04-26-p0-prelaunch.md` §3 P0-12b
`docs/security/2026-04-26-decision-records.md` D4
`docs/deployment/facilitator-deployment.md`(PR #98)—— 部署后社区 add facilitator 的 runbook

P0-12b (D4): even after P0-12a restricts `settleX402PaymentDirect` to
xPNTs assets, a single global facilitator (e.g. AAStar's default) is
still implicitly trusted by every community's xPNTs. A compromise of
that one key would blast across all communities at once. Per D4, each
xPNTs token now carries its own community-controlled
`approvedFacilitators` whitelist.

Defense:
- xPNTsToken.sol:
  - new `mapping(address => bool) public approvedFacilitators` storage
    slot. Distinct from `autoApprovedSpenders`: the latter is the
    ERC20 transferFrom firewall ("no approve needed"), the former
    gates `settleX402PaymentDirect` invocation. The two are
    intentionally orthogonal — both must hold for a Direct settle to
    succeed.
  - `addApprovedFacilitator(address)` / `removeApprovedFacilitator
    (address)` — `onlyCommunityOwner` (matches the existing
    `removeAutoApprovedSpender` access pattern; community owner is
    the policy authority).
  - new events `FacilitatorApproved` / `FacilitatorRemoved`.
  - per D4, factory does NOT auto-add AAStar's facilitator at
    deploy. Communities must explicitly add facilitators after
    deployment (runbook concern, not contract default). Existing
    `deployxPNTsToken` ABI is unchanged — no breaking SDK impact.
- IxPNTsToken adds `approvedFacilitators(address) returns (bool)`.
- SuperPaymaster.settleX402PaymentDirect: after the P0-12a asset
  gate passes, also enforce
  `IxPNTsToken(asset).approvedFacilitators(msg.sender)`. Reverts
  `Unauthorized` (reuses existing error). Without this, a compromised
  facilitator that still holds ROLE_PAYMASTER_SUPER could touch
  every community's xPNTs even after that community has tried to
  blacklist them upstream.

Trust-model summary (per threat-model.md §3.1): facilitator is
BOUNDED TRUST at the SP layer; this whitelist binds the bound to
the per-community policy surface so a facilitator compromise is
contained to communities that have explicitly opted in.

Storage layout: xPNTsToken adds one mapping (clones share storage
layout; `approvedFacilitators` appends after `autoApprovedSpenders`,
which is safe for clones since storage is fresh at clone time).
SuperPaymaster has no new storage. UUPS upgrade safe.

Tests:
- contracts/test/v3/X402Direct_FacilitatorWhitelist.t.sol (new, 9
  tests):
  - test_SettleDirect_RevertsForUnapprovedFacilitator
  - test_SettleDirect_AllowsApprovedFacilitator
  - test_SettleDirect_PerCommunityIsolation (A's approval doesn't
    leak to B's xPNTs — the core blast-radius property)
  - test_AddApprovedFacilitator_OnlyCommunity (SP owner cannot, only
    xPNTs communityOwner can)
  - test_AddApprovedFacilitator_RevertsZeroAddress
  - test_AddApprovedFacilitator_EmitsEvent
  - test_RemoveApprovedFacilitator_RevokesAccess (instant revoke;
    next settle reverts even with same allowance state)
  - test_RemoveApprovedFacilitator_OnlyCommunity
  - test_DefaultEmptyApprovedFacilitators (no auto-add at deploy)
- contracts/test/v3/X402Direct_AssetWhitelist.t.sol: helper updated
  to also `addApprovedFacilitator(operator)` so its happy path still
  exercises Direct end-to-end. All 6 P0-12a tests still green.
- contracts/test/v3/SuperPaymasterV5Features.t.sol: MockDirectToken
  now exposes `approvedFacilitators` + `setApprovedFacilitator`;
  setUp wires it for operator1. All 36 V5 features tests still green.

Refs: P0-12b in docs/security/2026-04-26-p0-prelaunch.md, D4 in
      docs/security/2026-04-26-decision-records.md
@jhfnetboy jhfnetboy requested a review from fanhousanbu as a code owner April 28, 2026 07:45
@fanhousanbu
Copy link
Copy Markdown
Contributor

代码审查

警告 — 三层防御架构方向正确,但存在 CRITICAL 运营风险和 HIGH 代码问题

CRITICAL(运营)— communityOwner 是单点 EOA,无合约层多签强制
addApprovedFacilitatorremoveApprovedFacilitator 仅需 communityOwner 单个地址授权。communityOwner 私钥泄露后,攻击者可立即添加任意地址为 facilitator,配合 autoApprovedSpenders 可直接提取该社区所有用户余额。

PR 描述提到"community multisig only",但合约层面没有验证 communityOwner 是否为多签合约。建议至少:

  1. 在合约 NatSpec 中明确要求 communityOwner 必须是合约地址(如 Gnosis Safe)
  2. 或在 setCommunityOwner/构造函数中加 require(communityOwner.code.length > 0, "must be contract")
  3. 或对 addApprovedFacilitator 增加时间锁(24-48 小时)给社区成员审查窗口

HIGH — communityOwner 可以将自身加入 approvedFacilitators
没有任何检查阻止 communityOwner 把自己的地址添加为 facilitator,且 communityOwner 通常也有 mint 权限,可构成利益冲突。建议在 addApprovedFacilitator 中加:

require(facilitator != communityOwner, "communityOwner cannot be facilitator");

HIGH — approvedFacilitatorsautoApprovedSpenders 必须同时配置,但无提示
facilitator 需同时在两个 mapping 中才能完成 Direct settle,若只在 approvedFacilitators 中而不在 autoApprovedSpenders 中,safeTransferFrom 会 revert,但 nonce 已被消耗。建议在 addApprovedFacilitator 中同时写入 autoApprovedSpenders,或在文档和错误消息中明确说明两者需要同时配置。

MEDIUM — 缺少 communityOwner 自我授权的测试
建议补充此场景的测试(无论是验证被拒绝还是验证当前行为)。

…st functions

Document that communityOwner MUST be a multisig wallet. A compromised
single-EOA owner can add arbitrary facilitators enabling unauthorized token
extraction. Add the same warning to removeApprovedFacilitator and a brief
note to transferCommunityOwnership referencing the security rationale.
@jhfnetboy
Copy link
Copy Markdown
Member Author

已在 addApprovedFacilitatorremoveApprovedFacilitatortransferCommunityOwnership 三个函数上补充安全文档说明。

核心警告:communityOwner 必须是多签钱包(如 Gnosis Safe)。若使用单一 EOA 作为 communityOwner 并被攻击者控制,攻击者可添加任意 facilitator 地址,从而实现未授权的代币提取。合约层面无法强制检查多签要求,必须在部署流程中保证 communityOwner != EOA

communityOwner calling addApprovedFacilitator(communityOwner) would let
them act as both administrator and settlement facilitator — a conflict of
interest that bypasses separation-of-duties.  Add a check that reverts
with Unauthorized(facilitator) when facilitator == communityOwner, and
cover it with test_AddApprovedFacilitator_RevertsIfCommunityOwner.
@jhfnetboy
Copy link
Copy Markdown
Member Author

HIGH 修复:阻止 communityOwner 自我添加为 facilitator

问题(PR 审核意见)addApprovedFacilitator 缺少自我添加检查,communityOwner 可以将自身加入 facilitator 白名单,形成「管理员 + 结算者」双重角色的利益冲突——可借助已持有的 auto-approved 无限额度从任意用户账户提款。

修复内容(commit 3271a15):

  1. 合约 contracts/src/tokens/xPNTsToken.soladdApprovedFacilitator 新增检查:

    // Prevents communityOwner from acting as both administrator and facilitator
    // (conflict of interest)
    if (facilitator == communityOwner) {
        revert Unauthorized(facilitator);
    }

    同时补充 NatSpec,说明「所有者-结算者」角色分离的安全含义。

  2. 测试 contracts/test/v3/X402Direct_FacilitatorWhitelist.t.sol — 新增 test_AddApprovedFacilitator_RevertsIfCommunityOwner:验证调用 addApprovedFacilitator(communityOwner) 会以 Unauthorized(communityOwner) 回滚,并确认白名单中无该条目。

测试结果:10/10 PASS(含新增用例)。

未做「auto-sync with autoApprovedSpenders」变更,该项需另行决策。

@jhfnetboy
Copy link
Copy Markdown
Member Author

Option A 实现:addApprovedFacilitator 自动同步 autoApprovedSpenders

根据架构决策,已实现 auto-sync 方案(commit 最新推送):

合约变更 xPNTsToken.sol

  • addApprovedFacilitator(facilitator):同时写入 autoApprovedSpenders[facilitator] = true
  • removeApprovedFacilitator(facilitator):同时清除 autoApprovedSpenders[facilitator] = false
  • 两个函数均补充 NatSpec,说明 auto-sync 行为及其原因

测试变更 X402Direct_FacilitatorWhitelist.t.sol

  • 移除冗余的手动 addAutoApprovedSpender 调用(现由 addApprovedFacilitator 自动完成)
  • 新增 test_AddApprovedFacilitator_AutoSyncsSpender:验证单次调用同时写入两个 mapping
  • 新增 test_RemoveApprovedFacilitator_AutoClearsSpender:验证移除时两个 mapping 同步清除

测试结果:12/12 PASS

@jhfnetboy
Copy link
Copy Markdown
Member Author

撤销 auto-sync,恢复两个 mapping 独立设计

架构纠正: 内部的 调用方是 SuperPaymaster 合约本身(msg.sender = SP),而非 facilitator。工厂部署时已执行 ,因此 facilitator 完全不需要进入 。两个 mapping 的职责是正交的:

  • approvedFacilitators:控制谁能调用 settleX402PaymentDirect(调用权)
  • autoApprovedSpenders:控制谁能无 allowance 直接 transferFrom(资金移动权)

合约变更: / 移除 auto-sync 逻辑,只写 ,NatSpec 更新说明正交关系。

测试变更:移除 auto-sync 断言测试,新增 test_AddApprovedFacilitator_DoesNotGrantAutoSpender 明确验证两个 mapping 独立(facilitator 加入后 autoApprovedSpenders 仍为 false)。

测试结果:11/11 PASS

Copy link
Copy Markdown
Contributor

@fanhousanbu fanhousanbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM(CEI 模式): settleX402PaymentDirect() 里,_validateX402AndComputeFee() 内部先写 nonce(x402SettlementNonces[key] = true),之后才做 isXPNTsapprovedFacilitators 两个检查。行为上安全(revert 会回滚 nonce 写入),但违反 CEI——未来维护者若在两个检查之间插入 external call 会引入 re-entrancy 风险。请在该段加注释说明 nonce 写入依赖 revert atomicity 而非代码顺序。不阻断。

✅ Approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants