Skip to content

fix(fsm): guard against empty PaymentPercents slice in HandleCertificateResults#426

Open
amathxbt wants to merge 2 commits into
canopy-network:mainfrom
amathxbt:fix/nil-payment-percent-panic
Open

fix(fsm): guard against empty PaymentPercents slice in HandleCertificateResults#426
amathxbt wants to merge 2 commits into
canopy-network:mainfrom
amathxbt:fix/nil-payment-percent-panic

Conversation

@amathxbt

Copy link
Copy Markdown

Bug

In fsm/automatic.go, HandleCertificateResults() checks that RewardRecipients is non-nil but does not guard against PaymentPercents being an empty slice:

if qc.Results.RewardRecipients == nil {
    return lib.ErrNilRewardRecipients()
}
// No check that PaymentPercents is non-empty ↓
for i, p := range results.RewardRecipients.PaymentPercents {
    results.RewardRecipients.PaymentPercents[i].Percent = ...
}

An empty PaymentPercents slice causes the reward-distribution loop to silently skip all payments — validators complete work but receive no rewards, and the block is accepted without error. A malicious or buggy proposer can craft a CertificateResult with a non-nil RewardRecipients but zero PaymentPercents to silently starve the committee of rewards for that block height.

Fix

Return ErrInvalidPercentAllocation when PaymentPercents is empty after the existing nil guard, making the invariant explicit and ensuring the block is rejected rather than silently misprocessed.

Impact

Critical — malicious proposer can craft blocks that pass validation but deliver zero rewards to the committee.

@andrewnguyen22 andrewnguyen22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This part is real:

  • HandleCertificateResults() only checks RewardRecipients != nil at fsm/automatic.go:152. An empty PaymentPercents slice can pass into
    UpsertCommitteeData() at fsm/automatic.go:224.

But this does not “silently burn all block rewards on every block” immediately. DistributeCommitteeRewards() explicitly skips empty PaymentPercents at fsm/committee.go:142.

The more accurate risk is malformed/empty reward samples can be persisted, which can distort later reward accounting or cause later under-distribution/burn when non-empty samples are eventually distributed.

Comment thread fsm/automatic.go Outdated
return lib.ErrNilRewardRecipients()
}
// guard against an empty PaymentPercents slice causing a silent no-op reward distribution
if len(qc.Results.RewardRecipients.PaymentPercents) == 0 && qc.Results.RewardRecipients != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Use the existing shared validation instead of an ad hoc check:

  if err := qc.Results.RewardRecipients.CheckBasic(); err != nil {
      return err
  }

…heck

The previous commit added a guard:
  if len(...PaymentPercents) == 0 && qc.Results.RewardRecipients != nil

The second condition is dead code — RewardRecipients non-nil is already
enforced two guards above. Simplify to just the length check.

Also update the comment to accurately describe the risk: an empty slice
would be persisted by UpsertCommitteeData and silently skipped by
DistributeCommitteeRewards, causing NumberOfSamples to accumulate without
corresponding recipients and distorting later reward accounting.
@amathxbt

amathxbt commented Jun 27, 2026

Copy link
Copy Markdown
Author

Thanks @andrewnguyen22 you're right that the immediate-burn description was overstated. Updated the branch with:

  • Removed the redundant && qc.Results.RewardRecipients != nil from the guard (RewardRecipients non-nil is already confirmed two lines above, so the second condition was dead code).
  • Rewrote the comment to accurately describe the actual risk: an empty PaymentPercents slice passes the nil guard, gets persisted by UpsertCommitteeData, is silently skipped by DistributeCommitteeRewards (line 142 of committee.go), and causes NumberOfSamples to accumulate without corresponding recipients — distorting later reward accounting rather than causing an immediate burn.

@amathxbt amathxbt requested a review from andrewnguyen22 June 28, 2026 06:03
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.

2 participants