Skip to content

fix(dao-rewards-distributor): apply precision scale before flooring emission (audit Critical 2) - #934

Open
noahsaso wants to merge 3 commits into
developmentfrom
fix-reward-emission-rounding
Open

noahsaso wants to merge 3 commits into
developmentfrom
fix-reward-emission-rounding

Conversation

@noahsaso

@noahsaso noahsaso commented Sep 1, 2026

Copy link
Copy Markdown
Member

Fixes audit Finding 2 (Critical): "A distribution whose emission is smaller than its duration destroys the emission". This is the surviving failure mode of Oak Security's November 2024 finding 4.

Root cause

get_active_total_earned_puvp computed the rewards emitted since the last update as

Uint256::from(amount)
    .checked_mul_floor(complete_distribution_periods)?
    .checked_mul(scale_factor())?

i.e. it floored to whole tokens before applying the 10^39 precision scale factor. Any update covering less than one full emission period (for example an amount of 100 over 200 blocks, updated every block by ordinary membership or staking changes) contributed zero, and because the accumulator's clock is advanced unconditionally afterwards, that interval could never contribute again. The emission was destroyed, while UndistributedRewards still reported it as distributed.

Fix

Apply the scale factor before the floor, so truncation happens at the precision the accumulator actually carries:

Uint256::from(amount)
    .checked_mul(scale_factor())?
    .checked_mul_floor(complete_distribution_periods)?

The total credited over any sequence of updates is now within 10^-39 of amount * elapsed / duration regardless of update frequency. Claims remain bounded by what get_rewards_until reports as distributed (whole-token floor of the same quantity), so Withdraw and UndistributedRewards cannot pay out more than was credited.

Tests

test_small_linear_emission_survives_incremental_accumulator_updates reproduces the audit scenario: 100 tokens over 200 blocks on a cw4 DAO, with the accumulator advanced once per block by membership changes that keep total voting power constant. It asserts the exact accumulator value after the first block and after 200 blocks, and that a member holding half the voting power can claim 50 tokens. It fails on development (accumulator stays at 0) and passes with the fix.

Verified locally: cargo test -p dao-rewards-distributor (67 passed), cargo +nightly-2024-01-08 fmt --all -- --check, cargo +nightly-2024-01-08 clippy --all-targets -- -D warnings.

Not included (audit follow-ups)

  • The audit also suggests optionally rejecting configurations whose emission amount is smaller than its duration. With this fix such configurations work correctly, so no validation is added.
  • Finding 12 (High), an interval with no voting power being charged in full, is a separate issue and is not addressed here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YBSNZfoCq2XqymbVV9r5rE

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.11%. Comparing base (0178cf5) to head (d1e04a4).

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #934      +/-   ##
===============================================
- Coverage        96.56%   92.11%   -4.46%     
===============================================
  Files              199      159      -40     
  Lines            67407    28774   -38633     
===============================================
- Hits             65094    26506   -38588     
+ Misses            2313     2268      -45     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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