feat: add EVM account blacklist and bump chain33 to aa71469 - #1304
Conversation
Wire ForkAccountBlacklist into EVM CheckTx/exec/runtime/statedb so blocked addresses are rejected consistently with chain33 consensus checks, and depend on github.com/33cn/chain33@aa71469c0974 from master.
Resolve Create() conflict by keeping account blacklist check and ForkEVMFixOverflow transfer protection together.
Keep dependency files unchanged per review feedback; blacklist feature does not require bumping the chain33 module version in this PR.
|
需要单独提pr处理 chain33 ethereum包升级兼容问题 @vipwzw @libangzhu |
0550d84 to
80d3dfa
Compare
…st is merged
The attack integration test had a TODO placeholder skipping blacklist
verification ('blacklist pending — roles attacker+accomplice marked')
waiting for the account blacklist PR to land. Now that ForkAccountBlacklist
is in master:
- Set ForkAccountBlacklist fork height to 1000 in test config (aligned
with ForkEVMFixOverflow).
- Replace t.Skip with real assertions: blacklisted attacker's EVM call
is rejected via checkEvmBlockedAccount, blacklisted accomplice's coins
tx is rejected via chain33 CheckTxBlockedAccount (mempool layer), and
a legit user deposit still works.
Co-Authored-By: Claude <noreply@anthropic.com>
Merging master into feature/account-blacklist left duplicated ForkParaFee=-1 and ForkAccountBlacklist=-1 entries in chain33.fork.toml, chain33.para.toml and chain33.proxyminer.toml (both master and the blacklist branch had added them independently, and git didn't detect the duplication). Duplicate TOML keys can cause config parse/behaviour issues that surface as nodes failing to start mining, leading to CI docker-compose failures (block_wait stuck at height 0, dht timeout). Keep the master versions, drop the blacklist branch's copies. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1304 +/- ##
==========================================
+ Coverage 28.91% 29.12% +0.20%
==========================================
Files 382 382
Lines 73450 73475 +25
==========================================
+ Hits 21239 21400 +161
+ Misses 50233 50087 -146
- Partials 1978 1988 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Improve codecov patch coverage for the account blacklist logic added in this PR: - statedb_test.go: TestBlacklistBlocksFundOps covers isBlockedAccount gating in CanTransfer / Transfer / TransferToToken (all three return failure when sender or recipient is blacklisted, and TransferToToken surfaces ErrBlockedAccount). - account_blacklist_test.go: TestCreateBlockedAccount covers the Create path's checkBlockedAccount, symmetric with the existing Call test. Co-Authored-By: Claude <noreply@anthropic.com>
|
还需要提供为什么需要专门在evm中过滤的必要性说明 |
|
结论:不需要修改 1. chain33 层不是只在 checkblock,而是三处、全部走同一个核心三处拦截最终都调用
核心判定覆盖四个维度(
即 EVM 特有的目标地址(合约地址 / 纯转账 20 字节地址)已经在 chain33 层被解析并拦截。 2. 框架 checkTx 先于 EVM 驱动执行
3. 为什么 PR 的集成测试“看起来需要” EVM 层检查PR 里 phase2 的断言直接调用 4. 实测证据
5. 唯一“缺口”也不支持改 statedb.go唯一交易级检查看不到的场景是“非黑名单用户触发合约内部逻辑向黑名单地址转账”(交易字段干净)。但:
因此 |
同意这条分析里关于交易级拦截的结论,并结合主网已全量跑本 PR 分支的现状,补充处理原则。
chain33 在 mempool / 出块验块 / executor.checkTx 三层走同一套 CheckTxBlockedAccount,覆盖 from / to / realTo / EVM ContractAddr+Para。真实节点上,命中名单的交易在这三层的第一层就会被拦下,进不了 EVM 驱动。 因此本 PR 在 evm.go 的 CheckTx、exec.go 的 innerExec 再拦一遍交易字段,对真实节点是冗余的:代码会执行,但命中分支对真实交易开不了火。集成测试 phase2 直接调 exec.Exec(...),绕过了框架 checkTx,所以会「看起来需要」EVM 层。这是真实节点里不存在的路径。
框架只看交易信封上的地址,不看合约跑起来以后内部又 CALL 了谁、又转给了谁。 举例:普通用户 U(不在名单)调用付款合约 C(不在名单),合约内部再把原生币转给黑名单地址 A,或 CALL A。 信封:from=U、to/ContractAddr=C,框架放行,交易进入 EVM。
开新 fork:旧高度仍走现在的 Call/Create/statedb 检查,新高度才跳过。老代码必须留在二进制里供回放,不是物理删除。 |
重新审查:以「拦资产打出、不拦打入」为判定标准先更正上一条评论(#issuecomment-5472790458)中的两处说法:
按 @libangzhu 指出的正确标准(主要拦截从黑名单把资产打出)重新逐条核对如下。 A. chain33 已完整拦截(EVM 层再拦一遍无意义)任何"打出"都必须有一笔交易,而黑名单自己签名的交易 100% 被
B. chain33 拦不住、本 PR 能拦(有真实价值,建议保留)B1. 黑名单合约被内部 CALL 唤醒后转出自身余额 ⭐chain33 只看到 B2. token 预编译
|
| 位置 | 覆盖 | 理由 |
|---|---|---|
runtime/evm.go Call 的 target 检查 |
B1 | 唯一能阻止黑名单合约被内部唤醒 |
statedb.go TransferToToken 的 from 检查 |
B2 | 唯一能拦 token 预编译第三方代打,不可替代 |
statedb.go Transfer 的 sender 检查 |
B3 / B4 | 最后一道闸,覆盖 SELFDESTRUCT |
实际无意义(建议删除,或至少注明是纯防御冗余)
| 位置 | 为什么没意义 |
|---|---|
evm.go CheckTx 的 from 检查 |
① chain33 三层已在驱动之前拦死;② 该函数开头 if IsPara() { return nil },平行链上这段永远不执行 |
exec.go innerExec 两处 from 检查 |
msg.From() == tx.From(),chain33 三层已覆盖 |
statedb.go CanTransfer 的 sender 检查 |
与 Transfer 完全重复;且返回 false 会被上层翻译成 ErrNoBalance(exec.go:105-108),错误语义误导排查 |
runtime/evm.go Call/Create 的 caller 检查 |
顶层 caller = tx.From(已拦);内部 caller 是合约,而该合约要能执行必先过 Call 的 target 检查 → 分支不可达 |
Transfer/TransferToToken 的 recipient 检查 |
拦的是"打入"。符合 chain33「禁止收发任何交易」的原始设计,但按本 PR 优先级属次要;保留无害,删除不影响冻结目标 |
关于回放兼容
完全同意 @libangzhu 的处理原则:上述"无意义"的代码若已随本分支在主网执行过,不能直接物理删除。删除需满足其一:
- 新开 fork,旧高度仍走现逻辑,新高度跳过;或
- 先证明「启用本 PR 且
ForkAccountBlacklist生效至今」无任何一笔交易的执行结果依赖这些分支(检索blocked account日志,或新旧二进制回放同段区块比对 state hash)。
在此之前,D 表的"无意义"仅作为代码评审结论记录,不建议在本 PR 内直接删除。
一处可选的补强
isTransferOnly 路径的 receiver = BytesToAddress(msg.Para()):Address.SetBytes(common/address.go:66-71)对超长输入取后 20 字节,而 chain33 的 IsBlockedAccountRaw 要求精确 20 字节。两者不对称 —— 用 32 字节 padding 的 Para 可绕过 chain33 的 Para 维度。
但这条只能打入、不能打出,按本 PR 标准不紧急,仅作记录。
小结
| 结论 | |
|---|---|
| chain33 是否已完全拦截"打出"? | 主路径是的(A1–A7,黑名单自签交易全封死) |
| EVM 层是否有必要单独处理? | 有必要,但只需 3 处(B1/B2/B3 对应的 Call target、TransferToToken from、Transfer sender) |
| statedb.go 是否需要改? | 需要,但只有 Transfer 的 sender 与 TransferToToken 的 from 两处有实质意义;CanTransfer 建议撤(受回放约束,见上) |
| 是否存在无解盲区? | 有,ERC20 transferFrom,建议文档化 |
…-blind paths Answers the review questions on #1304: why filter inside the EVM when chain33 already intercepts at mempool / consensus / executor.checkTx. The framework only parses the transaction envelope (from / to / ContractAddr / 20-byte Para). Three asset-outflow paths exist only at EVM runtime and are invisible to it: - B1 clean user -> clean contract -> CALL -> blocked contract drains itself (runtime.Call target check is the only gate) - B2 token precompile transfer(from,to,amount): `from` comes from calldata, unbound to caller; chain33's Para check requires exactly 20 bytes and cannot see a 100-byte ABI payload (statedb.TransferToToken from check) - B3 SELFDESTRUCT pays via AddBalance -> statedb.Transfer(sender=contract). opSuicide takes the payer from contract.CodeAddr, so DELEGATECALL into blocked code drains the *blocked* address while runtime.DelegateCall has no check at all — statedb.Transfer's sender check is the sole gate. blacklist_gap_test.go proves each path with real coins balances and a fork-off control run showing the funds do leave when the check is inactive. B2 goes through the real tokenPrecompile.Run. The remaining checks (CheckTx from, innerExec from/contractAddr, CanTransfer, Call/Create caller, recipient dimensions) are defence in depth that never fires on a real node. They are annotated as such, not removed: the branch is already live on mainnet, and dropping them would require a new fork or a history audit to stay replay-safe. The integration test now asserts that chain33's CheckTxBlockedAccount rejects the attacker tx first, and treats the direct exec.Exec call as the defence-in-depth path it is rather than the primary gate. docs/security/evm-account-blacklist.md records the full analysis, the one unfixable blind spot (storage-accounted ERC20 transferFrom), and the replay constraint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
已按 #issuecomment-5503872286 的取舍结论和 @libangzhu 的回放约束更新,推送 改动源码只加注释,零语义变更。 三处实质防线(
集成测试 phase2 先断言 一处需要更正上轮结论的发现上轮把
结论不变(statedb.go 的 sender 检查必须保留),但依据从"意外覆盖"变成"唯一防线"。是否要给 验证
|
opSuicide pays from contract.CodeAddr, not contract.Address(). Under DELEGATECALL/CALLCODE that is the *callee*, so a clean proxy funded with X that borrows a blacklisted contract's SELFDESTRUCT makes the blacklisted address pay X to the beneficiary. Only statedb.Transfer's sender check caught it before. Add the same fork-gated checkBlockedAccount(addr) gate to DelegateCall and CallCode that Call already applies to its target. StaticCall is left alone: opSuicide returns ErrWriteProtection in read-only context. No new fork: the branch has not shipped yet, so there is no history to replay. TestGapB3b now verifies both gates independently (runtime gate via the full CALL→DELEGATECALL path; statedb gate by running the delegate contract directly) plus the fork-off control run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
|
This reverts commit 5f7be91.
回滚 DELEGATECALL/CALLCODE 的 addr 检查#issuecomment-5523029815 里"是否要给 DelegateCall 加独立检查"的结论是:不加。此前短暂加过( 验证方法
回滚理由
现状B3b 的唯一防线是 |
10*ctypes.DefaultCoinPrecision is an untyped constant assignable to the int64 parameter directly; the explicit conversion fails the CI linter (check_fmt job: unconvert at attack_integration_test.go:281). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🎉 This PR is included in version 1.72.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Wire ForkAccountBlacklist into EVM CheckTx/exec/runtime/statedb so blocked addresses are rejected consistently with chain33 consensus checks, and depend on github.com/33cn/chain33@aa71469c0974 from master.