fix(protocol): address review findings on Proposal0020

[dantaik]

Applies the actionable findings from the in-depth review of #22010, stacked on revamp-security-council.

Fixes

• F1 — broken pre-approval verification command (Proposal0020.md): the documented getProposal cast signature omitted the uint16 approvals return value, so the exact command approvers are told to run before approving failed with an ABI decode error. Corrected signature verified working against mainnet proposal 21.
• F2 — stale verify-mode instruction: BuildDirectProposal.logProposalAction still wrote "run the verify mode" (dropped in 6d57357) into every generated action file. The template now points at the getProposal comparison from the spec, and Proposal0020.action.md is regenerated — all six action To/Value/Data fields are byte-identical; only the instruction text changes.
• F3 — silent invalid MODE: run() now reverts with InvalidMode(mode) instead of logging and exiting 0.
• F7 — EOA assumption: checkBaseline now asserts SC_GUSTAVO_GONZALEZ.code.length == 0.
• F4 — key-registration mechanism: post-execution step 2 now names setPublicKey (the agent path) instead of implying setOwnPublicKey, which reverts MustBeListed for a non-listed agent.
• F5 — removeUnused() accuracy: the Action 6 comment and spec now state it prunes the account enumeration only; the removed members' appointerOf/agent mappings persist (harmlessly).
• F6 — ordering precision: spec and script comments now distinguish the contract-enforced dependency (Action 1 before Action 3) from the defensive placement of Actions 4–5.

Verification

• Invalid mode: MODE=bogus now fails the script (InvalidMode("bogus")).
• MODE=print regeneration: committed Proposal0020.action.md matches; calldata diff vs the previous file is empty.
• MODE=l1dryrun on a mainnet fork: green end-to-end, including the new EOA baseline assertion.
• forge fmt with the repo config; no lines exceed 100 chars.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NXZ8HWv6KYVV3fgC6MGJiv

---

Generated by Claude Code

[github-actions]

🐋 DeepSeek Code Review

🟡 Warnings

• Overly broad post-state invariant in Proposal0020.s.sol
  The new checkPostState loop asserts that every account returned by getRegisteredAccounts() is a listed signer. This is stronger than the actual action guarantee: Action 6 only prunes the specific delisted members. If EncryptionRegistry can ever enumerate non-signer accounts (e.g., agent-held keys or other unlisted accounts not part of this proposal), the dryrun will fail even though the proposal executed correctly.
  This passed on the current mainnet fork/proposal, so it isn’t blocking, but it’s a brittle invariant. Consider checking only the removed accounts or documenting explicitly that the registry enumeration is expected to contain only listed signers.

🔵 Suggestions

• Use abi.encodeCall instead of abi.encodeWithSignature
  For the new getRegisteredAccounts() call in checkPostState, abi.encodeCall(IEncryptionRegistry.getRegisteredAccounts, ()) would be safer and avoid a raw string signature mismatch. Not critical for a script, but a clear type-safety improvement.

• Add NatSpec/documentation for InvalidMode
  The new custom error is declared at the bottom with the other errors but has no NatSpec. The other errors are grouped, so adding a brief inline comment would keep the section consistent.

---

Automatically triggered on PR update • model: deepseek-v4-pro

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96cc8ae47e

ℹ️ 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".

[chatgpt-codex-connector]

Move the custom error to the end of the contract

Adding InvalidMode alongside the declarations extends the existing layout violation in this non-interface Solidity file; place the custom error declarations in the contract's final custom-errors section as required by the protocol coding standard.

AGENTS.md reference: packages/protocol/AGENTS.md:L41-L45

Useful? React with 👍 / 👎.

[dantaik]

Fixed in 2b57591 — all three custom errors (MissingEnv, CheckFailed, InvalidMode) now live in a final Custom Errors section, resolving the pre-existing layout violation as well.

---

Generated by Claude Code

[dantaik]

Re the DeepSeek suggestions, disposition in 2b57591:

• Taken — make the removeUnused() claim auditable: checkPostState now asserts every account still enumerated by the registry is a listed signer (proving the pruning happened), and the spec's Action 6 note links the removeUnused() implementation. The persistence of appointerOf mappings stays documented prose rather than an assertion, since asserting implementation residue would make the dryrun fail if a future registry cleaned up more thoroughly.
• Declined — mode constants/enum: BuildDirectProposal.run() mirrors the existing BuildProposal.run() string-dispatch convention; diverging in one base only would be inconsistent. Happy to refactor both in a follow-up if the team wants it.
• Declined — drop the log before revert InvalidMode: kept intentionally; the log names the valid modes for the operator while the custom error carries the offending value.

---

Generated by Claude Code

[dantaik]

Re the latest DeepSeek round — no changes made, with reasons:

• The "overly broad invariant" warning is incorrect. In the deployed EncryptionRegistry, accountList only ever gains owner accounts that are listed at registration time (appointAgent requires isListed(msg.sender); setOwnPublicKey/setPublicKey require the owner to be listed; agents are never enumerated as accounts). removeUnused()'s postcondition is exactly "no unlisted account remains enumerated", and the assertion runs immediately after Action 6 in the same execution — so it asserts precisely the action's guarantee, not something stronger. Checking only the five removed seats would be strictly weaker.
• abi.encodeCall: would require vendoring the Aragon interfaces into taiko-mono; the whole BuildDirectProposal family deliberately uses string signatures against the deployed contracts (settings(), multisigSettings(), …). Keeping one style.
• NatSpec for InvalidMode: the protocol standard (packages/protocol/AGENTS.md) explicitly says "No natspec comments for errors"; the section is already consistent — all three errors are bare.

---

Generated by Claude Code

[ggonzalez94]

LGTM. Just left one small comment

[ggonzalez94]

should we check this explicitely?

[dantaik]

If "this" is the EOA property — it is now checked explicitly, on this exact line (code.length == 0, added by this PR).

If you mean the agent precondition (that SC_GUSTAVO_GONZALEZ is currently the Taiko Labs seat's appointed agent): that one is deliberately not pinned in checkBaseline, because it's expected to change before execution — Taiko Labs may rotate at any point after creation, and the dryrun must stay green in both worlds. simulatePreExecution handles it conditionally (rotates only if an appointer is still set), and checkPostState asserts the invariant that actually matters (appointerOf(SC_GUSTAVO_GONZALEZ) == address(0) once listed). An explicit appointerOf == SC_TAIKO_LABS baseline check would fail the rehearsal after a legitimate early rotation. Happy to add it as a warning-style log instead in a follow-up if you'd like visibility without the hard failure.

---

Generated by Claude Code

[dantaik]

Independent review: approve ✅

I re-verified this PR from scratch — deployed mainnet sources (Etherscan-verified), live chain state, and a clean toolchain reproduction (pnpm 9 + repo-pinned Foundry v1.4.2).

Findings confirmed fixed

• F1: the deployed getProposal returns (bool, uint16 approvals, ProposalParameters, bytes, IDAO.Action[], OptimisticTokenVotingPlugin) — six values. Live call against mainnet proposal 21: the corrected cast signature decodes cleanly; the old one fails with buffer overrun. ✅
• F2: MODE=print P=0020 regenerates Proposal0020.action.md byte-identical to the committed file; all six To/Value/Data fields are unchanged from the base branch — only the instruction text changed. ✅
• F3: MODE=bogus now fails the script with InvalidMode("bogus") instead of logging and exiting 0. ✅
• F4/F5: the Etherscan-verified EncryptionRegistry at 0x2eFD…42d1 is identical to taikoxyz/dao-contracts main. setOwnPublicKey reverts MustBeListed for a non-listed agent, so setPublicKey(owner, key) is indeed the agent path; removeUnused() prunes only accountList, leaving appointerOf/agent mappings intact (stale agent addresses stay reserved via the AlreadyAppointed check). The new spec wording is accurate. ✅
• F6: confirmed against deployed sources — only Action 1 → Action 3 is contract-enforced (removeSigners reverts when newLength < minSignerListLength; 10−5=5 < 8), while _updateMultisigSettings only requires minApprovals ≤ addresslistLength(), which 3 and 4 satisfy at every intermediate size (9, 10, 5). Actions 4–5 are defensive placement, exactly as now documented. ✅
• F7: checkBaseline now asserts the new member is an EOA, and the new checkPostState assertion verifies every account still enumerated by the registry is a listed signer (naming the offending address). ✅

Independent reproduction

• Recomputed all six calldata blobs with cast calldata — byte-identical to the committed Proposal0020.action.md.
• forge fmt --check passes on both changed files with the pinned toolchain (note: Foundry 1.5.x flags the readRaw header, but that's a formatter-version artifact — the repo-pinned v1.4.2 is clean).
• MODE=l1dryrun on a fresh mainnet fork: green end-to-end — baseline passes against live state, proposal 22 created, agent rotation simulated, all six actions executed as the DAO, and every post-state assertion (including the new pruning check) passes.
• Custom errors now sit in the end-of-file // Custom Errors section per the protocol conventions. 👍

Nits (non-blocking)

1. The console2.log("Error: Invalid mode…") in run() is redundant with the InvalidMode(mode) revert.
2. The spec links to dao-contracts/blob/main/.../EncryptionRegistry.sol — consider pinning to a specific commit for long-term reproducibility.

No functional concerns; with respect to the proposal calldata this is a docs/tooling-only change. Ship it.