[#3448] Harden MultiSignatureWallet and two-step the beacon

[aregng]

Summary

• Closes 3448: two confirmed authorization-bypass paths in MultiSignatureWallet, plus five secondary issues from the same audit finding.
• Confirmations are now scoped per owner-incarnation and per confirmation-threshold-epoch, so (1) a removed owner's earlier confirmation can't count again if they're later re-added, and (2) lowering the threshold can't retroactively satisfy a transaction that fell short of the old, higher threshold.
• Adds a multisig-gated cancelTransaction and a permissionless removeExpiredTransaction, bounds submission timeouts via a new multisig-settable maxTimeoutDuration, makes touching an expired transaction revert instead of succeeding, removes the dead numConfirmations field, trims OwnersAdded/OwnersRemoved event payloads, and switches MultisigBeacon to two-step ownership (Ownable2Step).

Motivation

Audit finding smr-moonshot#3448 (High, confirmed) found that MultiSignatureWallet — the sole governance authority over every EVM system contract in this fork — had two ways a quorum could be satisfied unintentionally. See the issue for the full write-up.

Test plan

• forge build — compiles clean, no new warnings on the changed files.
• forge test — full repo suite green (459/459 across 13 suites), including 94/94 in MultiSignatureWalletTest (new tests added for both fixed exploit chains, cancel/expiry/timeout mechanics, and beacon two-step ownership; 7 existing tests updated for the new revert-on-expiry behavior).
• New tests directly reproduce both original exploit chains from the issue and assert they are now inert.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

[isaacdoidge]

Ran forge test --match-contract MultiSignatureWalletTest at dec1f069: 94/94 pass. The epoch-stamping design is sound — the membership-first check in _isOwnerConfirmationValid, the remove/re-add and threshold-decrease coverage, and the cancel-self test all hold up.

Two items I'd like addressed before merge, plus smaller points inline.

Beacon renounceOwnership() (inline at MultisigBeacon.sol:56). Still reachable after the Ownable2Step switch; it is one of the items bluealloy#3448 lists, and this PR closes that issue.

Commit message. Two bullets in the body describe the behaviour the code no longer has ("… no longer counts if the address is later re-added", "rather than reusing confirmations gathered under a higher threshold"). Per smr-moonshot CONTRIBUTING.md → What Comments and Commit Messages May Say About a Vulnerability (which applies to this fork), commit messages state the rule now enforced and reference the issue for the rest — e.g. "Bind each confirmation to the owner incarnation and threshold under which it was cast", keeping the existing See #3448. This repo is public, so the message ships. The in-tree comments and test names are fine as written.

Not raised: the ExecutionFailed-only expectations on the nested admin-call tests (consistent with the nine existing tests of that shape, and mutation-sensitive), and views reverting on expiry (documented and tested as the intended behaviour).

[isaacdoidge]

Ownable2Step overrides only transferOwnership, so Ownable.renounceOwnership() is still callable here and would leave the beacon with no owner and upgradeTo permanently unreachable — the outcome the two-step change is guarding against, and one bluealloy#3448 lists explicitly. A short override that reverts (plus a test) closes it:

function renounceOwnership() public view override onlyOwner {
    revert OwnershipRenunciationDisabled();
}

[isaacdoidge]

Non-blocking: this re-implements OZ's UpgradeableBeacon by hand (~50 lines) to get Ownable2Step. contract MultisigBeacon is UpgradeableBeacon, Ownable2Step with the two override(Ownable, Ownable2Step) forwarders for transferOwnership/_transferOwnership gives the same ABI in ~12 lines and tracks OZ on library bumps. I checked that form: compiles on 0.8.34 against the vendored OZ and the full suite passes 94/94 with only the two upgradeTo.selector references in the test switching back to UpgradeableBeacon.

[isaacdoidge]

The interface NatSpec for confirmTransaction (L137), executeTransaction (L141) and revokeConfirmation (L146) still says the transaction is "removed if expired"; those now revert with TransactionAlreadyExpired and this function is the cleanup path. The contract's own @dev lines were updated — these should match. Likewise notExpired's @dev in the contract says it "reverts as though the transaction doesn't exist", but it uses a different error from txExists.

[isaacdoidge]

Minor: gating revoke on the full validity predicate means an owner whose stamp was invalidated (threshold lowered, or removed and re-added) gets TransactionNotConfirmed and can never emit RevokeConfirmation, while staying a member of confirmations[_txIndex]. Not a safety issue — the stale member is never counted — but semantically off. Gating on confirmations[_txIndex].contains(msg.sender) and clearing the stamps would let them clean up.

[isaacdoidge]

Two small things on the event trimming (here and at L353):

• No test asserts the OwnersAdded/OwnersRemoved payload, so the trimming — the behavioural change this block exists for — is unguarded. An expectEmit with the exact array in the add/remove tests would cover it.
• Under via_ir = true these blocks should carry assembly ("memory-safe") (shrinking a Solidity-owned array's length is memory-safe). solc only honours the annotation if every block in the contract has it, so the existing block in deployContract needs it too.

[isaacdoidge]

Nit: this three-line stamping block is duplicated verbatim in submitTransaction. A private _recordConfirmation(uint256 _txIndex, address owner) mirroring removeTransaction keeps the two paths from drifting. Relatedly, executeTransaction calls the public hasValidNumberOfConfirmations, which re-runs the txExists/notExpired guards it just executed; validNumberOfConfirmations(_txIndex) >= numConfirmationsRequired directly would do.

[isaacdoidge]

Re-reviewed at c4af829b. Every item from the first round is addressed:

• MultisigBeacon.renounceOwnership() now reverts with OwnershipRenunciationDisabled, with a test driving it through the multisig.
• Beacon is back on UpgradeableBeacon + Ownable2Step with the two override(Ownable, Ownable2Step) forwarders; the two test selectors moved back to UpgradeableBeacon.upgradeTo.
• Interface NatSpec for confirm/execute/revoke matches the revert-on-expiry behaviour, and notExpired's @dev now says why it uses its own error.
• revokeConfirmation gates on raw set membership so a stale entry can be cleared; covered by testRevokeConfirmationClearsStaleConfirmation.
• _recordConfirmation shared by submit/confirm; executeTransaction compares validNumberOfConfirmations directly.
• All three assembly blocks (including deployContract) carry ("memory-safe"); the trimmed OwnersAdded/OwnersRemoved payloads are asserted exactly.
• The first commit's message now states the enforced rule ("Bind each confirmation to the owner incarnation and threshold under which it was cast") and defers to See #3448.

Verified locally: forge test at c4af829b → 453/453 across 12 suites, 99/99 in MultiSignatureWalletTest.

One optional wording nit, not worth another round: the follow-up commit's body still has two clauses phrased as what the code used to do ("…the freeze-forever path Ownable2Step alone didn't cover", "instead of being permanently stuck as an unrevocable member"). If the branch gets rebased before merge, trimming those to the rule keeps the public history clean; if it merges as-is, fine.