-
Notifications
You must be signed in to change notification settings - Fork 10
Audit/9-11 #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Audit/9-11 #244
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 WalkthroughRefactors switchboard contracts to implement multi-signer authorization, replacing single-signer flows with batch signature validation across attest, setRevertingPayload, assignTransmitter, and markRefundEligible functions. Adds _validateSignature helper and internal _processAttestation for centralized logic. Updates event naming and test utilities to reflect multi-watcher scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Switchboard as EVMxSwitchboard/<br/>MessageSwitchboard
participant Validator as _validateSignature
participant Processor as _processAttestation
participant State as Contract State
rect rgb(200, 220, 240)
Note over Client,State: Multi-Signer Batch Attestation Flow
end
Client->>Switchboard: batchAttest(payloadId, digest, signatures[])
rect rgb(240, 230, 200)
Note over Switchboard,Validator: Validate All Signatures
end
loop For each signature in array
Switchboard->>Validator: _validateSignature(messageHash, sig, WATCHER_ROLE)
Validator->>Validator: _recoverSigner & _hasRole check
Validator-->>Switchboard: watcher address or revert
end
rect rgb(230, 240, 200)
Note over Switchboard,Processor: Process Per-Watcher Attestation
end
loop For each watcher
Switchboard->>Processor: _processAttestation(payloadId, digest, watcher)
Processor->>State: Check double-attestation guard
Processor->>State: Increment attestations[payloadId]
Processor->>State: Check if threshold reached → mark isValid
Processor-->>Switchboard: emit Attested(payloadId, digest, watcher)
end
Switchboard-->>Client: ✓ All attestations processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Multi-file refactoring introducing heterogeneous authorization patterns: batch signature validation with per-watcher nonce tracking, extracted _processAttestation logic, event renames, and new struct parameters. Requires separate reasoning across EVMxSwitchboard, MessageSwitchboard, test suites, and helper functions to verify signature validation correctness, nonce race conditions, role checks, and state consistency across double-attestation guards. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/protocol/switchboard/EVMxSwitchboard.sol (1)
253-277:setRevertingPayloadmust not accept empty signatures whentotalWatchers == 0
setRevertingPayloadrequiressignatures_.length == totalWatchersand then validates each signature. IftotalWatchers == 0(e.g. before any watcher is configured or after all are revoked), a call with an empty signatures array will:
- pass the length check,
- skip the validation loop entirely,
- and still update
revertingPayloadIds[payloadId_]and emitRevertingPayloadIdSet.This should not allow anyone to flip reverting status in a “no watchers configured” state.
This should either:
- enforce
totalWatchers > 0at entry, reverting (e.g. withWatcherNotSet()orWatcherNotFound()), or- treat
totalWatchers == 0as an invalid configuration and disable the function.A tight fix is:
function setRevertingPayload( bytes32 payloadId_, bool isReverting_, uint256 nonce_, bytes[] memory signatures_ ) external { - if (signatures_.length != totalWatchers) revert ArrayLengthMismatch(); + if (totalWatchers == 0) revert WatcherNotFound(); + if (signatures_.length != totalWatchers) revert ArrayLengthMismatch(); ... }Same class of issue exists in
assignTransmitterbelow and should be fixed there too.contracts/protocol/switchboard/MessageSwitchboard.sol (1)
618-643:setRevertingPayloadneeds the same watcher==0 guard
setRevertingPayloaduses the same pattern: length check, then a loop overtotalWatchersvalidating signatures. WhentotalWatchers == 0, an emptysignatures_array will:
- pass the length check,
- skip the
_validateSignature/_validateAndUseNonceloop,- and still update
revertingPayloadIds[payloadId_]and emitRevertingPayloadIdSet.This should be guarded identically to
markRefundEligibleto avoid unauthenticated state flips when no watchers are configured.
🧹 Nitpick comments (7)
contracts/protocol/switchboard/SwitchboardBase.sol (1)
8-16: Centralized_validateSignatureis correct but should be the single source of truthThe new
_validateSignaturecorrectly wraps_recoverSignerand enforces roles, with special-casing for watcher and fee-updater errors. This should be the canonical path for signature checks so child contracts don’t reimplement ECDSA + role logic with different errors; right now both switchboards also have their own batch validators that duplicate this behavior and emitRoleNotAuthorizedinstead ofWatcherNotFound/UnauthorizedFeeUpdater. Those should be refactored to call_validateSignaturein a loop to keep error semantics and maintenance centralized.Also applies to: 52-57, 133-153
test/protocol/switchboard/MessageSwitchboard.t.sol (1)
30-32: Multi-watcher helpers exist but multi-signer flows are only half-testedYou added extra watcher keys/getters and batch-signature helpers, but current tests still exercise
markRefundEligible,setRevertingPayload, andassignTransmitteronly in thetotalWatchers == 1case. Positive-path tests wheretotalWatchers > 1and all watchers’ signatures are required to succeed are missing, and the new helpers are effectively unused.This should get at least one happy-path test per multi-signer function with:
grantWatcherRolecalled for watcher2/watcher3,- signatures from all configured watchers,
- and assertions that calls succeed and state changes (fees eligibility, reverting flag, transmitter digest) occur only when all signatures are present.
That will catch regressions in threshold logic and signature handling that current tests would miss.
Also applies to: 69-77, 381-438
test/protocol/switchboard/EVMxSwitchboard.t.sol (2)
32-36: Multi-watcher test coverage is strong for attest but thin for other multi-signer flowsThe base test setup and helpers for multiple watchers (extra keys,
getWatcher2Address/3Address,addWatchers, batch signature creators) are solid and are used well in thebatchAttestandAttest_ReachesThresholdtests.However, new multi-signer behaviors on
setRevertingPayloadandassignTransmitterare only tested in thetotalWatchers == 1case (plus wrong-signature-count/invalid-watcher/invalid-digest failures). There is no positive-path test where:
addWatchers()is called (sototalWatchers == 3),- all three watchers sign the same message,
- and
setRevertingPayload/assignTransmittersucceed only when all signatures are present.This should be added so the threshold logic for these flows is actually exercised and not just validated via ArrayLengthMismatch/InvalidDigest branches. The existing batch signature helpers can be wired into those tests.
Also applies to: 191-215, 237-276
237-254:_createSetRevertingPayloadSignaturesnaming is misleadingThis helper is called “batch” signatures but currently only returns a single signature. That’s fine for the
totalWatchers == 1tests, but the name suggests multi-watcher usage. Either expand it to actually returntotalWatcherssignatures or rename it to reflect that it’s the single-watcher variant to avoid confusion when more multi-watcher tests are added.contracts/protocol/switchboard/EVMxSwitchboard.sol (1)
321-338: Batch signature validation should reuse_validateSignatureand align errors
_validateBatchSignaturesre-implements the signed-message prefixing, recovery, and role check logic already present in_validateSignature, but always reverts withRoleNotAuthorized(requiredRole_)for missing roles. That diverges from the per-signature path, which returnsWatcherNotFound/UnauthorizedFeeUpdaterfor those roles.This should:
- call
_validateSignature(messageHash_, signatures_[i], requiredRole_)inside the loop, and- drop the local ECDSA call and role check.
That keeps error types consistent across single- and batch-signature flows and avoids code duplication.
contracts/protocol/switchboard/MessageSwitchboard.sol (2)
777-794: Batch signature validation should delegate to_validateSignatureLike the EVMx switchboard,
_validateBatchSignatureshere duplicates signature-prefixing, recovery, and role checking that already exist inSwitchboardBase._validateSignature, but it always throwsRoleNotAuthorized(requiredRole_)for missing roles. Single-signature flows use watcher- and fee-updater-specific errors.This should loop over
signatures_and call_validateSignature(messageHash_, signatures_[i], requiredRole_)instead of rolling its own ECDSA +_hasRolelogic. That will align error types and reduce maintenance duplication.
765-775:_extractSignaturesis currently unused
_extractSignaturesisn’t referenced anywhere in this contract. If you no longer need per-batch slicing, this should be removed to keep the surface small; if you intend to support true multi-batch operations, wire it into those call sites.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
contracts/protocol/SocketBatcher.solcontracts/protocol/switchboard/EVMxSwitchboard.solcontracts/protocol/switchboard/MessageSwitchboard.solcontracts/protocol/switchboard/SwitchboardBase.solcontracts/utils/common/DigestUtils.solcontracts/utils/common/Errors.soltest/protocol/Socket.t.soltest/protocol/switchboard/EVMxSwitchboard.t.soltest/protocol/switchboard/MessageSwitchboard.t.sol
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.t.sol
⚙️ CodeRabbit configuration file
- Tests cover both happy path and failure scenarios. 2) Edge cases tested - zero values, maximum values, boundary conditions. 3) Proper use of setUp() for test initialization. 4) Assertions are specific - use assertEq, assertGt, etc. instead of generic assert. 5) Fuzz tests used appropriately for input validation. 6) Integration tests for cross-contract interactions. 7) Test names clearly describe what behavior is being tested. 8) Mock/stub external dependencies properly. 9) Gas usage tests for gas-sensitive functions.
Files:
test/protocol/Socket.t.soltest/protocol/switchboard/EVMxSwitchboard.t.soltest/protocol/switchboard/MessageSwitchboard.t.sol
🔇 Additional comments (10)
contracts/protocol/SocketBatcher.sol (2)
59-63: This formatting change aligns with readability standards.The multi-line formatting of the attest call improves clarity without altering functionality. Ensure the parameter order (
executionParams_.payloadId,digest_,proof_) matches the current IFastSwitchboard interface signature.
15-17: The IFastSwitchboard interface and SocketBatcher's usage of it are compatible. No evidence of multi-signer refactoring exists in the switchboard implementations—all attest functions remain single-signature across MessageSwitchboard, EVMxSwitchboard, and related interfaces. The call at line 59 matches the interface definition exactly.test/protocol/Socket.t.sol (1)
311-327: WritePrecompile init + Solana digest fixture look consistentInitializer arguments and the hard-coded Solana digest vectors are self-contained and only used in tests. No functional issues here; this should be safe as long as
WritePrecompile.initialize(address,address,uint64,uint64)has not changed its parameter order.Also applies to: 1137-1167
contracts/utils/common/DigestUtils.sol (1)
4-5: Import style-only changeImports were normalized without touching
createDigest. No functional concerns.contracts/utils/common/Errors.sol (1)
172-173: RoleNotAuthorized error is fine but should stay a “generic” fallbackThe new
RoleNotAuthorized(bytes32 role)error is appropriate as a generic fallback. Call sites should keep using more specific errors (WatcherNotFound,UnauthorizedFeeUpdater) where applicable so external behavior remains predictable; the current SwitchboardBase_validateSignaturerespects that, batch helpers are addressed in their files.test/protocol/switchboard/MessageSwitchboard.t.sol (1)
1049-1086: Watcher counter assertions are good; keep usingtotalWatchers()as the single truthThe watcher role management tests correctly assert
totalWatchers()before and aftergrantWatcherRole/revokeWatcherRole. This matches the contract’s reliance ontotalWatchersfor signature thresholds. No issues here.contracts/protocol/switchboard/EVMxSwitchboard.sol (2)
99-146: Attestation refactor to_processAttestationis correct
attestnow builds a chain- and contract-scopedmessageHash, validates the signer via_validateSignaturewithWATCHER_ROLE, and centralizes per-watcher state changes and event emission in_processAttestation. This keeps double-attestation protection and threshold checks in one place and makes the logic easier to audit.
357-373: Watcher count maintenance looks correct
grantWatcherRole/revokeWatcherRoleguard direct watcher role management and keeptotalWatchersin sync with WATCHER_ROLE members, whilegrantRole/revokeRoleexplicitly block WATCHER_ROLE. This is the right shape for usingtotalWatchersas the signature threshold.contracts/protocol/switchboard/MessageSwitchboard.sol (2)
143-150: Attestation refactor is solid;_processAttestationcentralizes correctness
attestnow derives a scopedmessageHash, uses_validateSignaturewithWATCHER_ROLE, then delegates to_processAttestation._processAttestationhandles double-attestation prevention, attestation counting, validity thresholding, and emitsAttestedwith watcher context.batchAttestshares the same core path via_validateBatchSignatures+_processAttestation.This is the right structure for multi-signer attestation.
Also applies to: 171-186
690-707: Watcher role andtotalWatchersbookkeeping is correct
grantWatcherRole/revokeWatcherRolemutations and thegrantRole/revokeRoleoverrides together enforce that WATCHER_ROLE can only be changed through the dedicated helpers, keepingtotalWatchersaligned with the actual watcher set. This is necessary for all thetotalWatchers-based threshold checks above.
| function assignTransmitter(AssignTransmitterParams memory params_) external { | ||
| if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch(); | ||
|
|
||
| DigestParams memory dp = params_.digestParams; | ||
| dp.transmitter = toBytes32Format(params_.oldTransmitter); | ||
| bytes32 oldDigest = createDigest(dp); | ||
| if (payloadIdToDigest[dp.payloadId] != oldDigest) revert InvalidDigest(); | ||
|
|
||
| dp.transmitter = toBytes32Format(params_.newTransmitter); | ||
| bytes32 newDigest = createDigest(dp); | ||
| bytes32 messageHash = keccak256( | ||
| abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest) | ||
| ); | ||
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | ||
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, nonce_); | ||
| for (uint256 k = 0; k < totalWatchers; k++) { | ||
| address watcher = _validateSignature(messageHash, params_.signatures[k], WATCHER_ROLE); | ||
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, params_.nonce); | ||
| } | ||
|
|
||
| payloadIdToDigest[digestParams_.payloadId] = newDigest; | ||
| emit TransmitterAssigned(digestParams_.payloadId, newTransmitter_); | ||
| payloadIdToDigest[dp.payloadId] = newDigest; | ||
| emit TransmitterAssigned(dp.payloadId, params_.newTransmitter); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignTransmitter should also guard against totalWatchers == 0
assignTransmitter has the same pattern as setRevertingPayload: length check then a loop over totalWatchers. When totalWatchers == 0, an empty signatures array will:
- pass the length check,
- skip the loop (no
_validateSignature/_validateAndUseNonce), - and still update
payloadIdToDigestplus emitTransmitterAssigned.
That effectively allows unauthenticated transmitter reassignment in a “no watchers configured” state. This should be guarded the same way:
function assignTransmitter(AssignTransmitterParams memory params_) external {
- if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch();
+ if (totalWatchers == 0) revert WatcherNotFound();
+ if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch();
...
}This keeps the semantics “no watcher set => no multisig operations” instead of “no watcher set => zero-signature operations”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function assignTransmitter(AssignTransmitterParams memory params_) external { | |
| if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch(); | |
| DigestParams memory dp = params_.digestParams; | |
| dp.transmitter = toBytes32Format(params_.oldTransmitter); | |
| bytes32 oldDigest = createDigest(dp); | |
| if (payloadIdToDigest[dp.payloadId] != oldDigest) revert InvalidDigest(); | |
| dp.transmitter = toBytes32Format(params_.newTransmitter); | |
| bytes32 newDigest = createDigest(dp); | |
| bytes32 messageHash = keccak256( | |
| abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest) | |
| ); | |
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | |
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, nonce_); | |
| for (uint256 k = 0; k < totalWatchers; k++) { | |
| address watcher = _validateSignature(messageHash, params_.signatures[k], WATCHER_ROLE); | |
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, params_.nonce); | |
| } | |
| payloadIdToDigest[digestParams_.payloadId] = newDigest; | |
| emit TransmitterAssigned(digestParams_.payloadId, newTransmitter_); | |
| payloadIdToDigest[dp.payloadId] = newDigest; | |
| emit TransmitterAssigned(dp.payloadId, params_.newTransmitter); | |
| } | |
| function assignTransmitter(AssignTransmitterParams memory params_) external { | |
| if (totalWatchers == 0) revert WatcherNotFound(); | |
| if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch(); | |
| DigestParams memory dp = params_.digestParams; | |
| dp.transmitter = toBytes32Format(params_.oldTransmitter); | |
| bytes32 oldDigest = createDigest(dp); | |
| if (payloadIdToDigest[dp.payloadId] != oldDigest) revert InvalidDigest(); | |
| dp.transmitter = toBytes32Format(params_.newTransmitter); | |
| bytes32 newDigest = createDigest(dp); | |
| bytes32 messageHash = keccak256( | |
| abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest) | |
| ); | |
| for (uint256 k = 0; k < totalWatchers; k++) { | |
| address watcher = _validateSignature(messageHash, params_.signatures[k], WATCHER_ROLE); | |
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, params_.nonce); | |
| } | |
| payloadIdToDigest[dp.payloadId] = newDigest; | |
| emit TransmitterAssigned(dp.payloadId, params_.newTransmitter); | |
| } |
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/EVMxSwitchboard.sol around lines 279 to 299,
add a guard that reverts when totalWatchers == 0 before allowing assignment to
prevent unauthenticated zero-signature operations; specifically,
require(totalWatchers > 0) (or revert with an appropriate error) immediately
before/alongside the existing signatures length check so an empty signatures
array cannot pass when no watchers are configured, ensuring the function always
validates signatures/nonces when watchers exist and denies multisig operations
when none are set.
| function markRefundEligible( | ||
| bytes32 payloadId_, | ||
| uint256 nonce_, | ||
| bytes calldata signature_ | ||
| bytes[] calldata signatures_ | ||
| ) external { | ||
| if (signatures_.length != totalWatchers) revert ArrayLengthMismatch(); | ||
|
|
||
| PayloadFees storage fees = payloadFees[payloadId_]; | ||
| if (fees.isRefundEligible) revert AlreadyMarkedRefundEligible(); | ||
| if (fees.nativeFees == 0) revert NoFeesToRefund(); | ||
|
|
||
| bytes32 digest = keccak256( | ||
| abi.encodePacked(toBytes32Format(address(this)), chainSlug, payloadId_, nonce_) | ||
| ); | ||
| address watcher = _recoverSigner(digest, signature_); | ||
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | ||
| _validateAndUseNonce(this.markRefundEligible.selector, watcher, nonce_); | ||
|
|
||
| for (uint256 i = 0; i < totalWatchers; i++) { | ||
| address watcher = _validateSignature(digest, signatures_[i], WATCHER_ROLE); | ||
| _validateAndUseNonce(this.markRefundEligible.selector, watcher, nonce_); | ||
| } | ||
|
|
||
| fees.isRefundEligible = true; | ||
| emit RefundEligibilityMarked(payloadId_, watcher); | ||
| emit RefundEligibilityMarked(payloadId_); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
markRefundEligible should forbid zero-signature calls when totalWatchers == 0
As with the EVMx switchboard, markRefundEligible currently allows:
totalWatchers == 0,signatures_.length == 0,
which passes the length check, skips the validation loop, and still sets fees.isRefundEligible = true and emits RefundEligibilityMarked. That removes watcher authorization from this flow in a misconfigured “no watcher” state.
This should explicitly reject the totalWatchers == 0 configuration:
function markRefundEligible(
bytes32 payloadId_,
uint256 nonce_,
bytes[] calldata signatures_
) external {
- if (signatures_.length != totalWatchers) revert ArrayLengthMismatch();
+ if (totalWatchers == 0) revert WatcherNotFound();
+ if (signatures_.length != totalWatchers) revert ArrayLengthMismatch();
...
}Same comment applies to setRevertingPayload and assignTransmitter below.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 483 to
505, markRefundEligible currently permits calls when totalWatchers == 0 because
signatures_.length == 0 passes the length check and the loop is skipped; add an
explicit check that totalWatchers > 0 (revert with a clear error like
NoWatchersConfigured() or reuse an existing error) before the signatures length
check so zero-watcher configurations cannot mark refunds, then apply the same
explicit totalWatchers > 0 guard to setRevertingPayload and assignTransmitter to
prevent zero-signature bypass in those flows as well.
| function assignTransmitter(AssignTransmitterParams memory params_) external { | ||
| if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch(); | ||
|
|
||
| if (payloadIdToDigest[digestParams_.payloadId] != oldDigest) revert InvalidDigest(); | ||
| DigestParams memory dp = params_.digestParams; | ||
| dp.transmitter = toBytes32Format(params_.oldTransmitter); | ||
|
|
||
| digestParams_.transmitter = toBytes32Format(newTransmitter_); | ||
| bytes32 newDigest = createDigest(digestParams_); | ||
| bytes32 oldDigest = createDigest(dp); | ||
| if (payloadIdToDigest[dp.payloadId] != oldDigest) revert InvalidDigest(); | ||
| dp.transmitter = toBytes32Format(params_.newTransmitter); | ||
|
|
||
| address watcher = _recoverSigner( | ||
| keccak256(abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest)), | ||
| signature_ | ||
| bytes32 newDigest = createDigest(dp); | ||
| bytes32 messageHash = keccak256( | ||
| abi.encodePacked(toBytes32Format(address(this)), chainSlug, oldDigest, newDigest) | ||
| ); | ||
| if (!_hasRole(WATCHER_ROLE, watcher)) revert WatcherNotFound(); | ||
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, nonce_); | ||
|
|
||
| payloadIdToDigest[digestParams_.payloadId] = newDigest; | ||
| emit TransmitterAssigned(digestParams_.payloadId, newTransmitter_); | ||
| for (uint256 k = 0; k < totalWatchers; k++) { | ||
| address watcher = _validateSignature(messageHash, params_.signatures[k], WATCHER_ROLE); | ||
| _validateAndUseNonce(this.assignTransmitter.selector, watcher, params_.nonce); | ||
| } | ||
|
|
||
| payloadIdToDigest[dp.payloadId] = newDigest; | ||
| emit TransmitterAssigned(dp.payloadId, params_.newTransmitter); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assignTransmitter must also guard totalWatchers == 0 before accepting empty signatures
assignTransmitter now requires params_.signatures.length == totalWatchers and validates each signature, but with totalWatchers == 0 an empty signatures array will:
- pass the length check,
- skip signature and nonce validation,
- and still overwrite
payloadIdToDigestand emitTransmitterAssigned.
This should add the same guard as the other multi-signer flows:
function assignTransmitter(AssignTransmitterParams memory params_) external {
- if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch();
+ if (totalWatchers == 0) revert WatcherNotFound();
+ if (params_.signatures.length != totalWatchers) revert ArrayLengthMismatch();
...
}That keeps multi-signer assignments impossible when there are no watchers configured.
🤖 Prompt for AI Agents
In contracts/protocol/switchboard/MessageSwitchboard.sol around lines 731 to
753, add a guard to reject calls when totalWatchers == 0 before
accepting/validating signatures (place it before the params_.signatures.length
check); specifically, detect totalWatchers == 0 and revert with the
same/no-watchers error used by other multi-signer flows (or a new descriptive
revert) so an empty signatures array cannot bypass signature and nonce
validation and overwrite payloadIdToDigest.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.