fix(events): cap per-event hackathon submissions to bound storage growth - #86
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughHackathon submissions now enforce a 5,000-submission per-event cap and a 256-character content URI limit. Persistent submission counts support idempotent resubmissions and slot reclamation after withdrawal, with tests covering the new behaviors. ChangesSubmission limit enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Applicant
participant EventOps
participant Storage
Applicant->>EventOps: submit content_uri
EventOps->>EventOps: validate URI length
EventOps->>Storage: append_submission(event_id, applicant, cap)
Storage-->>EventOps: reserve slot or return cap error
EventOps->>Storage: persist submission
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/events/src/event_ops.rs`:
- Around line 25-26: Replace the global MAX_SUBMISSIONS_PER_EVENT and
MAX_CONTENT_URI_LEN constants with fields on EventRecord or its event variant
payload, and update all submission-count and content-URI validation to read
those per-event values. Ensure event creation/configuration initializes the
fields so each event can set its own limits, preserving the existing defaults
where required.
In `@contracts/events/src/storage.rs`:
- Around line 413-422: Update remove_submission to first check
get_submission(env, id, applicant) and return immediately when no submission
exists; only then remove the EventSubmission key and decrement or remove the
per-event count, preserving the existing count-update behavior for present
submissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05063aad-1b20-43dc-bcfc-3327a6347985
📒 Files selected for processing (5)
contracts/events/src/errors.rscontracts/events/src/event_ops.rscontracts/events/src/storage.rscontracts/events/src/tests/hackathon_pillar.rscontracts/events/src/types.rs
|
@armandocodecr Please fix conflict so i can merge |
6e1f399 to
6c0258c
Compare
Quota reachedYour plan allows 300 CI/CD file units per month. You've used 289 and this scan would add 28 more (total: 317). |
|
@0xdevcollins Done |
…ractmeta (#98) Version stamps had drifted and were internally inconsistent: events contractmeta said 1.2.0 while INITIAL_VERSION said 1.3.0, and neither reflected the public-surface / storage changes merged since (#86 submission cap, #88 manager two-step, #96 OpSeen namespacing). Profile was still 1.1.0 despite #95 namespacing its OpSeen. Bump both contracts coherently — INITIAL_VERSION, contractmeta, and the Cargo package version all set to: events 1.3.0 -> 1.4.0 (submission cap, manager two-step, OpSeen ns) profile 1.1.0 -> 1.2.0 (namespaced OpSeen) version()-asserting admin tests updated to match. 225 events + 66 profile tests green; make build OK (events 56,091 B, profile 15,893 B, both under the 64 KB ceiling); fmt clean.
The submission deadline was enforced by the contract (reject apply/submit/ withdraw_submission after it, require+future-check at create), but it gates nothing the contract is responsible for: - no money path reads it (since #61 the payout/refund liveness is anchored to select-time via PRIZE_CLAIM_WINDOW_SECS, not the deadline); - winners are the manager's discretion — select_winners never consults submissions, so a late submission row is inert; - permissionless-submit storage abuse is bounded by the #86 count cap, not the deadline. Meanwhile the deadline is immutable on-chain (no set/extend entrypoint), so the contract could not support the extensions organizers do routinely. A submission window — with its extensions, grace periods, and cutoffs — is a backend/product concern; the chain keeps only custody, the count cap, and discretionary winner selection. Remove the enforcement: - delete the deadline checks in submit / withdraw_submission / bounty apply and the create-time DeadlineMustBeFuture check; - drop the required-deadline check from hackathon/crowdfunding validate_create. Keep EventRecord.deadline as advisory metadata (still stored, emitted at create, backend-owned) — no storage-layout change, no ABI break. The now-dead DeadlineRequired/DeadlinePassed/DeadlineMustBeFuture variants are retired, freeing 3 slots against the 50-case error-enum cap. Removed the 5 tests that asserted the deleted behavior. 220 events + 66 profile green; make build OK (events 55,742 B); fmt clean.
Summary
Hackathon events have needs_application = false, so any address could call submit() and create a persistent EventSubmission entry with an arbitrarily long content_uri, with no cap on the number of distinct submissions. This let an attacker spam submit() from many fresh addresses to grow the contract's persistent state and rent burden without bound. This adds a per-event submission counter with a cap, and a length bound on content_uri.
Closes the storage bloat issue surfaced by the Almanax scan of e1f1793 (severity medium, boundless-events, event_ops.rs submit).
Closes #71
Changes
Testing
Added to contracts/events/src/tests/hackathon_pillar.rs:
Ran locally:
I did not run cargo clippy as a gate since it isn't part of this repo's CI (verify-build.yml only builds, checks WASM size, and runs cargo test --release), and CONTRIBUTING.md's documented local check is just cargo test.
Summary by CodeRabbit
content_urilength.content_urivalues now fail with a “title too long” error.