Fix security-review findings on #762 (storageProgram broadcast apply)

[hackobi]

Fixes for the security-review findings on #762. This PR targets feature/storage-program-execute, so merging it adds this commit to #762.

Heads-up: this fix reverses #762's behaviour change. The broadcast path now simulates storageProgram edits like every other tx type. The real write happens once, in the block path: applyGCREditsFromMergedMempool during consensus, syncGCRTables during sync. Apart from a comment, endpointHandlers.ts now matches the merge base. If the goal was storage writes visible right after broadcast, that needs its own design: an atomic applied-marker plus a real undo, or serving pending state from the mempool.

#	Severity	Finding	Status
0	critical	Edits applied twice (broadcast write, then consensus/sync)	Fixed
1	high	Resubmitting the same validatedData re-executed before the mempool duplicate check	Fixed: nothing is written before Mempool.addTransaction
2	high	Broadcast write persisted when the tx was never included	Partly fixed. A gap that predates this PR remains: during simulation, HandleGCR.rollback doesn't pass simulate, so a failed multi-edit tx can write for real. Fixing it changes rollback for every edit type, so it's left to maintainers
3	medium	Storage-program state isn't in hashGCRTables	Not fixed: needs an SDK type change and a consensus upgrade
4	medium	Consensus pruned a CREATE on the broadcasting node while peers included it	Fixed as a consequence of 0

Tests: npx jest tests/storageprogram/executeTransaction.test.ts: 4 pass. All 4 fail against #762's head. The type-check errors are the same before and after the change.

🤖 Generated with Claude Code

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[hackobi]

Automated launch review: needs-changes at 0443abf

An automated reviewer checked this PR for the Demos mainnet launch, using public research on how established chains handle the same problem. Each finding cites the code at this head. If a finding is wrong, reply here with the reason.

1. [high] Rollback inside a simulated applyToTx performs real writes, and is called twice

   On a failed edit, applyToTx calls rollback before throwing, then catches its own throw and calls rollback again. rollback calls apply(edit, tx, true), so simulate defaults to false. With simulate=false, balance and nonce routines save (flipping operation in place each time), storage routines write forward, and addAssignedTx persists the tx hash for the sender. At broadcast this means a tx rejected by simulation can still change persisted state. This is outside the PR's diff, but it decides whether broadcast-time handling is free of side effects.

   Code: src/libs/blockchain/gcr/handleGCR.ts:416-439, src/libs/blockchain/gcr/handleGCR.ts:543-569, src/libs/blockchain/gcr/gcr_routines/GCRBalanceRoutines.ts:39-87, src/libs/blockchain/gcr/handleGCR.ts:321-333

2. [high] Storage-program edits have no rollback: isRollback is ignored, so a rollback re-applies the write

   GCRStorageProgramRoutines.apply dispatches only on context.operation and never reads editOperation.isRollback. Every rollback path, including rollbackGCREditsFromTxs after an invalid block and applyToTx's failure rollback, therefore performs the forward storage write again. A storageProgram tx in a block that consensus rejects keeps its write, and may get it twice.

   Code: src/libs/blockchain/gcr/gcr_routines/GCRStorageProgramRoutines.ts:420-460, src/libs/consensus/v2/PoRBFT.ts:363-379

3. [high] Consensus applies edits without per-tx atomicity; sync path differs

   applyGCREditsFromMergedMempool pushes the tx hash once per edit and never undoes the earlier edits of a tx that partly fails, so a hash can land in both successfulTxs and failedTxs. syncGCRTables uses applyToTx, which rolls back. The block is forged from tempMempool, while failed txs are only pruned from the persistent Mempool. The resulting state depends on which path a node took.

   Code: src/libs/consensus/v2/PoRBFT.ts:413-423, src/libs/consensus/v2/PoRBFT.ts:131-141, src/libs/consensus/v2/PoRBFT.ts:186-186, src/libs/blockchain/routines/Sync.ts:724-731

4. [medium] No transaction nonce check anywhere before apply

   assignNonce is a stub that returns true, nonce validation in Transaction.structured is a TODO, and GCRNonceRoutines only increments or decrements. Replay and ordering protection relies only on tx-hash uniqueness in the chain and mempool.

   Code: src/libs/blockchain/routines/validateTransaction.ts:248-253, src/libs/blockchain/transaction.ts:487-493

5. [low] New tests mock HandleGCR and cannot detect real writes

   executeTransaction.test.ts replaces HandleGCR with a stub applyToTx and asserts only the simulate argument. It cannot catch the rollback-with-simulate=false writes or the storage isRollback gap that sit inside HandleGCR.

   Code: tests/storageprogram/executeTransaction.test.ts:32-35