Repository navigation
Conversation
CREATE took storageAddress from the payload as-is and the salt was unused,
so any sender could claim any unused GCR_Main pubkey, including the address
a victim's SDK derives deterministically or a regular account row.
Require every storage operation to target a stor-{40 hex} address, and
require CREATE's address to equal StorageProgram.deriveStorageAddress(sender,
programName, salt) from the SDK.
Adds bun:test unit tests under tests/storageprogram.
validateStorageProgramData (size, nesting depth, key length) ran only in the RPC-side handler, while applyStorageProgramEdit, which every node runs, re-checked size only. Run the full validation on CREATE and WRITE edits when they are applied.
applyStorageProgramEdit ignored editOperation.isRollback, so a consensus rollback (PoRBFT rollbackGCREditsFromTxs -> applyToTx(tx, true)) re-ran CREATE, WRITE, UPDATE_ACCESS_CONTROL and DELETE forward. - CREATE rollback removes the row, but only when that row was created by the same tx (createdTxHash is now recorded in the metadata). - WRITE, UPDATE_ACCESS_CONTROL and DELETE overwrite state that the edit does not carry, so their rollback now fails visibly instead of applying the edit a second time. Full inversion needs recorded prior state, which is a design decision left open.
…gcr_edits handleExecuteTransaction replaced tx.content.gcr_edits with node-generated edits after the hash was fixed. sha256(JSON.stringify(content)) then no longer matched tx.hash, so peers saw the tx as incoherent and Mempool.receive rejected the whole merged batch. The replacement also dropped the client's signed gas and nonce edits (SDK 2.4.24 GCRGeneration emits exactly [balance, nonce] for storageProgram txs), so storage txs paid no fee and did not bump the nonce. - The execute path still validates the payload but no longer touches tx.content. - HandleGCR.getTxEdits derives the storage edit from the signed tx (content.data, content.from, content.timestamp, hash) and appends it after the signed gas and nonce edits. applyToTx and the PoRBFT forward apply both use it, so every node applies the same edits. Client-supplied storageProgram edits are rejected. - created/lastModified come from content.timestamp instead of Date.now(), so the derived edit is identical on every node.
nodeCall is a no-auth RPC method, so getStorageProgram only ever sees an anonymous reader. Add tests that pin the current behaviour: public reads succeed, private reads are refused, and caller-supplied sender or requesterAddress values grant nothing. Authenticated reads need a client protocol change and are left open.
applyToTx rolled back the applied edits twice on a failing edit (once before throwing and again in the catch), and HandleGCR.rollback always applied for real, even when the forward edits were only simulated. Storage edits now run after the gas and nonce edits, so a storage edit that fails on the RPC simulate path (e.g. a WRITE denied by access control) would have credited the sender's balance and decremented the nonce for real, twice. Roll back once, and pass simulate through to the rollback.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. Comment |
|
Automated launch review: needs-changes at 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 participant
Fixes for the security-review findings on #477 (Storage Programs). This PR targets the
storagebranch, so merging it adds these commits to #477.gcr_editsafter hashing, so the tx became incoherent and broke mempool mergesHandleGCR.getTxEdits), andtx.contentis never modified3bd030d6,4da60ed93bd030d6isRollback, so a rollback re-applied them21b9c38esaltwas unused (squatting/front-running)deriveStorageAddress(sender, programName, salt)and every op requires thestor-format2021cc92getStorageProgramcan never authenticate a reader6644f9fe5411d825tests/storageprogram/4da60ed9also fixesapplyToTxrolling back twice, and rolling back for real during a simulation.Still open (author decision)
@kynesyslabs/demosdkis^2.4.24with no lockfile; a fresh install resolves 2.12.2, which breaks this code path. Please pin the SDK or commit a lockfile.testnet.Tests:
bun test tests/storageprogram: 24 pass.🤖 Generated with Claude Code