fix(p0-1): BLS pkAgg reconstruction from on-chain PKs#112
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ac34d9c08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function registerBLSPublicKey( | ||
| address validator, | ||
| BLS.G1Point calldata publicKey, | ||
| uint8 slot |
There was a problem hiding this comment.
This commit changes the on-chain ABI (registerBLSPublicKey now takes (address,BLS.G1Point,uint8) and a new external verify(...) entrypoint is added), but the checked-in artifact abis/BLSAggregator.json still exposes the old (address,bytes) signature and is missing new callable methods like verify, getBLSPublicKey, and validatorAtSlot. Any SDK/tooling that consumes the repo ABI will encode wrong calldata and fail against deployed contracts, so ABI artifacts need to be regenerated in the same change set.
Useful? React with 👍 / 👎.
| string[12] memory contractKeys = [ | ||
| "aPNTs", | ||
| "blsAggregator", |
There was a problem hiding this comment.
By removing blsValidator from the canonical deployment key set here (and in deploy scripts), the generated config schema no longer matches existing root tooling that still expects blsValidator (for example scripts/verify-all.sh reads .blsValidator and tries to verify the deleted BLSValidator.sol). In post-commit deployments this makes the verification pipeline fail before completing other contract checks, so the remaining scripts need to be updated alongside this schema change.
Useful? React with 👍 / 👎.
代码审查拒绝合并 — 存在 CRITICAL 密码学安全问题 CRITICAL — G1 公钥注册缺少 on-curve 及主子群验证
EIP-2537 的 G1ADD precompile（ 修复方案：在 require(BLS.isOnCurveAndSubgroupG1(publicKey), "InvalidBLSKey");若库不提供，需手动调用 precompile。 HIGH — 缺少真实密码学的端到端集成测试 建议在 CI 中增加一个使用真实 BLS12-381 密钥对的集成测试，在支持 EIP-2537（Pectra fork）的 anvil 环境中运行： forge test --fork-url <pectra-fork-rpc> -vvv --match-test test_BLS_RealCryptoHIGH — Nonce-before-verify 设计对运营造成同步挑战（见 PR#113） INFO — 需确认 |
|
已修复。 变更摘要: 在
新增错误类型： 测试（均已通过，31/31 BLS 测试 pass）：
同时修复了 |
CRITICAL resolved: G1 public key on-curve + subgroup validation addedCommit What was added
New error types:
Confirming EIP-2537 vs BN254 (HIGH finding)
address internal constant BLS12_G1ADD = 0x000000000000000000000000000000000000000b; // EIP-2537
address internal constant BLS12_G1MSM = 0x000000000000000000000000000000000000000C; // EIP-2537
address internal constant BLS12_PAIRING_CHECK = 0x000000000000000000000000000000000000000F; // EIP-2537These are all EIP-2537 BLS12-381 precompiles (0x0b–0x0f). The BN254
Test coverage
|
fanhousanbu
left a comment
fanhousanbu
left a comment
There was a problem hiding this comment.
Re-review: _validateG1Point G1ADD + G1MUL 子群校验实现已确认，CRITICAL 修复通过。
MEDIUM: SlotAlreadyTaken(slot) 用于两个语义不同的场景：
- validator 已注册且尝试换 slot（应是 ValidatorSlotLocked）
- 目标 slot 被其他 validator 占用（才是真正的 SlotAlreadyTaken）
调用方无法区分原因。建议拆成两个独立 error。不阻断。
LOW: _validateG1Point() 步骤1对 identity point 使用 InvalidBLSKeyNotOnCurve()，但 identity 技术上在曲线上，只是被明确禁止注册。error 名字有误导性，建议改为 InvalidBLSKeyIsIdentity()。
✅ Approved（re-review 通过）
Identity (all-zero G1 point) passes both G1ADD and r*O==O subgroup checks but is cryptographically invalid. Explicit pre-check matches fix/p0-1-bls-rewrite (PR #112) hardening.
Documents 5 on-chain roles, register/run/revoke flows, real-time liveness checks, SDK ABI breaking changes (proof encoding), and deployment ordering. Companion to PR #112 (BLS rewrite).
Removes stale '已知缺口' note: _reconstructPkAgg now does per-slot role+stake liveness check. Adds removeValidator/pruneValidator/ revokeBLSPublicKey to revocation flow (B + B'). Updates §5.1 matrix to include the new real-time validation entries.
fanhousanbu
left a comment
fanhousanbu
left a comment
There was a problem hiding this comment.
Re-review commit 27301a8: real-time liveness check 实现已看完。
HIGH（可用性）: _reconstructPkAgg 里 StakingNotConfigured 是硬 revert。
GTOKEN_STAKING() 返回 address(0) 时，所有 BLS verify/verifyAndExecute/executeProposal 全部失败，整个 DVT 共识系统停摆。Staking 合约升级、地址轮换或初始部署顺序不对时都会触发。建议加一个 owner 开关（如 skipLivenessCheck flag），让运维在紧急情况下临时绕过，避免因 Registry 配置问题导致无法 slash。
MEDIUM: revokeBLSPublicKey 从幂等（silent return）改为 revert KeyNotActive。现有运维脚本若以防御性方式调用（先 revoke 再重新注册）会意外中断。请在 NatSpec 或 Migration Guide 里注明这个语义变更。
LOW: _reconstructPkAgg 每次执行最多 13 次 REGISTRY.hasRole + 13 次 staking.roleLocks external call。单次 verify() 的 gas 估算请补充在 PR 描述里，让集成方知道 gas budget 上限。
设计方向正确，关闭了 validator 退出后仍有投票权的漏洞。
✅ Approved
jhfnetboy
force-pushed
the
fix/p0-2-validator-stake-gate
branch
2 times, most recently
from
2324453 to
4455df8
Compare
jhfnetboy
force-pushed
the
fix/p0-1-bls-rewrite
branch
from
27301a8 to
3da096e
Compare
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
…istration
registerBLSPublicKey now calls _validateG1Point() before storing any key.
The function:
1. Rejects the identity point (all-zero coordinates) — prevents key-
cancellation attacks where a ghost validator contaminates pkAgg.
2. Calls G1ADD precompile (0x0b) with P + O; a failed staticcall means
P is not on the BLS12-381 G1 curve (InvalidBLSKeyNotOnCurve).
3. Calls G1MUL precompile (0x0c) with scalar r (subgroup order); if
r*P != O the point is in a small subgroup and is rejected with
InvalidBLSKeyNotInSubgroup.
Without (3) an attacker could register a small-subgroup point that biases
the reconstructed pkAgg used in all subsequent pairing checks.
New errors: InvalidBLSKeyNotOnCurve, InvalidBLSKeyNotInSubgroup.
Tests (BLSAggregatorUnit.t.sol):
- test_ValidG1Point_Registers_Successfully
- test_IdentityPoint_Rejected_NotOnCurve
- test_OffCurvePoint_Rejected_NotOnCurve
- test_SmallSubgroupPoint_Rejected_NotInSubgroup
Also fixed BLSAggregator_PkAggReconstruct.t.sol setUp() to install G1ADD
and G1MUL precompile mocks BEFORE calling registerBLSPublicKey, so stub
keys pass the new validation gate.
All 31 BLS tests pass.
- Add _requireActiveValidator() at createProposal/executeWithProof - _reconstructPkAgg validates each slot's validator role+stake - New: DVTValidator.pruneValidator (permissionless), removeValidator (owner) - New: BLSAggregator.revokeBLSPublicKey (owner) - Tests for all new paths
jhfnetboy
force-pushed
the
fix/p0-1-bls-rewrite
branch
from
3da096e to
88eb870
Compare
|
@fanhousanbu 再次 rebase（#105 P0-2 已合并到 main，跳过了 3 个 commits）。代码内容不变，请 approve。 |
fanhousanbu
left a comment
fanhousanbu
left a comment
There was a problem hiding this comment.
✅ Approved（force-push re-review）
上次 review（commit 27301a8）已 approved，本次 rebase 后内容一致，重新确认：
已验证 CRITICAL 修复（上次 re-review 已通过）：
_validateG1PointG1ADD + G1MUL 子群校验已实现_reconstructPkAgg按 slot 重建聚合公钥，validator 退出后投票权漏洞关闭revokeBLSPublicKey/pruneValidator/removeValidator新增路径正确
HIGH（可用性）— 非阻断，已知限制：
_reconstructPkAgg 内 GTOKEN_STAKING() == address(0) 时硬 revert，Staking 合约轮换窗口期间 BLS 系统停摆。建议后续 sprint 加 skipLivenessCheck owner 开关。当前无 owner bypass，运维需保证 Registry 配置先行。
MEDIUM: revokeBLSPublicKey 从幂等改为 revert KeyNotActive，语义变更需在 Migration Guide 里注明（非阻断）。
设计方向正确，approve。
2 participants
Base 是 `fix/p0-2-validator-stake-gate`（PR #105），因为 P0-1 的测试 fixture 用了 P0-2 的 MockStakingPermissive。完整 stack: main → #104 P0-4+17 → #105 P0-2 → 本 PR。GitHub 只展示这条 commit 的 delta。
P0-1 (B6-C1a)
`BLSAggregator.verify(message, signerMask, pkAgg, sig)` 接受 caller 提供的 `pkAgg` —— 配对方程 `e(pk_agg, H(m)) == e(g1, sig)` 在数学上对调用者自由选择的任意 (sig, pkAgg) 组合都成立 → 完全无防护。组合 P0-4 `executeWithProof` 无鉴权，匿名灾难级：任意调用者伪造任意 BLS 证明触发任意 slash / blacklist。
Defense（采用 solady / 业界标准）
删除的文件
Storage 兼容性
Registry 里 `address public blsValidator` 槽位保留为 `address public __deprecated_blsValidator` —— UUPS 升级安全（不 shift slot）。
Bumps BLSAggregator to `4.0.0`, Registry to `5.3.0`.
Tests
Spec
`docs/security/2026-04-26-p0-prelaunch.md` §3 P0-1
`docs/security/2026-04-26-threat-model.md` T-01
2026-05-05 追加：Codex 第二轮深度审计修复
Commit
27301a8— 实时 DVT validator 活性检查 + 撤销函数背景
Codex 第二轮审计（参考
docs/architecture/dvt-validator-workflow.md）发现：原版只在addValidator注册时检查 stake，注册后退押 / slash 不会让isValidator和 BLS 公钥失效。攻击场景：恶意 validator 退押后保留 quorum 投票权。修复内容
DVTValidator.sol（+96 行）_requireActiveValidator(v)：实时检查isValidator+Registry.hasRole(ROLE_DVT, v)+roleLocks(v, ROLE_DVT) >= cfg.minStakecreateProposal/executeWithProof入口调用_requireActiveValidator(msg.sender)pruneValidator(address v)— permissionless eviction（仅当 v 已失去 role 或 stake 时成功）removeValidator(address v)— onlyOwner 强制清理BLSAggregator.sol（+77 行，构造器签名未变）_reconstructPkAgg对 mask 选中的每个 slot 新增 per-slot 实时校验：role + stake livenessrevokeBLSPublicKey改为严格语义（重复 revoke 现在 revertKeyNotActive）IRegistryStakingAwareBLS接口拿 staking + minStake，避免循环依赖；deploy 脚本零改动测试
DVTValidatorLiveness.t.sol+BLSAggregatorLiveness.t.solABI 变化（需通知集成方）
新 errors：
NotActiveValidator,ValidatorRoleRevoked,ValidatorStillEligible（DVTValidator）SlotValidatorRoleRevoked,SlotValidatorStakeBelowMinimum,StakingNotConfigured,KeyNotActive（BLSAggregator）新 events：
ValidatorPruned,ValidatorRemoved,BLSPublicKeyRevoked新函数：见上述 4 个
版本号：
DVTValidator-0.6.0/BLSAggregator-4.1.0关联文档
docs/architecture/dvt-validator-workflow.md（在security/audit-2026-04-25分支）— 完整描述 5 个角色 + 注册/运行/撤销三阶段 + SDK 破坏性 ABI 变更（proof 编码：abi.encode(uint256 signerMask, bytes sigG2Bytes)）。合并依赖（更正之前的 Supersedes 误判）
合并顺序必须：先 #104，再 #105，再本 PR，最后 #113。每合并一层，下游 PR 的 base 会被 GitHub 自动更新到 main。