fix(accounts): limit policy enforcement event size - #896
IvanBelyakoff wants to merge 3 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe three reference policies now publish enforcement events with projected contexts instead of complete authorization contexts. Threshold-policy events report signer IDs aligned with the context rule, and all three events place the context rule ID in their topics. New tests check event contents and serialized sizes. ChangesEnforcement event payloads
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The event payload changes have no established new merge-blocking defect. Oversized ExternalRef tags remain a documented limitation, not a regression from this PR. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The smaller events address the reported size problem, but existing event decoders need to change with them. The available evidence does not establish whether downstream consumers are ready. An oversized deployment tag can still produce an oversized event, although that exposure existed before this PR. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the main
✨ Finishing Touches🧪 Generate unit tests (beta)
A rabbit checks the event stream bright Comment |
|
#866 was merged, can you plz change the base of this PR to |
Project authorization contexts to omit caller-controlled arguments, and emit compact signer IDs instead of full signer values. Move the context-rule ID into event topics and cover large payloads, maximum external signers, malformed rule input, and Protocol 28 executable variants.
Replace the bare 16_384 in the ExternalRef tag growth assertion with TX_MAX_CONTRACT_EVENTS_SIZE_BYTES, documented as the testnet txMaxContractEventsSizeBytes measured in OpenZeppelin#852.
8a86219 to
1be218e
Compare
|
@brozorec I've rebased the PR from main. Please review |
Fixes #852
Includes #866, now merged into
main.Policy enforcement events currently copy authorization arguments and full signer data, which can exceed the transaction event-size limit.
This change:
EnforcedContextprojection that keeps the target and function name for calls, or the executable and salt for deployments, while omitting call and constructor arguments.context_rule_idinto topics across all three reference policies.Policy checks and spending-limit argument handling are unchanged. The enforcement event schemas change, so consumers must update their decoders.
Tests cover large arguments, maximum external signer counts, signer-ID mapping, both deployment context variants, and complete event sizes at the ExternalRef ledger-key size boundary. All 196 account tests pass.
One Protocol 28 limitation remains:
ExternalRef.tagis retained unchanged. The host can pass oversized tags to__check_auth, including through unused child authorizations. These can still exceed the event-size limit. This limitation is documented and covered by a serialization regression test; this PR does not introduce a tag-length restriction.PR Checklist
Summary by CodeRabbit