opp: erase an outbound envelope only once the outpost has consumed it (WIRE-348 / CertiK WNS-18) - #592
Conversation
CertiK WNS-18 (WIRE-348) reports that buildenv's cleanup drops every prior outenvelopes row for the destination unconditionally, so a batch operator delayed past the next buildenv would see its envelope deleted before delivery, permanently breaking the outbound chain. The deletion is unconditional as described, but the scenario is unreachable. buildenv runs only as an inline from sysio.epoch::advance, which post-genesis accepts no caller but sysio.msgch; msgch sends advance only from chkcons, and only once every active outpost has outpcons.epoch_index at the current epoch; that row is written only when an inbound envelope from the outpost reaches consensus, and deliver requires the envelope's epoch to equal the current one; and an outpost can emit its epoch-N envelope only after accepting the depot's epoch-N envelope -- Solana asserts it (emit.rs, EmitBeforeEpochAccepted, binding the admin recovery instruction too) and Ethereum reaches emitOutboundEnvelope only from the OPPInbound consensus tip under OPP_FINALIZER_ROLE. So the previous emit is provably consumed before the next buildenv can run, and a stalled relay stalls the epoch rather than losing an envelope. What the code did not do was say any of that. The sweep referenced none of the state it depended on, so the invariant lived entirely in the call path and a later change there could have invalidated it silently. It is now local: the sweep erases a row only when the outpost's consensus record covers that row's epoch_index, and prints a diagnostic when it retains one instead. Absence of an outpcons row means nothing has been acknowledged rather than epoch 0 has been, so an outpost that has never delivered inbound retains every emit. Healthy-path behaviour is unchanged -- acked always covers the prior emit there, the same row is erased, and the table stays one-deep -- so this costs no storage against the pruning added in 0f54317. A retained row is the only surviving copy of an envelope the outpost still needs: envlog keeps a checksum, not a payload, and the source attestations are drained by the same call. Retention makes the tip read ambiguous, so it moves too. It walked byoutpost with lower_bound, which returns the OLDEST row for the outpost because KV secondary entries sort by ascending primary key within one secondary key; that was the tip only because exactly one row survived. It now walks byoutepoch, whose composite packs (chain_code, epoch_index), and takes the last match -- forward-only, the same shape as the sweep below it, one row in the healthy path. Tests cover both halves. buildenv_retains_unacknowledged_outenvelopes drives buildenv without consensus and asserts successive emits accumulate and the tip is still the newest row; buildenv_erases_acknowledged_outenvelope drives real inbound consensus and asserts one acknowledgement drains every emit at or below it, leaving the table one-deep. Three existing readers that selected "the single row" by lowest id were switched to newest-row selection. No ABI change. Change-Id: I6e37aa22ac4d99d7379ffe2de34d1f87f95c7c31
huangminghuang
left a comment
huangminghuang
left a comment
There was a problem hiding this comment.
Two issues found; details inline.
| outpost_consensus_t opcons(get_self()); | ||
| const auto opc_pk = outpost_consensus_key{chain_code}; | ||
| const bool has_ack = opcons.contains(opc_pk); | ||
| const uint32_t acked = has_ack ? opcons.get(opc_pk).epoch_index : 0; |
There was a problem hiding this comment.
High: outpcons.epoch_index is not proof that Ethereum consumed the matching depot envelope. OutpostManager supports explicit OPP_FINALIZER_ROLE grants for recovery callers, while OPP.emitOutboundEnvelope(N) checks only latestOutboundEpoch + 1, not whether OPPInbound accepted WIRE epoch N. A recovery finalizer can therefore emit EVM outbound N before EVM accepts WIRE N; after WIRE consensus sets outpcons=N, this sweep deletes the still-needed WIRE-N payload and the stream wedges. Please add the same accepted-epoch guard Solana has, or bind the acknowledgement to the consumed depot digest.
There was a problem hiding this comment.
Re-reviewed current head b1313d2. This remains blocking: current wire-ethereum origin/next b90035b still checks only latestOutboundEpoch + 1 in OPP.emitOutboundEnvelope, and OPP still has no accepted-inbound-epoch guard. The current sysio change therefore still treats an Ethereum inbound epoch as an acknowledgement it does not prove. I am leaving this thread open and am not approving until that guard lands and is made a deployment prerequisite.
| for (auto it = by_outpost.lower_bound(chain_code); | ||
| it != by_outpost.end() && it->chain_code == chain_code; ) { | ||
| if (it->id == out_id) { ++it; continue; } | ||
| if (!has_ack || it->epoch_index > acked) { |
There was a problem hiding this comment.
Medium: This retains epoch N, but the production relay cannot replay it after the depot advances to N+1. outpost_opp_job always requests the depot's current epoch, and read_pending_outbound exact-matches (chain_code, epoch). It therefore asks for N+1 while the outpost still expects N. The new test fabricates the N+1 inbound acknowledgement rather than exercising this relay path. Please either abort later builds while an unacknowledged predecessor exists, or implement ordered backlog replay/cursor catch-up.
There was a problem hiding this comment.
Re-reviewed after the draft response. Closing the Ethereum gap does not make this retained-row path unreachable: sysio.epoch::advance explicitly accepts sysio.epoch self-authorization after genesis and does not recheck outpcons. A governance/recovery advance can therefore create N+1 without chkcons, while the stock relay still requests only the depot current epoch and never selects retained N. Please either guard/restrict that advance path, add ordered catch-up, or document and test the required manual replay procedure. Leaving this thread open.
|
Converting to draft pending ETH-256. On the High — agreed, and it invalidates the claim this PR's comment makes
There is no sound depot-only substitute:
So the fix is the first option you named: port Solana's guard 0b to It lands standalone, with no wire-sysio change: OPPInbound is the only production caller (wire-sysio's On the MediumFair that the new test exercises retention as state rather than as a recovery mechanism — it can't, because no catch-up exists. The two findings are coupled. Once ETH-256 lands the interlock is sound, so a retained unacknowledged predecessor is unreachable in production and the retained-row path is purely defensive — no relay catch-up needed. If ETH-256 is declined instead, the Medium becomes load-bearing and I'd take the depot-side option, as a skip rather than an abort: a Will un-draft once ETH-256 lands. |
2 participants
Addresses CertiK WNS-18 (Medium, Denial of Service) — tracked as WIRE-348.
The finding, and why the reported impact does not hold
CertiK reports that
buildenvdrops every prioroutenvelopesrow for the destination unconditionally, so a batch operator delayed past the nextbuildenvwould see its envelope deleted before delivery, permanently breaking the outbound chain and stalling the outpost's message stream.The deletion is unconditional as described. The scenario is unreachable, because
buildenvcannot run again until the destination has provably consumed the previous envelope:buildenvruns only as an inline fromsysio.epoch::advance(sysio.epoch.cpp:843-850); no other caller, andrequire_auth(EPOCH_ACCOUNT).advanceaccepts no caller butsysio.msgch(sysio.epoch.cpp:351-354).msgchsendsadvanceonly fromchkcons, and only once every active outpost hasoutpcons.epoch_index == <current epoch>and the boundary has elapsed (sysio.msgch.cpp:1613-1660).deliverrequires the envelope'sepoch_indexto equal the current WIRE epoch (sysio.msgch.cpp:1355).emit.rs,EmitBeforeEpochAccepted, which binds the standalone admin recovery instruction too); Ethereum reachesOPP::emitOutboundEnvelopeonly from theOPPInboundconsensus tip (OPPInbound.sol:958) underOPP_FINALIZER_ROLE, granted only toOPPInbound(OutpostManager.sol:56).A delayed or offline relay therefore stalls the epoch — a loud, monitored condition — rather than losing an envelope.
What changed
The code said none of that. The sweep referenced no state it depended on, so the invariant lived entirely in the call path and a later change there could have invalidated it silently. Two edits in
buildenv, both local:outpcons.epoch_indexcovers that row'sepoch_index, and prints a diagnostic when it retains one. Absence of anoutpconsrow means nothing acknowledged rather than epoch 0 acknowledged, so an outpost that has never delivered inbound retains every emit.byoutepoch. It walkedbyoutpostwithlower_bound, which returns the oldest row (KV secondary entries sort by ascending primary key within one secondary key) — the tip only because exactly one row survived. It now walks the(chain_code, epoch_index)composite and takes the last match: forward-only, the same shape as the sweep below it, one row in the healthy path.Healthy-path behaviour is unchanged —
ackedalways covers the prior emit there, the same row is erased, and the table stays one-deep — so this costs no storage against the pruning added in0f543179fb. A retained row is the only surviving copy of an envelope the outpost still needs:envlogkeeps a checksum, not a payload, and the source attestations are drained by the same call.No ABI change, so no
SysioContractTypesregeneration and no downstream TS rebuild.sysio.msgch.wasmis rebuilt from the source in this commit.Testing
New coverage:
buildenv_retains_unacknowledged_outenvelopes— drivesbuildenvwithout consensus; successive emits accumulate rather than overwrite, and the tip is still the newest row.buildenv_erases_acknowledged_outenvelope— drives real inbound consensus; one acknowledgement drains every emit at or below it, leaving the table one-deep.buildenv_drops_previous_outenvelopespinned the old unconditional behaviour and is replaced by the first of these. Three existing readers that selected "the single row" by lowest id now select the newest.