Skip to content

Fix #57: emit fee provenance in withdraw events - #187

Open
praizeD10 wants to merge 1 commit into
SPulse-Org:mainfrom
praizeD10:fix/issue-57-fee-provenance
Open

Fix #57: emit fee provenance in withdraw events#187
praizeD10 wants to merge 1 commit into
SPulse-Org:mainfrom
praizeD10:fix/issue-57-fee-provenance

Conversation

@praizeD10

Copy link
Copy Markdown

closes #172

Track which markets contribute to each fee withdrawal so indexers can detect cross-market contamination. The fees_withdrawn event now includes a Vec with (market_id, amount) pairs in drain order (legacy first, then newest-to-oldest).

Changes:

  • Add FeeProvenance struct (market_id, amount)
  • debit_proven_fees now returns the provenance it debits
  • Add preview_fee_provenance helper for non-mutating reads
  • Add get_withdrawal_provenance view function for integrators
  • Update withdraw_fees and execute_withdraw_fees events to include provenance
  • Update event schema comment to document new format

Existing safeguards (20% cap, 24h timelock, per-market ledger, admin cancellation) remain unchanged.

Track which markets contribute to each fee withdrawal so indexers can
detect cross-market contamination. The fees_withdrawn event now includes
a Vec<FeeProvenance> with (market_id, amount) pairs in drain order
(legacy first, then newest-to-oldest).

Changes:
- Add FeeProvenance struct (market_id, amount)
- debit_proven_fees now returns the provenance it debits
- Add preview_fee_provenance helper for non-mutating reads
- Add get_withdrawal_provenance view function for integrators
- Update withdraw_fees and execute_withdraw_fees events to include provenance
- Update event schema comment to document new format

Existing safeguards (20% cap, 24h timelock, per-market ledger, admin
cancellation) remain unchanged.

@Muyideen-js Muyideen-js left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@praizeD10 The PR adds fee provenance tracking but does not address the critical security requirements in the issue. The issue explicitly requires a per-transaction cap, per-market segregation, and a timelock. The diff only modifies event emission and adds a preview function; the withdrawal logic still allows a recipient to drain the entire accumulator in one call, and there is no restriction on which markets' fees can be withdrawn. Please implement the missing safeguards: add a cap (e.g., MAX_WITHDRAWAL_BPS already exists but is not enforced as a per-call cap? Actually it is used, but the issue asks for a percentage cap with timelock; ensure the cap is applied and add a timelock between request and execute). Also, enforce that a recipient can only withdraw fees from markets they are authorized for. Add tests for the new provenance and the security fixes. CI is missing, so please run tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CRITICAL] withdraw_fees lets fee recipient drain entire accumulator — no cap, no provenance

2 participants