Skip to content

docs(sp): _recordDebt described a balance short-circuit that was deleted - #431

Merged
jhfnetboy merged 1 commit into
mainfrom
fix/stale-h1-comment
Sep 8, 2026
Merged

jhfnetboy merged 1 commit into
mainfrom
fix/stale-h1-comment

Conversation

@jhfnetboy

Copy link
Copy Markdown
Member

Comment-only. No bytecode change — checked, not asserted (below).

The comment described code that no longer exists

Two blocks in _recordDebt explain the ceiling re-check and the else branch in terms of "_creditExceeded's validation-time balance short-circuit", defeatable by a mid-UserOp drain.

That short-circuit was:

if (IERC20(token).balanceOf(user) >= xPNTsCharge) return false;

and c6493ade (audit H-1, Plan A) deleted it — read out of that commit's diff, not inferred. _creditExceeded is now three lines and never reads a balance.

Why this one is worth a PR rather than a note

The mode it describes — a zero-credit user paying from balance — is exactly what a "balance or refusal, no debt" community switch would need. Anyone arriving to evaluate that (as happened while writing docs/design/credit-switch/) reads this comment first and concludes the behaviour already exists.

It does not. With no balance short-circuit, a user whose recorded debt already fills the ceiling is refused at validation however much xPNTs they hold.

What replaces it

The half that is still true is kept: recordDebtWithOpHash checks only maxSingleTxLimit, not getCreditLimit, so calling it unconditionally would let debt pass the ceiling — the C-01 scenario.

The dead cause is replaced by what can actually reach the else: validation asserted getDebt + pendingDebts + charge <= getCreditLimit, this re-evaluates the same inequality later, and all three inputs can move — getDebt and pendingDebts can grow if another op for the same user settles first, and getCreditLimit can fall, being creditTierConfig[_levelForReputation(globalReputation[user])] with both writable.

Written as reachability, not diagnosis: no live instance of any of the three was constructed, and the new comment says so instead of naming a mechanism with the confidence the old text had. Trading one confident wrong cause for another confident unverified one would repeat the defect.

Comment-only is measured

Same worktree, same profile, built before and after with only this file differing:

main version     -> 23,569 B deployed bytecode
with the comment -> 23,569 B, byte-identical

And the comparison is not vacuous — it separates SuperPaymaster from Registry. A first attempt at this check read a missing artifact and printed "DIFFERENT"; the reading was the measurement failing, not the answer.

https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2

Two comment blocks in _recordDebt explained the ceiling re-check and the
`else` branch in terms of "_creditExceeded's validation-time balance
short-circuit", defeatable by a mid-UserOp drain. That short-circuit was
`if (IERC20(token).balanceOf(user) >= xPNTsCharge) return false;` and
c6493ad (audit H-1, Plan A) deleted it — confirmed from the diff of
that commit, not inferred. _creditExceeded is now three lines and never
reads a balance.

It matters more than a stale note usually would: the mode it describes
— a zero-credit user paying from balance — is exactly what a
"balance or refusal, no debt" community switch would need, so anyone
arriving to evaluate that reads this comment first and concludes the
behaviour already exists. It does not: with no balance short-circuit, a
user whose recorded debt already fills the ceiling is refused at
validation however much xPNTs they hold.

The rewrite keeps what is still true — recordDebtWithOpHash checks only
maxSingleTxLimit, not getCreditLimit, so an unconditional call would let
debt pass the ceiling — and replaces the dead cause with what can
actually reach the `else`: validation asserted
getDebt + pendingDebts + charge <= getCreditLimit, this re-evaluates the
same inequality later, and all three inputs can move. getDebt and
pendingDebts can grow if another op for the same user settles first, and
getCreditLimit can FALL, being
creditTierConfig[_levelForReputation(globalReputation[user])] with both
writable.

Written as reachability, not diagnosis: no live instance of any of the
three was constructed, and the comment says so rather than naming a
mechanism with the confidence the old text had.

Comment-only, and that is checked rather than asserted: same worktree,
same profile, built before and after with only this file differing —
deployed bytecode byte-identical at 23,569 B. The comparison is not
vacuous: it separates SuperPaymaster from Registry.

Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2

@clestons clestons left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — #431 @ 29e80c1059b6ac378fef363d7728b8fdbd5d0418

意图:_recordDebt 的注释在描述一段已经被删掉的代码(_creditExceeded 里的余额短路),把它改成描述现在真实存在的东西。

一个只改注释的 PR,它的全部价值就是那些句子的真假。所以本轮审的不是「改得好不好读」,而是逐条核那几个可验证的声称。


声称逐条核(四条,全部属实)

# 注释里的声称 实测
① 短路曾是 if (IERC20(token).balanceOf(user) >= xPNTsCharge) return false;,被 c6493ade 删除 c6493ade = fix(audit-H1): enforce credit ceiling in validation (Plan A),其 diff 里有 - if (IERC20(token).balanceOf(user) >= xPNTsCharge) return false; ✅
② _creditExceeded 现在很短、从不读余额 SuperPaymaster.sol:832-835,函数体两条语句,无任何 balanceOf ✅
③ getCreditLimit = creditTierConfig[_levelForReputation(globalReputation[user])],二者皆可写 Registry.sol:891 逐字如此;globalReputation 写于 batchUpdateGlobalReputation(:587),creditTierConfig 写于 setCreditTier(...) external onlyOwner(:676→:691) ✅
④ recordDebtWithOpHash 只查 maxSingleTxLimit,不查 getCreditLimit xPNTsToken.sol:558-571:查 emergencyDisabled / SP 地址 / msg.sender / maxSingleTxLimit / 两个 hash map —— 没有 getCreditLimit ✅

③ 我第一遍差点判错:grep 加了 | head -5,只看见 145-149 那几行初始化,没看见 691 行的 setCreditTier。一个要用来支撑「可写/不可写」结论的枚举,不能截断 —— 去掉 head 重查才拿到完整清单。

那段「还有什么能走到 else」的推理

新注释把可达性拆成三个可动的输入(getDebt / pendingDebts 增长,getCreditLimit 下降),并且明写「Stated as reachability, not as a diagnosis: no live instance of any of the three has been constructed here.」

这个措辞是对的,我特意核了它站得住:三个输入的可写性我逐个找到了写入点(见 ③),所以「机制存在」不是空话;而它没有声称构造出了实例,也确实没有。一个未构造的可达性,标注成未构造,就是准确的;反过来把它写成「可以被攻击」才是我要拦的那种。

真的只是注释吗 —— 机械判,不靠肉眼

改动行总数 39,其中非注释行 0
  负对照:往清单里塞一条 `uint256 x = 1;` → 判据认出 1 条非注释行(所以它不是恒真)
指令型注释(solhint/slither/forge-lint/prettier-ignore/@custom:)在 diff 里 0 命中
  正对照:同一判据在 contracts/src/ 下能命中 GTokenStaking.sol 与 X402Facilitator.sol
  (所以「0 命中」是真的没有,不是判据没工作)

编译产物:字节码逐字节相同,而且我证明了这个比较不是恒真

foundry.toml:20 有 bytecode_hash = "none"(注释写明就是为了让 codehash 只反映真实改动),所以注释变更本应产出完全相同的 bytecode。

base 4caa0cdc  vs  head 29e80c10   deployedBytecode 长度 47140 / 47140   相同? True

控制实验(否则「相同」什么也不证明):
  把 _creditExceeded 里 `used = getDebt + pendingDebts` 改成 `+ 1`,重编译
    → 47140 / 44770   相同? False   ← 这个比较确实能分辨
  还原重编译 → 相同? True,worktree diff 为空

CI:全绿,含 Stage 1 — solhint + build、Stage 2 — forge test + fuzz、test、Stage 3 — Slither。


结论:APPROVE

APPROVE 即表示本仓库可以直接合并这个 PR。(我自己不执行合并。)

四条事实声称逐条对上源码与 git 历史;「可达性未构造」这个限定词准确且必要;改动行全是注释(带负对照)、无指令型注释(带正对照);base 与 head 的 deployedBytecode 逐字节相同,且控制实验证明该比较能分辨真实代码改动。R1(DeepSeek) 全量与安全两遍均无 findings,我独立核完同意。

⚠️ 有效期:锚在 29e80c1059b6ac378fef363d7728b8fdbd5d0418。


🔎 自评

  • 轮数:2 轮实跑(R1a + R1b DeepSeek → 我的机械验证)。 走 2-round 的依据是 🔧 机械判据实跑:这个 .sol 文件的消费方是编译器,base 与 head 的 deployedBytecode 逐字节相同(并带控制实验),且 diff 无任何指令型注释。R2/R3/R4 未跑。不虚标。
  • R1 跑了。
  • 机械证据:git show c6493ade 核删除行;读 _creditExceeded / getCreditLimit / recordDebtWithOpHash 源码;无截断地枚举 creditTierConfig / globalReputation 的全部写入点并定位所在函数与权限修饰;diff 改动行的注释性机械判定(带负对照);指令型注释扫描(带正对照);双 worktree forge build + bytecode 比对 + 变异控制实验 + 还原复验。
  • DeepSeek flash 评级:3/5 —— 两遍空答,结论方向对(确实无 findings),但它给出的理由是「comment-only,无功能影响」,而这个 PR 的全部风险恰恰在注释内容的真假上,它一条声称都没去核。空答碰巧正确 ≠ 审对了地方。
    改进建议:对 comment/docs 类 diff,prompt 里要求它把注释中的每一条可验证声称抽成列表(哪个符号、哪个 commit、哪个函数),哪怕不去验,抽出来也能让我少漏。
  • 我驳回了哪些 finding:无可驳回 — R1a/R1b 均为空,我独立核实后同意。
  • 我自己的量具错过一次(写进结论前修了):枚举 creditTierConfig 写入点时用了 | head -5,只看到初始化、没看到 setCreditTier,差点把「governance 可改」判成假。支撑「有没有」结论的枚举不许截断 —— 与既有那条「用计数推翻别人前先审量具」同源。
  • 🧪 试用项:
    · T1 意图:命中 —— 「只改注释的 PR,价值全在句子真假」这句话直接把本轮定成逐条核声称,而不是读一遍觉得通顺就过。
    · T2 R3 产物核验:N/A —— R3 未跑。
    · T3 驳回清单:空转 —— 本轮无 finding 可驳。如实记。

Reviewed by PR-Daemon · head 29e80c1059b6ac378fef363d7728b8fdbd5d0418

@jhfnetboy
jhfnetboy merged commit 3b0d482 into main Sep 8, 2026
14 checks passed
@jhfnetboy
jhfnetboy deleted the fix/stale-h1-comment branch September 8, 2026 05:57
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 8, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants