security(evm-solana): add on-chain M-of-N operator withdraw approve #176

New issue

Open

opened 2026-09-12 11:44:18 +00:00 by PlasticDigits · 0 comments

PlasticDigits commented

2026-09-12 11:44:18 +00:00

Owner

Summary

EVM withdrawApprove and Solana withdraw_approve are 1-of-1. Any single authorized operator (EVM: member of _operators or owner(); Solana: bridge.operator) can mark a pending withdrawal approved and start the cancel window. There is no on-chain deposit proof and no M-of-N vote. The off-chain operator service signs with one EVM_PRIVATE_KEY / SOLANA_PRIVATE_KEY.

This is not #175 (Terra CosmWasm WithdrawApprove ignores min_signatures — sibling, keep separate). This is not #135 (wasm / config.admin handoff to the DEX 2-of-3). Closed #115 is RPC read-consensus, not operator signing. Closed #68 is Solana QA hardening (fees, PDA seeds), not quorum.

Bundle (same ticket, do not split): on-chain M-of-N operator approvals on EVM and Solana, or at minimum N-of-M signatures in the operator service before broadcast, with tests that a single operator key cannot complete approval when N≥2. Terra stays on #175.

EVM: set exists, threshold does not

Bridge.withdrawApprove (onlyOperator + nonReentrant) loads pendingWithdraws[xchainHashId], then:

reverts if not submitted / already executed / already approved
rejects a reused (srcChain, nonce) via withdrawNonceUsed and immediately sets it used
sets approved = true, approvedAt = block.timestamp
pays operatorGas to msg.sender

No deposit lookup. No vote count. _operators is already an EnumerableSet (addOperator / removeOperator / getOperators), but _onlyOperator treats any member or owner() as sufficient. PendingWithdraw stores a boolean approved / approvedAt only.

test_WithdrawApprove in packages/contracts-evm/test/Bridge.t.sol is a single vm.prank(operator). Enumeration tests assume getOperatorCount() == 1. There is no N≥2 approve fixture.

Solana: one pubkey

withdraw_approve::handler requires operator.key() == bridge.operator, then sets approved / approved_at and inits the NonceUsed PDA. BridgeConfig has a single operator: Pubkey. PendingWithdraw has no approver collection.

Existing tests (non-operator cannot approve, cannot approve twice, pause) do not cover a threshold. Layout change for an operator set + min_signatures is in scope (Anchor InitSpace / migrate), not a follow-up.

Operator service is also 1-of-1

EvmWriter::submit_withdraw_approve sends withdrawApprove from one PrivateKeySigner (EVM_PRIVATE_KEY). The Solana writer uses one SOLANA_PRIVATE_KEY. Deposit existence is checked off-chain before broadcast; a holder of the hot key skips that path. Circuit-breaker threshold in writers/mod.rs is consecutive RPC failure count, not signing quorum.

docs/security-model.md currently treats a compromised operator as a canceler-window problem (default 5 minutes). That is the only on-chain brake today.

Impact (Immunefi-style)

Question	Answer
Funds at risk today?	No permissionless user drain. A non-operator cannot approve. Control-gap / single hot key: one compromised operator (or EVM owner) can approve arbitrary pending user withdraws (mint or unlock after the cancel window unless a canceler fires).
Auth / admin required?	EVM: `_operators.contains` or `owner()`. Solana: exact `bridge.operator`.
Theft vs lock?	Compromised single operator key can approve pending withdraws as if the set had signed. Canceler + delay still apply after that one approval. Not a lock bug.
Sticky once armed?	Until EVM proxy / Solana program is upgraded (and operator writers learn N txs or aggregated sigs). Adding extra EVM operators without a threshold increases the number of keys that can solo-approve.
Scope	EVM `Bridge.withdrawApprove` + Solana `withdraw_approve` + operator writers + approve tests + security-model docs. Not Terra CosmWasm (#175). Not canceler RPC. Not frontend UX.

Invariant that is broken: operator approval is M-of-N (or at least N distinct hot keys before broadcast). Today M is silently 1.

Preferred fix is on-chain quorum on both chains. Off-chain N-of-M in the operator before broadcast is an allowed interim only if keys are not co-resident in one process; it does not replace the on-chain ticket.

Constraints / guardrails

Do not remove canceler, cancelWindow / withdraw_delay, pause, rate limits, or (srcChain, nonce) replay. Execute still requires approved after the window.
Same operator voting twice must not count twice. Unauthorized callers still revert (Unauthorized / UnauthorizedOperator).
Do not start the cancel window, set approved = true, or pay operatorGas / lamports until the threshold is met. Document when withdrawNonceUsed / NonceUsed PDA is created: first distinct vote vs threshold. Prefer not paying the tip or starting approvedAt on a partial vote.
EVM owner: default is one distinct vote, not a solo override of M. If product wants owner override, that must be a documented exception with tests.
EVM storage: use __gap / new mapping keyed by hash for votes; do not break existing pending rows. Solana: account resize / migrate must keep live pending PDAs loadable.
Operator writers / E2E currently assume one withdrawApprove flips approved. After this, N>1 deployments need N distinct txs (or one tx carrying M signatures). Out of scope to redesign cancelers, but tests/docs must not claim one key meets N≥2.
Founder-required contracts / keys / wallet 2-of-3. No community autoland. Do not add ready. No public mainnet approve recipe. Do not publish a working single-key mint/unlock sequence.

Relevant files

Path	Why
`packages/contracts-evm/src/Bridge.sol`	`withdrawApprove` 1-of-1; `_onlyOperator` includes owner
`packages/contracts-evm/src/interfaces/IBridge.sol`	`PendingWithdraw` has no vote set / `minSignatures`
`packages/contracts-evm/test/Bridge.t.sol`	Single-operator approve happy path; add N≥2
`packages/contracts-solana/programs/cl8y-bridge/src/instructions/withdraw_approve.rs`	Single `bridge.operator` check
`packages/contracts-solana/programs/cl8y-bridge/src/state/bridge.rs`	One `operator: Pubkey`
`packages/contracts-solana/programs/cl8y-bridge/src/state/pending_withdraw.rs`	Boolean `approved` only
`packages/contracts-solana/tests/deposit_withdraw.test.ts`	`non-operator cannot approve`; add threshold
`packages/operator/src/writers/evm.rs`	One signer `withdrawApprove`
`packages/operator/src/writers/solana.rs`	One keypair `withdraw_approve`
`packages/operator/src/config.rs`	Single `EVM_PRIVATE_KEY` / `SOLANA_PRIVATE_KEY`
`docs/security-model.md`	Watchtower assumes cancelers are the only brake on a bad approve

Recommended direction

EVM (preferred): add minSignatures (constructor / owner setter, 0 illegal, <= getOperatorCount()). Store distinct approvers per xchainHashId. First votes persist without approved=true. When count >= minSignatures, keep today’s terminal-approve behavior (window, nonce policy as documented, tip). Owner is one voter unless a written exception exists.
Solana (preferred): replace or extend bridge.operator with an operator set + min_signatures (PDA list or remaining accounts). Same vote-then-threshold semantics as EVM. Migrate BridgeConfig / pending accounts without invalidating in-flight withdraw PDAs.
Operator writers: for N>1, either submit N on-chain votes from distinct keys, or (interim) require N-of-M signatures over the hash in separate signing environments before any broadcast. One process holding every key is not quorum.
Tests: two operators, M=2 → first vote leaves approved == false; second distinct voter flips it; same key twice does not; M=1 still matches today’s 1-of-1 so current deploys do not stall.
Update docs/security-model.md (and operator docs) so “compromised operator” is not only a canceler race when M>1 is configured.

Do not implement Hyperlane MultisigValidationModule from HYPERLANE_FUTURE_SUPPORT.md in this ticket.

Acceptance criteria

AC1. With M=2 and two registered EVM operators, a single operator (or owner acting as one voter) cannot set approved or approvedAt > 0. Pending row stays unapproved and still pending.
AC2. Two distinct authorized EVM voters meet the threshold: approved == true, cancel window starts once, execute remains blocked until delay.
AC3. Same EVM address voting twice does not meet M=2. Unauthorized caller still reverts.
AC4. Solana: with M=2 and two operators, a single bridge.operator (or leftover 1-key config after upgrade) cannot complete approval. Two distinct operator signers can. Unauthorized still UnauthorizedOperator.
AC5. M=1 (today’s tests/deploy default) still succeeds on the first authorized approve so 1-of-1 deployments do not stall.
AC6. Operator writer (or documented interim) cannot broadcast dest approve from one key when configured N≥2. Focused Forge + Anchor tests pass.
AC7. Docs no longer claim watchtower-only is the operator-key story once on-chain M>1 exists.

Verification (non-exploitative)

Do not publish a mainnet single-key approve + execute sequence. Local / Anvil / surfpool / Anchor test only:

EVM Forge: two operators, minSignatures = 2, user withdrawSubmit. First withdrawApprove → approved == false; funds still locked; nonce policy matches the written choice.
Second distinct operator → approved == true; further approve reverts; execute still waits for cancelWindow.
Repeat with owner() as the first voter only: still not approved until a second distinct voter (unless a documented owner-override exception exists — then test that exception instead of AC1).
Solana Anchor: same matrix on withdraw_approve.
Regression: existing 1-of-1 withdraw-flow tests stay green (approve, cancel, uncancel, execute after delay). Unauthorized still fails.
If interim off-chain N-of-M ships first: unit-test the aggregator rejects a single signature; do not require a live dest-chain mint to prove it.

First-pass model recommendation

Recommendation: grok-high

Rationale: Security + founder-required contracts, operator keys, and wallet / 2-of-3 policy. Composer is disallowed (security class; contracts/auth/keys; more than three production files; two chains plus operator writers; Solana account layout). Wrong nonce or tip timing can stall withdrawals or pay a tip before the set has signed. Verify with Forge + Anchor threshold tests, not a live approve.

## Summary EVM `withdrawApprove` and Solana `withdraw_approve` are 1-of-1. Any single authorized operator (EVM: member of `_operators` **or** `owner()`; Solana: `bridge.operator`) can mark a pending withdrawal approved and start the cancel window. There is no on-chain deposit proof and no M-of-N vote. The off-chain operator service signs with one `EVM_PRIVATE_KEY` / `SOLANA_PRIVATE_KEY`. This is **not** [#175](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/175) (Terra CosmWasm `WithdrawApprove` ignores `min_signatures` — sibling, keep separate). This is **not** [#135](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/135) (wasm / `config.admin` handoff to the DEX 2-of-3). Closed [#115](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/115) is RPC read-consensus, not operator signing. Closed [#68](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/68) is Solana QA hardening (fees, PDA seeds), not quorum. Bundle (same ticket, do not split): on-chain M-of-N operator approvals on **EVM and Solana**, or at minimum N-of-M signatures in the operator service **before** broadcast, with tests that a single operator key cannot complete approval when N≥2. Terra stays on #175. ### EVM: set exists, threshold does not [`Bridge.withdrawApprove`](packages/contracts-evm/src/Bridge.sol) (`onlyOperator` + `nonReentrant`) loads `pendingWithdraws[xchainHashId]`, then: - reverts if not submitted / already executed / already approved - rejects a reused `(srcChain, nonce)` via `withdrawNonceUsed` and immediately sets it used - sets `approved = true`, `approvedAt = block.timestamp` - pays `operatorGas` to `msg.sender` No deposit lookup. No vote count. `_operators` is already an `EnumerableSet` (`addOperator` / `removeOperator` / `getOperators`), but `_onlyOperator` treats **any** member **or** `owner()` as sufficient. [`PendingWithdraw`](packages/contracts-evm/src/interfaces/IBridge.sol) stores a boolean `approved` / `approvedAt` only. `test_WithdrawApprove` in `packages/contracts-evm/test/Bridge.t.sol` is a single `vm.prank(operator)`. Enumeration tests assume `getOperatorCount() == 1`. There is no N≥2 approve fixture. ### Solana: one pubkey [`withdraw_approve::handler`](packages/contracts-solana/programs/cl8y-bridge/src/instructions/withdraw_approve.rs) requires `operator.key() == bridge.operator`, then sets `approved` / `approved_at` and inits the `NonceUsed` PDA. [`BridgeConfig`](packages/contracts-solana/programs/cl8y-bridge/src/state/bridge.rs) has a **single** `operator: Pubkey`. [`PendingWithdraw`](packages/contracts-solana/programs/cl8y-bridge/src/state/pending_withdraw.rs) has no approver collection. Existing tests (`non-operator cannot approve`, `cannot approve twice`, pause) do not cover a threshold. Layout change for an operator set + `min_signatures` is in scope (Anchor `InitSpace` / migrate), not a follow-up. ### Operator service is also 1-of-1 [`EvmWriter::submit_withdraw_approve`](packages/operator/src/writers/evm.rs) sends `withdrawApprove` from one `PrivateKeySigner` (`EVM_PRIVATE_KEY`). The Solana writer uses one `SOLANA_PRIVATE_KEY`. Deposit existence is checked **off-chain** before broadcast; a holder of the hot key skips that path. Circuit-breaker `threshold` in `writers/mod.rs` is consecutive RPC failure count, not signing quorum. `docs/security-model.md` currently treats a compromised operator as a canceler-window problem (default 5 minutes). That is the only on-chain brake today. ## Impact (Immunefi-style) | Question | Answer | | --- | --- | | Funds at risk today? | No permissionless user drain. A non-operator cannot approve. **Control-gap / single hot key:** one compromised operator (or EVM owner) can approve arbitrary pending user withdraws (mint or unlock after the cancel window unless a canceler fires). | | Auth / admin required? | EVM: `_operators.contains` **or** `owner()`. Solana: exact `bridge.operator`. | | Theft vs lock? | Compromised single operator key can approve pending withdraws as if the set had signed. Canceler + delay still apply after that one approval. Not a lock bug. | | Sticky once armed? | Until EVM proxy / Solana program is upgraded (and operator writers learn N txs or aggregated sigs). Adding extra EVM operators without a threshold **increases** the number of keys that can solo-approve. | | Scope | EVM `Bridge.withdrawApprove` + Solana `withdraw_approve` + operator writers + approve tests + security-model docs. Not Terra CosmWasm (#175). Not canceler RPC. Not frontend UX. | Invariant that is broken: operator approval is M-of-N (or at least N distinct hot keys before broadcast). Today M is silently 1. Preferred fix is **on-chain** quorum on both chains. Off-chain N-of-M in the operator **before broadcast** is an allowed interim only if keys are not co-resident in one process; it does not replace the on-chain ticket. ## Constraints / guardrails - Do not remove canceler, `cancelWindow` / `withdraw_delay`, pause, rate limits, or `(srcChain, nonce)` replay. Execute still requires `approved` after the window. - Same operator voting twice must not count twice. Unauthorized callers still revert (`Unauthorized` / `UnauthorizedOperator`). - Do not start the cancel window, set `approved = true`, or pay `operatorGas` / lamports until the threshold is met. Document when `withdrawNonceUsed` / `NonceUsed` PDA is created: first distinct vote vs threshold. Prefer not paying the tip or starting `approvedAt` on a partial vote. - EVM owner: default is **one distinct vote**, not a solo override of M. If product wants owner override, that must be a documented exception with tests. - EVM storage: use `__gap` / new mapping keyed by hash for votes; do not break existing pending rows. Solana: account resize / migrate must keep live pending PDAs loadable. - Operator writers / E2E currently assume one `withdrawApprove` flips `approved`. After this, N>1 deployments need N distinct txs (or one tx carrying M signatures). Out of scope to redesign cancelers, but tests/docs must not claim one key meets N≥2. - Founder-required contracts / keys / wallet 2-of-3. No community autoland. Do not add `ready`. No public mainnet approve recipe. Do not publish a working single-key mint/unlock sequence. ## Relevant files | Path | Why | | --- | --- | | `packages/contracts-evm/src/Bridge.sol` | `withdrawApprove` 1-of-1; `_onlyOperator` includes owner | | `packages/contracts-evm/src/interfaces/IBridge.sol` | `PendingWithdraw` has no vote set / `minSignatures` | | `packages/contracts-evm/test/Bridge.t.sol` | Single-operator approve happy path; add N≥2 | | `packages/contracts-solana/programs/cl8y-bridge/src/instructions/withdraw_approve.rs` | Single `bridge.operator` check | | `packages/contracts-solana/programs/cl8y-bridge/src/state/bridge.rs` | One `operator: Pubkey` | | `packages/contracts-solana/programs/cl8y-bridge/src/state/pending_withdraw.rs` | Boolean `approved` only | | `packages/contracts-solana/tests/deposit_withdraw.test.ts` | `non-operator cannot approve`; add threshold | | `packages/operator/src/writers/evm.rs` | One signer `withdrawApprove` | | `packages/operator/src/writers/solana.rs` | One keypair `withdraw_approve` | | `packages/operator/src/config.rs` | Single `EVM_PRIVATE_KEY` / `SOLANA_PRIVATE_KEY` | | `docs/security-model.md` | Watchtower assumes cancelers are the only brake on a bad approve | ## Recommended direction 1. **EVM (preferred):** add `minSignatures` (constructor / owner setter, `0` illegal, `<= getOperatorCount()`). Store distinct approvers per `xchainHashId`. First votes persist without `approved=true`. When `count >= minSignatures`, keep today’s terminal-approve behavior (window, nonce policy as documented, tip). Owner is one voter unless a written exception exists. 2. **Solana (preferred):** replace or extend `bridge.operator` with an operator set + `min_signatures` (PDA list or remaining accounts). Same vote-then-threshold semantics as EVM. Migrate `BridgeConfig` / pending accounts without invalidating in-flight withdraw PDAs. 3. **Operator writers:** for N>1, either submit N on-chain votes from distinct keys, or (interim) require N-of-M signatures over the hash **in separate signing environments** before any broadcast. One process holding every key is not quorum. 4. Tests: two operators, M=2 → first vote leaves `approved == false`; second distinct voter flips it; same key twice does not; M=1 still matches today’s 1-of-1 so current deploys do not stall. 5. Update `docs/security-model.md` (and operator docs) so “compromised operator” is not only a canceler race when M>1 is configured. Do not implement Hyperlane `MultisigValidationModule` from `HYPERLANE_FUTURE_SUPPORT.md` in this ticket. ## Acceptance criteria - AC1. With M=2 and two registered EVM operators, a single operator (or owner acting as one voter) cannot set `approved` or `approvedAt > 0`. Pending row stays unapproved and still pending. - AC2. Two distinct authorized EVM voters meet the threshold: `approved == true`, cancel window starts once, execute remains blocked until delay. - AC3. Same EVM address voting twice does not meet M=2. Unauthorized caller still reverts. - AC4. Solana: with M=2 and two operators, a single `bridge.operator` (or leftover 1-key config after upgrade) cannot complete approval. Two distinct operator signers can. Unauthorized still `UnauthorizedOperator`. - AC5. M=1 (today’s tests/deploy default) still succeeds on the first authorized approve so 1-of-1 deployments do not stall. - AC6. Operator writer (or documented interim) cannot broadcast dest approve from one key when configured N≥2. Focused Forge + Anchor tests pass. - AC7. Docs no longer claim watchtower-only is the operator-key story once on-chain M>1 exists. ## Verification (non-exploitative) Do not publish a mainnet single-key approve + execute sequence. Local / Anvil / surfpool / Anchor test only: 1. EVM Forge: two operators, `minSignatures = 2`, user `withdrawSubmit`. First `withdrawApprove` → `approved == false`; funds still locked; nonce policy matches the written choice. 2. Second distinct operator → `approved == true`; further approve reverts; execute still waits for `cancelWindow`. 3. Repeat with `owner()` as the first voter only: still not approved until a second distinct voter (unless a documented owner-override exception exists — then test that exception instead of AC1). 4. Solana Anchor: same matrix on `withdraw_approve`. 5. Regression: existing 1-of-1 withdraw-flow tests stay green (approve, cancel, uncancel, execute after delay). Unauthorized still fails. 6. If interim off-chain N-of-M ships first: unit-test the aggregator rejects a single signature; do not require a live dest-chain mint to prove it. ## First-pass model recommendation Recommendation: grok-high Rationale: Security + founder-required contracts, operator keys, and wallet / 2-of-3 policy. Composer is disallowed (security class; contracts/auth/keys; more than three production files; two chains plus operator writers; Solana account layout). Wrong nonce or tip timing can stall withdrawals or pay a tip before the set has signed. Verify with Forge + Anchor threshold tests, not a live approve.

PlasticDigits added the

high-risk

security

labels

2026-09-12 11:44:20 +00:00

PlasticDigits referenced this issue

2026-09-12 11:49:25 +00:00

security(bridge): timelock cancel-window changes and raise 15s floor #177

PlasticDigits referenced this issue

2026-09-12 11:53:17 +00:00

security(evm): fail closed when guard and rate-limit bridge are unset #178

PlasticDigits referenced this issue