Feat/multi watcher #232
Feat/multi watcher #232
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (9)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR refactors attestation tracking from global boolean flags to per-watcher cumulative models in EVMxSwitchboard and MessageSwitchboard. It removes Changes
Sequence Diagram(s)sequenceDiagram
participant Watcher
participant Switchboard
participant Storage
rect rgb(200, 220, 255)
note over Watcher,Storage: OLD: Global Attestation Boolean
Watcher->>Switchboard: attest(digest)
Switchboard->>Storage: isAttested[digest] = true
Note over Storage: Single boolean, no per-watcher tracking
Switchboard->>Switchboard: allowPayload checks isAttested[digest]
end
rect rgb(220, 200, 255)
note over Watcher,Storage: NEW: Per-Watcher Cumulative Model
Watcher->>Switchboard: attest(digest) with signature
Switchboard->>Storage: isAttested[watcher][digest] = true
Switchboard->>Storage: attestations[digest]++
alt attestations[digest] >= totalWatchers
Switchboard->>Storage: isValid[digest] = true
end
Note over Storage: Per-watcher tracking + threshold validation
Switchboard->>Switchboard: allowPayload checks isValid[digest]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
|
|
||
| isAttested[digest_] = true; | ||
| // Prevent double attestation | ||
| if (isAttested[watcher][digest_]) revert AlreadyAttested(); |
There was a problem hiding this comment.
is digest not unique enough to check if it was attested ?
There was a problem hiding this comment.
here every watcher will attest on same digest, digest doesn't include watcher address
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
test/PausableTest.t.sol (1)
224-244: These tests don't verify revert behavior.Lines 224-244 pause the watcher then only assert it's paused—they never call
executePayload()orresolvePayload()to test that these functions actually revert. The function names promise to test revert behavior but don't deliver.Compare with the correct tests at lines 246-255 and 257-273, which actually invoke the functions and expect reverts. Remove these incomplete tests or rewrite them to match the pattern of the working tests below.
Apply this diff to remove the incomplete tests:
- function test_Watcher_ExecutePayload_WhenPaused_ShouldRevert() public { - // Pause the contract - vm.prank(owner); - watcher.grantRole(PAUSER_ROLE, pauser); - vm.prank(pauser); - watcher.pause(); - - // The executePayload function should revert due to whenNotPaused modifier - assertTrue(watcher.paused()); - } - - function test_Watcher_ResolvePayload_WhenPaused_ShouldRevert() public { - // Pause the contract - vm.prank(owner); - watcher.grantRole(PAUSER_ROLE, pauser); - vm.prank(pauser); - watcher.pause(); - - // The resolvePayload function should revert due to whenNotPaused modifier - assertTrue(watcher.paused()); - } function test_Watcher_executePayload_WhenPaused_ShouldRevert() public {auditor-docs/MESSAGE_FLOW.md (1)
203-277: Attestation and digest storage description is stale vs refactored designThis section still documents the old model where:
- Socket stores
payloadIdToDigest[payloadId] = digestin_verify().- Switchboards expose a single
isAttested[digest]boolean, andallowPayloadsimply returns that flag.Per the PR summary, the implementation now:
- Removes
payloadIdToDigeststorage fromSocket.sol.- Refactors attestation tracking to a per‑watcher cumulative model in
EVMxSwitchboardandMessageSwitchboard(likely with thresholds/weights and “valid” status rather than a single global bool).This should be updated so that:
- Socket’s role in digest storage is described accurately (or omitted if Socket no longer holds digest state).
- The attestation flow explains how individual watcher attestations are combined, what constitutes a “valid” payload, and which contract owns the final verdict.
- The
allowPayloadbehavior matches the new per‑watcher logic instead of returning a legacyisAttested[digest]boolean.As written, auditors and integrators reading this flow will reason about the wrong trust and replay properties.
Also applies to: 354-417
auditor-docs/SECURITY_MODEL.md (1)
203-263: Security invariants and state lists still reference removed/changed attestation storageThe “Critical Invariants” and “State Modification Points” sections still assume:
- A write‑once
payloadIdToDigest[payloadId]mapping owned by Socket.- A global boolean
isAttested[digest]that flips from false to true and never back.- These are listed as core replay/authorization state (
executionStatus,payloadIdToDigest,isAttested, etc.).Per the PR summary, the implementation now:
- Removes
payloadIdToDigestfromSocket.sol.- Moves to a per‑watcher cumulative attestation model in the switchboards.
This should be updated so that:
- Invariants describe whatever replaces
payloadIdToDigest(e.g., digest computation rules and any new binding between payload parameters and switchboard state).- Attestation invariants describe the per‑watcher aggregation (e.g., thresholds/weights, monotonicity of individual watcher votes) rather than a single global
isAttested[digest]flag.- The list of “High Impact State Changes” matches actual storage variables in Socket and the switchboards after the refactor.
Leaving these invariants in their pre‑refactor form will cause auditors to reason about guarantees that the code no longer enforces.
Also applies to: 265-283, 297-313, 363-378
auditor-docs/TESTING_COVERAGE.md (1)
50-55: Update Socket.sol coverage section to reflect digest storage moving out of Socket
Socket.soltests still listpayloadIdToDigest storageunder “State Management”, but the contract no longer stores digests; that logic now lives in switchboard contracts. This should be updated so auditors and reviewers focus digest-related tests on the correct contract (e.g.,MessageSwitchboard.sol) and don’t chase a non-existent mapping inSocket.sol.Also applies to: 89-101
test/protocol/switchboard/EVMxSwitchboard.t.sol (1)
12-13: ImportGOVERNANCE_ROLEor adjust usage to avoid compile errorThis test uses
GOVERNANCE_ROLEinevmxSwitchboard.grantRole(GOVERNANCE_ROLE, owner);but only importsWATCHER_ROLEfromAccessRoles.sol. UnlessGOVERNANCE_ROLEis brought into scope elsewhere (it isn’t in this file), this will not compile.This should either:
- extend the import to include
GOVERNANCE_ROLE, e.g.
import {GOVERNANCE_ROLE, WATCHER_ROLE} from ".../AccessRoles.sol";
or- switch to whatever role/token the contract actually expects for
grantWatcherRoleadministration.Without that, these tests won’t run.
Also applies to: 57-61
🧹 Nitpick comments (10)
auditor-docs/FAQ.md (3)
8-45: Consider converting bold subsection headers to markdown headings for better document structure.Lines like
**A1: Switchboards are trusted by Plugs/Apps**use emphasis instead of proper markdown headings (e.g.,### A1: Switchboards are trusted by Plugs/Apps). This pattern repeats throughout the Assumptions and subsequent Q&A sections. While functionally fine, converting bold sections to###or####headings would improve markdown compliance and enable better document parsing/navigation.
445-445: Add language identifiers to fenced code blocks.Lines 445 and 508 contain fenced code blocks without language specifications. Line 445 (transaction ordering example) and line 508 (fee distribution example) should specify a language hint (e.g.,
```solidityor```text) for proper syntax highlighting and markdown compliance.Also applies to: 508-508
533-545: Consolidate repetitive "only" adverbs in the Q16 answer.Lines 534–538 use "only" four times in close proximity ("Can only increase," "can only increase," "only the source plug," etc.). This creates a monotonous rhythm. Rewording a few instances (e.g., "Can only increase" → "Cannot decrease") improves readability without changing meaning.
contracts/protocol/SocketUtils.sol (1)
24-27: Digest/return-data behavior unchanged; consider tightening comment wordingThe only changes here are to the
maxCopyBytesdeclaration formatting and the_createDigestdoc comment. Runtime behavior of digest creation andsimulate()is unchanged and still matches the intended pattern (length‑prefixed variable fields +abi.encodePacked).This comment should explicitly say something like “Uses
abi.encodePackedinstead ofabi.encodefor bytes fields, together with length prefixes, to remain collision‑resistant and cross‑chain compatible.” to avoid confusion when auditors scan for collision risks.Also applies to: 61-62
auditor-docs/MESSAGE_FLOW.md (1)
21-21: Add languages to fenced code blocks for linting and readabilityMany code fences use bare triple‑backticks. This should add appropriate languages (for example
solidity,text, orbash) to each of these to satisfy MD040 and make rendered highlighting clearer for auditors.Also applies to: 36-36, 51-51, 159-159, 185-185, 193-193, 199-199, 205-205, 213-213, 238-238, 246-246, 259-259, 293-293, 301-301, 310-310, 643-643, 650-650, 656-656
auditor-docs/SECURITY_MODEL.md (1)
209-213: Fix fenced code languages and straynoncereference for markdown hygieneSeveral fenced code examples in the invariants/attack‑surface/nonce‑management sections use bare ``` without a language, and markdownlint flags an undefined reference for “nonce”.
This should:
- Add an explicit language to each fence (for example
solidityfor code‑like invariants ortextfor logical formulas) to satisfy MD040 and improve readability.- Ensure any reference‑style link or footnote label involving
nonce(e.g.,[nonce]) is either defined at the bottom of the doc or rewritten as inline codenonceto satisfy MD052.These are low‑risk doc cleanups but will keep the security docs linter‑clean for audit pipelines.
Also applies to: 221-225, 233-237, 245-247, 257-259, 267-271, 279-281, 289-291, 299-302, 310-313, 323-331, 338-343, 351-358, 383-390, 397-402
auditor-docs/SETUP_GUIDE.md (2)
584-596: Add language identifier to code fence.The fenced code block should specify a language for proper syntax highlighting.
Apply this diff:
-``` +```text Socket: 0x... (to be deployed) MessageSwitchboard: 0x... (to be deployed) FastSwitchboard: 0x... (to be deployed)--- `617-625`: **Format URLs as proper markdown links.** Bare URLs should be formatted as markdown links for better readability and consistency. Apply this diff: ```diff -- Solidity Docs: https://docs.soliditylang.org/ -- Foundry Book: https://book.getfoundry.sh/ -- Solady Docs: https://github.com/Vectorized/solady +- [Solidity Docs](https://docs.soliditylang.org/) +- [Foundry Book](https://book.getfoundry.sh/) +- [Solady Docs](https://github.com/Vectorized/solady) **Security Resources**: -- Smart Contract Security Best Practices: https://consensys.github.io/smart-contract-best-practices/ -- DeFi Security Tools: https://github.com/crytic/building-secure-contracts +- [Smart Contract Security Best Practices](https://consensys.github.io/smart-contract-best-practices/) +- [DeFi Security Tools](https://github.com/crytic/building-secure-contracts)auditor-docs/TESTING_COVERAGE.md (1)
666-687: Fix markdown headings to satisfy MD036 and improve structureSections like “Invariant 1: Execution Uniqueness”, “Invariant 2: Fee Conservation”, etc., are formatted as bold text instead of markdown headings, which triggers MD036 (
no-emphasis-as-heading). These should be changed to proper headings (for example,### Invariant 1: Execution Uniqueness) so the document structure is clearer and markdown linters pass cleanly.Also applies to: 702-711, 714-724, 763-776, 778-787
test/protocol/switchboard/EVMxSwitchboard.t.sol (1)
76-95: Helper factories for params are clearer and reduce duplicationThe refactored
_createExecutionParams,_createExecutionParams(overload),_createTransmissionParams, and_createTransmissionParams(overload), plus_createPayloadAndOverrides, now construct the structs directly in the return statement and centralize default values (e.g.,WRITEcallType, zeroedprevBatchDigestHash/extraData, default overrides).This should:
- reduce repetition across tests,
- keep callType consistently locked to
WRITEin these helpers,- make it easier to tweak defaults in a single place.
No functional issues here.
Also applies to: 100-122, 127-135, 166-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
.prettierignore(1 hunks)auditor-docs/AUDIT_FOCUS_AREAS.md(30 hunks)auditor-docs/AUDIT_PREP_SUMMARY.md(12 hunks)auditor-docs/CONTRACTS_REFERENCE.md(16 hunks)auditor-docs/FAQ.md(49 hunks)auditor-docs/MESSAGE_FLOW.md(28 hunks)auditor-docs/README.md(20 hunks)auditor-docs/SECURITY_MODEL.md(22 hunks)auditor-docs/SETUP_GUIDE.md(33 hunks)auditor-docs/SYSTEM_OVERVIEW.md(10 hunks)auditor-docs/TESTING_COVERAGE.md(54 hunks)contracts/evmx/base/AppGatewayBase.sol(1 hunks)contracts/evmx/helpers/ForwarderSolana.sol(2 hunks)contracts/protocol/Socket.sol(2 hunks)contracts/protocol/SocketConfig.sol(1 hunks)contracts/protocol/SocketUtils.sol(2 hunks)contracts/protocol/base/MessagePlugBase.sol(1 hunks)contracts/protocol/interfaces/ISocket.sol(0 hunks)contracts/protocol/switchboard/EVMxSwitchboard.sol(9 hunks)contracts/protocol/switchboard/MessageSwitchboard.sol(8 hunks)contracts/protocol/switchboard/SwitchboardBase.sol(2 hunks)contracts/utils/common/Constants.sol(0 hunks)contracts/utils/common/Errors.sol(1 hunks)test/PausableTest.t.sol(1 hunks)test/SetupTest.t.sol(1 hunks)test/encode.t.sol(10 hunks)test/protocol/Socket.t.sol(2 hunks)test/protocol/switchboard/EVMxSwitchboard.t.sol(16 hunks)test/protocol/switchboard/MessageSwitchboard.t.sol(18 hunks)
💤 Files with no reviewable changes (2)
- contracts/utils/common/Constants.sol
- contracts/protocol/interfaces/ISocket.sol
🧰 Additional context used
🪛 LanguageTool
auditor-docs/FAQ.md
[style] ~536-~536: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...the source plug can increase fees - Can only increase, not decrease - Native fees: A...
(ADVERB_REPETITION_PREMIUM)
[style] ~635-~635: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ource plug can increase fees 2. Refunds only to specified refundAddress 3. Refunds o...
(ADVERB_REPETITION_PREMIUM)
[style] ~636-~636: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...y to specified refundAddress 3. Refunds only when watcher-approved 4. Refunds only p...
(ADVERB_REPETITION_PREMIUM)
[style] ~637-~637: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...s only when watcher-approved 4. Refunds only possible once 5. Fees in successful exe...
(ADVERB_REPETITION_PREMIUM)
auditor-docs/AUDIT_PREP_SUMMARY.md
[uncategorized] ~124-~124: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...g, not race condition; low probability, low impact - Verdict: Acceptable **9. Cross-C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~129-~129: Ensure spelling is correct
Context: ...y** - Reason: CEI pattern + unique payloadIds per call - Verdict: Safe by design ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
auditor-docs/SECURITY_MODEL.md
[style] ~104-~104: To form a complete sentence, be sure to include a subject.
Context: ...e (Fully adversarial) Behavior: - May be malicious or buggy - Can attempt ree...
(MISSING_IT_THERE)
[style] ~149-~149: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “DOS”.
Context: ...or**: - May underpay fees - May try to DOS system with spam - May attempt double-spending...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.18.1)
auditor-docs/FAQ.md
14-14: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
20-20: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
26-26: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
33-33: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
40-40: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
362-362: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
445-445: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
508-508: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
997-997: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
auditor-docs/MESSAGE_FLOW.md
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
159-159: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
185-185: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
199-199: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
205-205: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
213-213: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
238-238: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
246-246: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
259-259: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
301-301: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
310-310: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
643-643: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
650-650: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
656-656: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
auditor-docs/CONTRACTS_REFERENCE.md
335-335: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
auditor-docs/TESTING_COVERAGE.md
678-678: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
690-690: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
714-714: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
763-763: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
778-778: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
auditor-docs/SECURITY_MODEL.md
209-209: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
233-233: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
399-399: Reference links and images should use a label that is defined
Missing link or image reference definition: "nonce"
(MD052, reference-links-images)
400-400: Reference links and images should use a label that is defined
Missing link or image reference definition: "nonce"
(MD052, reference-links-images)
auditor-docs/SETUP_GUIDE.md
584-584: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
617-617: Bare URL used
(MD034, no-bare-urls)
618-618: Bare URL used
(MD034, no-bare-urls)
619-619: Bare URL used
(MD034, no-bare-urls)
623-623: Bare URL used
(MD034, no-bare-urls)
624-624: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (23)
.prettierignore (1)
39-40: Entries appropriately ignore generated and documentation artifacts.Adding
foundry.lock(generated by Foundry's package manager) andinternal-audit/(audit documentation directory) to Prettier's ignore list makes sense for this project setup. Both entries follow the existing pattern and won't conflict with other ignores.contracts/evmx/base/AppGatewayBase.sol (1)
115-116: Line reflow looks good. This formatting change improves readability without altering behavior. The method chaining across lines follows standard Solidity style patterns.test/PausableTest.t.sol (2)
9-9: Import formatting corrected.Minor indentation adjustment for consistency.
246-273: Correct test pattern.These tests properly call the functions and expect reverts. The ones immediately above (lines 224-244) should be removed in favor of these.
auditor-docs/FAQ.md (1)
1-1235: FAQ content is comprehensive and well-structured; no functional issues detected.The documentation effectively covers system assumptions, architecture, security, operations, fees, and edge cases. Changes align with the PR's focus on multi-watcher and per-chain governance improvements. Note: Q30 (deadline enforcement) and related sections could reference the new
grantWatcherRole/revokeWatcherRolegovernance functions if they introduce changes to watcher trust models, but this appears to be intentionally deferred to contract-specific documentation.contracts/protocol/base/MessagePlugBase.sol (1)
24-33: No behavioral change in constructorChange here is formatting-only (blank line removal). Validation,
_setSocket,switchboardId, andswitchboardwiring plusconnectcall all behave exactly as before.contracts/protocol/SocketConfig.sol (1)
141-151: Spacing-only change ingetPlugConfigThis should keep the existing behavior intact (
switchboardId == 0continues to short‑circuit to(0, "")) and just improves readability of the equality check.contracts/utils/common/Errors.sol (1)
163-167: LGTM!The new error declarations follow consistent patterns and align with the governance-driven access model introduced in this PR.
contracts/evmx/helpers/ForwarderSolana.sol (2)
32-39: LGTM! Correct validation added.The else clause now properly reverts with
InvalidSolanaChainSlug()when the chain slug is neither Solana mainnet nor devnet. This is a correctness improvement over the previous implementation.
66-66: LGTM! Formatting change only.Single-line function signature is more concise without changing behavior.
test/encode.t.sol (1)
18-18: LGTM! Formatting changes only.These are pure formatting adjustments with no functional impact.
auditor-docs/README.md (1)
1-597: LGTM! Documentation improvements.The expanded documentation structure with additional sections and formatting improvements enhances audit readiness and reviewer experience.
auditor-docs/AUDIT_FOCUS_AREAS.md (1)
1-728: LGTM! Comprehensive audit focus documentation.The expanded documentation with explicit file references, code examples, and priority classifications provides clear guidance for auditors.
contracts/protocol/switchboard/SwitchboardBase.sol (2)
8-8: LGTM! Added GOVERNANCE_ROLE import.This import aligns with the broader governance role management introduced in this PR.
40-49: LGTM! Improved constructor initialization order.Moving
_initializeOwner(owner_)before settingchainSlugandsocket__follows best practice by initializing access control first. This ensures ownership is established before any other state initialization.test/protocol/Socket.t.sol (2)
25-30: LGTM! Formatting change only.Multi-line constructor parameters improve readability without changing behavior.
283-288: LGTM! Formatting change only.Multi-line constructor parameters improve readability without changing behavior.
test/SetupTest.t.sol (1)
233-242: Watcher governance wiring for switchboards looks correctThis setup should give
socketOwnergovernance/rescue on both switchboards and correctly assign the watcher EOA as:
- global watcher on
EVMxSwitchboard, and- per-chain watcher on
MessageSwitchboardfor botharbChainSlugandoptChainSlug.This matches the per-switchboard/per-chain watcher model; no issues here.
contracts/protocol/Socket.sol (1)
29-35: Constructor and fallback encoding remain correctThe constructor still just forwards its parameters to
SocketUtilswith no behavioral change, and the fallback’s manual ABI encoding ofbytes32asbytes(offset=0x20,length=0x20, followed bypayloadId) is intact. This should keep all existing call patterns working as before.Also applies to: 242-255
test/protocol/switchboard/EVMxSwitchboard.t.sol (4)
422-433:isValidassertion matches the new attestation API
test_Attest_Successnow assertsevmxSwitchboard.isValid(digest)after a successfulattest. This should track the new validation API (replacingisAttested) and ensures tests are checking the right public surface for watcher attestations.
594-608: Nonce + signature tests forsetRevertingPayloadmatch the described schemeThe
setRevertingPayloadtests now:
- include a
nonce,- hash
address(evmxSwitchboard),CHAIN_SLUG,payloadId,isReverting, andnonce,- sign with the watcher key for the success path,
- use a mismatched signer for the failure path.
This should exercise:
- watcher authorization,
- correct digest domain separation (contract address + chain slug),
- nonce replay protection for this selector/flow.
This must stay in sync with the on-chain
setRevertingPayloadimplementation’s digest construction and_validateAndUseNonceusage; as long as those match, these tests look correct.Also applies to: 612-638
644-657: Default deadline event and state assertion cover the new API
test_SetDefaultDeadline_Successnow:
- expects
DefaultDeadlineSet(newDeadline)and- asserts
evmxSwitchboard.defaultDeadline() == newDeadline.This should fully cover the new event and ensure the stored default deadline actually updates, not just the emission.
697-754: Custom vs zero-deadlineprocessPayloadbehavior is exercised correctlyThe
ProcessPayloadtests now distinguish:
- non-zero overrides: only check that a non-zero payloadId is produced and the counter increments, and
- zero overrides: expect a
PayloadRequestedevent whose overrides encodeblock.timestamp + defaultDeadline().This should validate both “explicit deadline” and “use default” branches in
processPayloadwithout overfitting to internal implementation details.
| **2. Replay Protection** | ||
|
|
||
| - **Status**: ✅ Multi-layer protection in place | ||
| - **Mechanisms**: executionStatus, isAttested, nonce system | ||
| - **Result**: No double-execution or replay possible | ||
|
|
There was a problem hiding this comment.
Align replay protection description with isValid API
Replay protection is described as using executionStatus, isAttested, nonce system, but the switchboard API now exposes isValid(...) instead of isAttested(...). This should use the current name (isValid) so auditors don’t chase a non-existent function and so the documented mechanisms match the deployed interface.
Also applies to: 176-187
🤖 Prompt for AI Agents
In auditor-docs/AUDIT_PREP_SUMMARY.md around lines 36-41 (and also update the
similar block at lines 176-187), the document references a non-existent function
name isAttested; change the wording to reference the current switchboard API
function isValid(...) and adjust any explanatory text to reflect the isValid
naming (e.g., replace “isAttested” with “isValid” and ensure examples/mentions
match the deployed interface) so the replay protection mechanisms accurately
match the live API.
| ## Access Control Matrix | ||
|
|
||
| | Function | Contract | Roles Required | Restriction | | ||
| |----------|----------|----------------|-------------| | ||
| | `execute()` | Socket | None | Not paused, valid params | | ||
| | `sendPayload()` | Socket | None | Not paused, connected plug | | ||
| | `connect()` | Socket | None (msg.sender = plug) | Valid switchboard | | ||
| | `disconnect()` | Socket | None (msg.sender = plug) | Currently connected | | ||
| | `registerSwitchboard()` | Socket | None (msg.sender = switchboard) | Not already registered | | ||
| | `disableSwitchboard()` | SocketConfig | SWITCHBOARD_DISABLER_ROLE | - | | ||
| | `enableSwitchboard()` | SocketConfig | GOVERNANCE_ROLE | - | | ||
| | `setNetworkFeeCollector()` | SocketConfig | GOVERNANCE_ROLE | - | | ||
| | `setGasLimitBuffer()` | SocketConfig | GOVERNANCE_ROLE | >= 100 | | ||
| | `setMaxCopyBytes()` | SocketConfig | GOVERNANCE_ROLE | - | | ||
| | `pause()` | SocketUtils | PAUSER_ROLE | - | | ||
| | `unpause()` | SocketUtils | UNPAUSER_ROLE | - | | ||
| | `rescueFunds()` | SocketUtils/Switchboards | RESCUE_ROLE | - | | ||
| | `attest()` | Switchboards | WATCHER_ROLE | Valid signature | | ||
| | `markRefundEligible()` | MessageSwitchboard | WATCHER_ROLE | Valid signature + nonce | | ||
| | `refund()` | MessageSwitchboard | None | Must be eligible | | ||
| | `setMinMsgValueFees()` | MessageSwitchboard | FEE_UPDATER_ROLE | Valid signature + nonce | | ||
| | `setEvmxConfig()` | FastSwitchboard | onlyOwner | - | | ||
| | `setRevertingPayload()` | Switchboards | onlyOwner | - | | ||
| | Function | Contract | Roles Required | Restriction | | ||
| | -------------------------- | ------------------------ | ------------------------------- | -------------------------- | | ||
| | `execute()` | Socket | None | Not paused, valid params | | ||
| | `sendPayload()` | Socket | None | Not paused, connected plug | | ||
| | `connect()` | Socket | None (msg.sender = plug) | Valid switchboard | | ||
| | `disconnect()` | Socket | None (msg.sender = plug) | Currently connected | | ||
| | `registerSwitchboard()` | Socket | None (msg.sender = switchboard) | Not already registered | | ||
| | `disableSwitchboard()` | SocketConfig | SWITCHBOARD_DISABLER_ROLE | - | | ||
| | `enableSwitchboard()` | SocketConfig | GOVERNANCE_ROLE | - | | ||
| | `setNetworkFeeCollector()` | SocketConfig | GOVERNANCE_ROLE | - | | ||
| | `setGasLimitBuffer()` | SocketConfig | GOVERNANCE_ROLE | >= 100 | | ||
| | `setMaxCopyBytes()` | SocketConfig | GOVERNANCE_ROLE | - | | ||
| | `pause()` | SocketUtils | PAUSER_ROLE | - | | ||
| | `unpause()` | SocketUtils | UNPAUSER_ROLE | - | | ||
| | `rescueFunds()` | SocketUtils/Switchboards | RESCUE_ROLE | - | | ||
| | `attest()` | Switchboards | WATCHER_ROLE | Valid signature | | ||
| | `markRefundEligible()` | MessageSwitchboard | WATCHER_ROLE | Valid signature + nonce | | ||
| | `refund()` | MessageSwitchboard | None | Must be eligible | | ||
| | `setMinMsgValueFees()` | MessageSwitchboard | FEE_UPDATER_ROLE | Valid signature + nonce | | ||
| | `setEvmxConfig()` | FastSwitchboard | onlyOwner | - | | ||
| | `setRevertingPayload()` | Switchboards | onlyOwner | - | |
There was a problem hiding this comment.
Access control matrix misstates who can call unpause()
The table lists:
- Contract:
SocketUtils - Function:
unpause() - Roles Required:
UNPAUSER_ROLE
In the current code, unpause() is declared as:
function unpause() external onlyRole(GOVERNANCE_ROLE) {
_unpause();
}This should be updated so the matrix reflects GOVERNANCE_ROLE as the required role, or the implementation changed if the intended role really is UNPAUSER_ROLE. As written, this is a security‑relevant discrepancy for auditors.
🤖 Prompt for AI Agents
In auditor-docs/SECURITY_MODEL.md around lines 177 to 199, the access control
matrix incorrectly states that SocketUtils.unpause() requires UNPAUSER_ROLE
whereas the implementation uses onlyRole(GOVERNANCE_ROLE); update the matrix to
list GOVERNANCE_ROLE for unpause(), or alternatively change the contract
implementation to onlyRole(UNPAUSER_ROLE) if the intended policy is to allow the
UNPAUSER_ROLE to unpause; ensure the doc and code match and note which option
was chosen in the commit message.
| function setRevertingPayload( | ||
| bytes32 payloadId_, | ||
| bool isReverting_, | ||
| uint256 nonce_, | ||
| bytes calldata signature_ | ||
| ) external { | ||
| bytes32 digest = keccak256( | ||
| abi.encodePacked( | ||
| toBytes32Format(address(this)), | ||
| chainSlug, | ||
| payloadId_, | ||
| isReverting_, | ||
| nonce_ | ||
| ) | ||
| ); | ||
|
|
||
| address watcher = _recoverSigner(digest, signature_); | ||
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | ||
| usedNonces[watcher][nonce_] = true; | ||
|
|
There was a problem hiding this comment.
Block signature replays before toggling revertingPayloadIds.
setRevertingPayload flips the flag using a watcher-signed message but never checks usedNonces[watcher][nonce_] before writing it. Anyone can replay an old signature (e.g., the last isReverting = true message) indefinitely to force the payload back into the reverting state, even after the watcher has cleared it with a new nonce. This should gate the call with a NonceAlreadyUsed check (and revert) before setting the flag, mirroring the _validateAndUseNonce guard in MessageSwitchboard. Add the guard so previously consumed signatures cannot be replayed to overwrite the latest watcher intent.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/EVMxSwitchboard.sol around lines 210 to 229,
the function setRevertingPayload computes a watcher-signed digest and sets
usedNonces[watcher][nonce_] = true without first checking if that nonce was
already consumed, allowing signature replay to flip reverting flags; add a
nonce-usage guard like _validateAndUseNonce: first check
usedNonces[watcher][nonce_] and revert with NonceAlreadyUsed if true, then
proceed to mark the nonce used only after the check (or call the existing
_validateAndUseNonce helper if available) before toggling revertingPayloadIds so
previously consumed signatures cannot be replayed to overwrite current watcher
intent.
| /** | ||
| * @notice adds a watcher | ||
| * @param watcher_ watcher address | ||
| */ | ||
| function grantWatcherRole(address watcher_) external onlyRole(GOVERNANCE_ROLE) { | ||
| if (_hasRole(WATCHER_ROLE, watcher_)) revert WatcherFound(); | ||
| _grantRole(WATCHER_ROLE, watcher_); | ||
|
|
||
| ++totalWatchers; | ||
| } | ||
|
|
||
| /** | ||
| * @notice removes a watcher | ||
| * @param watcher_ watcher address | ||
| */ | ||
| function revokeWatcherRole(address watcher_) external onlyRole(GOVERNANCE_ROLE) { | ||
| if (!_hasRole(WATCHER_ROLE, watcher_)) revert WatcherNotFound(); | ||
| _revokeRole(WATCHER_ROLE, watcher_); | ||
|
|
||
| --totalWatchers; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Overriding this function from AccessControl to make sure owner can't grant Watcher Role directly, and should | ||
| * only use grantWatcherRole function instead. This is to make sure watcher count remains correct | ||
| */ | ||
| function grantRole(bytes32 role_, address grantee_) external override onlyOwner { | ||
| if (role_ == WATCHER_ROLE) revert InvalidRole(); | ||
| _grantRole(role_, grantee_); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Overriding this function from AccessControl to make sure owner can't revoke Watcher Role directly, and should | ||
| * only use revokeWatcherRole function instead. This is to make sure watcher count remains correct | ||
| */ | ||
| function revokeRole(bytes32 role_, address grantee_) external override onlyOwner { | ||
| if (role_ == WATCHER_ROLE) revert InvalidRole(); | ||
| _revokeRole(role_, grantee_); | ||
| } |
There was a problem hiding this comment.
Keep totalWatchers in sync when watchers renounce.
You increment totalWatchers on grant and decrement on revoke, but AccessControl still lets a watcher call renounceRole(WATCHER_ROLE, watcher_). That path skips your bookkeeping, so totalWatchers stays higher than the number of active watchers. After one renounce, new digests will never reach the quorum (attestations[digest_] >= totalWatchers) and isValid will remain false. This should either override renounceRole to block watcher self-renounce or adjust totalWatchers when it happens. Without it, the multi-watcher attestation flow bricks itself after a single renounce.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/EVMxSwitchboard.sol around lines 257-295,
renouncing the WATCHER_ROLE via AccessControl.renounceRole bypasses your
grantWatcherRole/revokeWatcherRole bookkeeping and leaves totalWatchers stale;
either override renounceRole to prevent self-renounce for WATCHER_ROLE or
override it to decrement totalWatchers when a watcher renounces. Implement an
override of renounceRole(bytes32 role_, address account_) that, when role_ ==
WATCHER_ROLE and account_ currently has the role, decrements totalWatchers (or
reverts to force using grantWatcherRole/revokeWatcherRole), then calls the
underlying _revokeRole/parent implementation; otherwise delegate to the parent
behavior. Ensure the function preserves the original access semantics (only the
role holder can renounce their own role) and keeps totalWatchers consistent.
| function grantWatcherRole( | ||
| uint32 siblingChainSlug_, | ||
| address watcher_ | ||
| ) external onlyRole(GOVERNANCE_ROLE) { | ||
| bytes32 role = keccak256(abi.encode(WATCHER_ROLE, siblingChainSlug_)); | ||
| if (_hasRole(role, watcher_)) revert WatcherFound(); | ||
| _grantRole(role, watcher_); | ||
|
|
||
| ++totalWatchers[siblingChainSlug_]; | ||
| } | ||
|
|
||
| /** | ||
| * @notice removes a watcher | ||
| * @param watcher_ watcher address | ||
| */ | ||
| function revokeWatcherRole( | ||
| uint32 siblingChainSlug_, | ||
| address watcher_ | ||
| ) external onlyRole(GOVERNANCE_ROLE) { | ||
| bytes32 role = keccak256(abi.encode(WATCHER_ROLE, siblingChainSlug_)); | ||
| if (!_hasRole(role, watcher_)) revert WatcherNotFound(); | ||
| _revokeRole(role, watcher_); | ||
|
|
||
| --totalWatchers[siblingChainSlug_]; | ||
| } | ||
|
|
||
| /** | ||
| * @dev Overriding this function from AccessControl to make sure owner can't grant Watcher Role directly, and should | ||
| * only use grantWatcherRole function instead. This is to make sure watcher count remains correct | ||
| */ | ||
| function grantRole(bytes32 role_, address grantee_) external override onlyOwner { | ||
| if (role_ != GOVERNANCE_ROLE && role_ != RESCUE_ROLE && role_ != FEE_UPDATER_ROLE) | ||
| revert InvalidRole(); | ||
| _grantRole(role_, grantee_); | ||
| } | ||
|
|
||
| /** | ||
| * @dev Overriding this function from AccessControl to make sure owner can't revoke Watcher Role directly, and should | ||
| * only use revokeWatcherRole function instead. This is to make sure watcher count remains correct | ||
| */ | ||
| function revokeRole(bytes32 role_, address grantee_) external override onlyOwner { | ||
| if (role_ != GOVERNANCE_ROLE && role_ != RESCUE_ROLE && role_ != FEE_UPDATER_ROLE) | ||
| revert InvalidRole(); | ||
| _revokeRole(role_, grantee_); | ||
| } |
There was a problem hiding this comment.
Account for watcher self-renounce to avoid permanent quorum inflation.
Watchers can still call renounceRole(keccak256(abi.encode(WATCHER_ROLE, siblingChainSlug_)), watcher_) via AccessControl. That path bypasses your revokeWatcherRole bookkeeping, leaving totalWatchers[siblingChainSlug_] unchanged even though the watcher is gone. From then on, attest() never sees attestations[digest_) >= totalWatchers[siblingChainSlug_], so isValid stays false and payloads stop executing. This should either override renounceRole (and adjust the counter) or prevent watchers from renouncing outright; otherwise a single renounce bricks the quorum logic you just introduced.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 704-748,
renouncing a watcher role via AccessControl.renounceRole bypasses
revokeWatcherRole and leaves totalWatchers[] stale; add tracking and an override
to handle this: when granting/revoking watcher roles set a mapping(bytes32 =>
uint32) watcherRoleToSlug and/or mapping(bytes32 => bool) isWatcherRole, then
override renounceRole(bytes32 role_, address account_) to detect watcher roles
(via isWatcherRole or watcherRoleToSlug) and either call revokeWatcherRole(slug,
account_) so the counter is decremented, or revert to forbid renounce for
watcher roles; update grantWatcherRole and revokeWatcherRole to populate/clear
the mappings accordingly.
Summary by CodeRabbit
Release Notes
Documentation
Refactor
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.