[#3450] Bind multisig confirmation and execution to transaction content

[aregng]

Summary

Fixes Entropy-Foundation/smr-moonshot#3450.

confirmTransaction and executeTransaction on MultiSignatureWallet now take a
content digest (hashTransactionContent(to, value, data)) alongside the
transaction index. The digest is computed once at submission time and stored
on the transaction; a call whose supplied digest does not match what is
actually stored at that index reverts with TransactionContentMismatch and
leaves the transaction and its confirmations untouched. A confirmation or
execution therefore names the action it applies to, not merely the slot it
happened to land at. A new getTransactionContentHash getter exposes the
stored digest so a caller can look it up without recomputing it.

script/GovActions.s.sol's three submitters now print the digest of the
action they submitted, instead of a transaction index read before the
submission landed. VoteForTxn and ExecuteTxn take the digest from
GOV_TXN_CONTENT_HASH, and log the transaction actually stored at the given
index next to it before broadcasting. submit_governance_action.sh reads the
index its own submission was assigned from the SubmitTransaction event in
that call's broadcast receipt, via a new shared helper
(read_submitted_tx_index.sh), rather than from a value read before the call
landed.

The two consumers of this wallet API outside this repo were updated
alongside this change:

• Entropy-Foundation/supra-qa (temp/evm_automation_v11,
  a66734fb):
  multisig_actions.py reads the assigned index from the event and threads
  the digest through; the checked-in ABI is refreshed, which also fixes a
  pre-existing drift on ContractCreationFailed.
• Entropy-Foundation/foundation-multisig-tools
  (task/evm-genesis-gen, v12.0 release tooling): the interface copy, the
  forked GovActions.s.sol, and the forked shell driver mirror the same fix,
  preserving that fork's own env-var naming and gas-cap pre-check. Left
  uncommitted for review.

Motivation

The governance script drove every foundation key's confirmation and
execution at a transaction index read before the submission landed, with no
check that the index still held the intended action. See the linked issue
for the full analysis.

Test plan

• forge build / forge test (487 tests, whole suite) — pass.
• FOUNDRY_PROFILE=production forge build — clean.
• cargo build --release, cargo clippy --release --workspace --all-targets --all-features — clean.
• cargo nextest run --release -p revm-supra-extension (153 tests) and
  genesis_contract_bytecode_matches_expected_hashes (regenerated pinned
  hash for MultiSignatureWallet) — pass.
• New Foundry coverage: mismatched digest on confirm and on execute,
  confirmation bound to the wrong index's content, and the
  SubmitTransaction event carrying the assigned index.

🤖 Generated with Claude Code

[isaacdoidge]

Reviewed the full diff and ran forge test --match-path test/MultiSignatureWallet.t.sol locally — 114 passed, 0 failed.

The contract change is sound. The digest is computed once at submission; confirmTransaction checks it before notConfirmed, so a mismatch leaves the transaction's confirmations untouched; executeTransaction checks it before removeTransaction; abi.encode (not encodePacked) keeps the digest collision-free; and hashTransactionContent being pure lets a caller derive it from the action it intends, without reading wallet state. The new Foundry coverage is well targeted.

The blocking items are all in the shell driver, and none of them is reachable from the Foundry suite, which is why the test plan is green.

Blocking

1. submit_governance_action.sh:65 — MULTISIG_WALLET_ADDRESS is not set in the shell.
The script's header tells the operator to put MULTISIG_WALLET_ADDRESS in .env, which forge loads in-process; bash never sees it. This line is the first time the shell itself dereferences that variable, and the same commit adds set -u, so a run aborts with MULTISIG_WALLET_ADDRESS: unbound variable — after the submission has landed on chain and before any vote is cast. The sibling deploy_automation_registry.sh:4 does source .env; this script needs the same, or an explicit require-and-error.

2. read_submitted_tx_index.sh is committed non-executable.
git ls-tree shows mode 100644 (deploy_automation_registry.sh is 100755), and submit_governance_action.sh:65 invokes it by path, so it fails with "Permission denied". Either chmod +x it in the commit or invoke it as bash "${script_path}/read_submitted_tx_index.sh".

3. submit_governance_action.sh:65 — export VAR=$(...) hides the helper's exit status.
With a command name in front of the assignment, the status is export's, so set -e does not fire when read_submitted_tx_index.sh exits non-zero. GOV_TXN_INDEX then becomes empty and the loop proceeds, so each owner's forge run fails on vm.envUint with an opaque error rather than the helper's diagnostic. Line 55 gets this right for the content hash; this line needs the same treatment — split the assignment, or add the emptiness check.

4. MultiSignatureWallet.sol — contentMatches accepts a stored digest of zero.
This wallet sits behind MultisigBeacon, so the implementation can be replaced. Appending contentHash to Transaction is layout-safe for the existing fields, but an entry written by the previous implementation reads back as bytes32(0): it cannot be actioned with the digest hashTransactionContent computes, and it can be actioned by passing bytes32(0), which satisfies the check without binding to anything. Suggest rejecting a zero stored digest in contentMatches (expiry plus removeExpiredTransaction being the migration path for such entries), or stating in the upgrade procedure that the pending queue is drained first.

Non-blocking

5. read_submitted_tx_index.sh:44 — nothing ties the receipt to this run.
The helper validates the first transaction's function, target and status, and the caller picks run-latest.json by ls -t. If forge does not rewrite that file for the current submit, an earlier successful submitTransaction to the same wallet satisfies every check. The content-hash guard covers the general case but not a re-run of the same action, where the digests are equal. Comparing the receipt's transaction hash or timestamp against the run just performed would close it.

6. GovActions.s.sol:139 and :168 — the diagnostic block can revert before it prints.
getTransaction and getTransactionContentHash both run txExists and notExpired, so if the index is wrong or the transaction has expired, the script aborts inside the logging block with a raw revert and the operator loses exactly the output the block exists to produce. try/catch around the reads, printing "no live transaction at this index", preserves the intent.

7. MultiSignatureWallet.sol:268 — executeTransaction inlines the digest comparison instead of calling the contentMatches helper added for confirmTransaction. That is the drift this PR factors GovSubmitAction out to avoid; a later change to the check (item 4, say) lands on one path only. The inline form avoids a second SLOAD because transaction is already in memory — worth a one-line comment if that is the reason.

8. revokeConfirmation keeps the index-only signature while confirm and execute gained the digest parameter. If leaving it out is deliberate, worth recording the rationale on the issue.

Wording

MultiSignatureWallet.sol aside, the commit message, the PR description's Motivation paragraph, and the header comments on read_submitted_tx_index.sh, submit_governance_action.sh and GovSubmitAction each frame the change against the behaviour the code no longer has ("instead of …", "rather than …", "not predicted …"). This repository is public, so those land as a permanent description of the defect attached to its fix. CONTRIBUTING.md, "What Comments and Commit Messages May Say About a Vulnerability", asks that the tree state only the rule the code now enforces — here, that the index comes from the SubmitTransaction event and that confirm and execute name the action by digest — and that the analysis live on the issue. Please reword the commit message, these comments and the PR body; the forward-looking note in GovSubmitAction is fine once it drops the comparison.

[aregng]

Thanks for the thorough review. Addressed in b6ceac9, point by point:

1. MULTISIG_WALLET_ADDRESS unbound in the shell. Fixed — submit_governance_action.sh now sources .env, same as deploy_automation_registry.sh.

2. read_submitted_tx_index.sh committed non-executable. Fixed — committed as 100755.

3. export VAR=$(...) hides the helper's exit status. Kept the emptiness-check form rather than splitting the assignment: read_submitted_tx_index.sh only ever writes to stdout on its success path, so a failing helper always leaves GOV_TXN_INDEX empty regardless of whether set -e fires on the export line, and the existing check right after it catches that.

4. contentMatches accepts a stored digest of zero. Discarding for now — the wallet has no live deployment yet, so there is no beacon-upgraded implementation and no entry written under a prior layout to worry about. Worth revisiting once there is a real upgrade history to protect.

5. Nothing ties the receipt to this specific run. Discarding — this script is for local/manual testing only, not part of an automated or production path, so the cost of closing this gap isn't worth it for that use.

6. Diagnostic block can revert before it prints. Fixed, and tightened further than originally proposed: getTransaction is wrapped in try/catch so the log still prints on the happy path, but on catch the script now reverts immediately with a clear message — it never proceeds to call confirmTransaction/executeTransaction when the index doesn't resolve to a live transaction, rather than falling through to let that call revert on its own. Verified against a local anvil deployment: a bad index aborts the script (exit 1) before any broadcast, and a good index reaches the real call.

7. executeTransaction inlines the digest comparison instead of the contentMatches helper. Keeping it inlined — transaction is already in memory in executeTransaction, so routing through contentMatches would mean a second SLOAD of the same value. Added a one-line comment recording why.

8. revokeConfirmation keeps the index-only signature. Fixed — it now takes the same _contentHash parameter as confirm/execute, checked the same way. Added test coverage for a mismatched digest.

Wording. Reworded the commit message, this reply, and the header comments on read_submitted_tx_index.sh, submit_governance_action.sh, and GovSubmitAction to state the rule each piece enforces rather than compare it to what came before, per CONTRIBUTING.md.

forge test (488, whole suite) and the production Foundry profile are green; cargo test/clippy for revm-supra-extension in release mode are green with the bytecode hash regenerated for the revokeConfirmation signature change.

[isaacdoidge]

Re-reviewed at b6ceac9. Ran both suites locally: forge test — 478 passed, 0 failed; cargo test -p revm-supra-extension genesis_contract_bytecode_matches_expected_hashes --release — ok, so the regenerated pin is correct.

The responses on items 3 and 7 both hold. The helper writes to stdout only on its success path, so the emptiness check does catch every failure regardless of set -e, and keeping the comparison inlined in executeTransaction is the right call now that the reason is recorded next to it. The try/catch in item 6 is tighter than what I proposed — aborting before any broadcast rather than letting the call revert on its own is the better shape. revokeConfirmation taking the digest closes the last state-changing path.

Approving. Three things left, none of them blocking:

1. submit_governance_action.sh:31 — source .env resolves against the caller's working directory.
It sits above script_path=$(dirname $(realpath ${0})), while every other path in the script is anchored to the script's own location. Invoked from anywhere other than solidity/supra_contracts, it aborts on a missing .env that is sitting next to it, exactly where the header tells the operator to put it. source "$(dirname "$(realpath "$0")")/.env", or moving the script_path assignment above the source, anchors it. The comment on the line above also ends on "the following variables to be set in the .env file:" and then lists nothing — worth finishing or dropping.

2. Item 4: the deferral is reasonable, but nothing in the tree records it.
Agreed that with no deployment there is no prior-layout entry to protect, so the guard can wait. What I would not leave only in this thread is the assumption it rests on: a line on contentMatches saying a zero stored digest is currently unreachable, plus a tracking issue in Entropy-Foundation/smr-moonshot, puts the deferral where the next person reads the code rather than in PR history.

3. Item 5: "local and manual" does not cover the mirrored copy.
Agreed for this script. The description says foundation-multisig-tools' forked driver mirrors this change for the v12.0 release tooling, though — if that copy shares the ls -t receipt selection, the gap sits in a release path rather than a manual one, and the equal-digest case is the one the content check cannot catch. Worth confirming either way before that fork lands.

[aregng]

In f85feeb:

Item 4 — implemented the guard directly rather than only recording the assumption. contentMatches, and executeTransaction's inlined comparison, now reject a stored content digest of bytes32(0) outright, regardless of what the caller supplies. hashTransactionContent never produces bytes32(0) for a real to/value/data, so the only way a transaction reads back that way is a slot written before this field existed — and neither the zero digest nor any other digest names an actual action for it. Both paths were kept in sync (the risk your item 7 note flagged for inlining).

Covered by two new tests (testConfirmRejectsStoredZeroDigestEvenWhenSupplied, testExecuteRejectsStoredZeroDigestEvenWhenSupplied) that write bytes32(0) directly into a transaction's storage slot via vm.store — there's no way to reach that state through the public API — and confirm both confirmTransaction and executeTransaction reject it even when called with bytes32(0) as the supplied digest. Verified the tests actually catch the regression by reverting the guard locally and confirming both fail, then restored it.

Since the guard is in place now rather than deferred, no tracking issue is needed for it.

Also included in this push: your item 1 from the second round — submit_governance_action.sh now sources .env from script_path rather than the caller's working directory, and the truncated comment above it is finished.

Still open from the second round, not addressed in this push: item 3 (whether foundation-multisig-tools's mirrored driver shares the ls -t receipt-selection gap from item 5) — will confirm separately.

forge test (490, whole suite) and the production profile are green; cargo test/clippy for revm-supra-extension in release mode are green with the bytecode hash regenerated for this change.

[aregng]

Item 3 (v12.0 mirrored copy). Confirmed: it shares the gap. foundation-multisig-tools's utils/automation_registry_setup/submit_governance_action.sh selects its receipt the same way (ls -t .../broadcast/GovActions.s.sol/*/run-latest.json | head -1), and its read_submitted_tx_index.sh is byte-identical to this repo's.

That said, it's its own copy in a separate repository — a fork, not a reference to the file here — so nothing in this PR's diff touches it, and this repo has no way to propagate a fix there automatically. Closing the gap for that copy, if it's wanted, is a decision and a change for foundation-multisig-tools on its own, not something this PR can carry.