Skip to content

refactor(dba-pool): move eligibility filtering on-chain (V4)#3418

Open
Agilulfo1820 wants to merge 5 commits into
mainfrom
feat/dba-pool-on-chain-filtering
Open

refactor(dba-pool): move eligibility filtering on-chain (V4)#3418
Agilulfo1820 wants to merge 5 commits into
mainfrom
feat/dba-pool-on-chain-filtering

Conversation

@Agilulfo1820

Copy link
Copy Markdown
Member

Summary

  • DBAPool V4: distributeDBARewards(uint256) derives the eligible set on-chain (no array argument)
  • Replicates the 3 off-chain rules: round membership + ≥1 rewarded action + endorsement at start OR end
  • Reads VeBetterPassport.appRoundActionCount and X2EarnApps.isEligible (already checkpointed)
  • Old array-form removed; off-chain filterEligibleAppsForDBA deleted from lambda helpers
  • New eligibleAppsForRound(uint256) view for monitoring

Key files

  • packages/contracts/contracts/DBAPool.sol (V4 upgrade — storage append, reinitializer(4))
  • packages/contracts/contracts/deprecated/V3/ (V3 snapshot for upgrade tests)
  • packages/contracts/scripts/upgrade/upgrades/dba-pool/dba-pool-v4.ts
  • packages/contracts/test/dba-pool/v4-{upgrade,compatibility,scalability}.test.ts
  • packages/lambda/src/distributeWeeklyAppRewards/lambda.ts + distributeDBA/lambda.ts

Test plan

  • DBAPool V4 contract tests + 3 new shards pass on Hardhat network (8 V4 tests + 75 legacy tests = 83 total)
  • 10-app scalability test reports ~1.43M gas (linear ⇒ ~14M @ 100 apps; under 30M assertion)
  • LAMBDA_ENV=testnet-staging yarn build:lambda + LAMBDA_ENV=mainnet yarn build:lambda clean; bundles contain no filterEligibleAppsForDBA
  • Deploy to testnet-staging via yarn contracts:upgrade:testnet-staging → DBA Pool V4; verify version()=='4' + wired contract refs
  • At next testnet-staging cycle, observe distribute-weekly-app-rewards Slack confirms On-chain eligible set has N app(s) log + successful distribution

Notes

Agilulfo1820 and others added 2 commits June 8, 2026 11:54
…uteWeeklyAppRewards

Isolate emissions start from app rewards distribution so failures in one
step don't block the other and each is independently observable.

- startEmissionsRound: only Emissions.distribute() (same schedule as before)
- distributeWeeklyAppRewards: XAllocationPool.claim() + DBAPool.distributeDBARewards(),
  scheduled 5 min after startEmissionsRound
- standalone distributeDBA lambda left untouched (manual fallback)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The off-chain `filterEligibleAppsForDBA` lambda helper is replaced by an
on-chain derivation inside `DBAPool.distributeDBARewards(uint256)` using
`VeBetterPassport.appRoundActionCount` and `X2EarnApps.isEligible` (already
checkpointed). The array-form distribute is removed; the lambda calls the
no-arg variant.

Contracts:
- DBAPool V4: new storage refs (veBetterPassport + xAllocationVoting),
  `initializeV4` reinitializer, new `distributeDBARewards(uint256)` that
  applies rules (1) round membership, (2) >=1 rewarded action,
  (3) NOT unendorsed at both round-start AND round-end. Adds
  `eligibleAppsForRound(uint256)` view for off-chain monitoring.
- V3 snapshot kept under `deprecated/V3/` for upgrade tests.

Tests: existing DBAPool.test.ts updated to V4 signature with an inline
action-registration helper. New shards: V4 Upgrade, V4 Compatibility,
V4 Scalability (10-app gas budget; 50/100 skipped due to Mjolnir node-cap
in the StarGate mock).

Lambda: distributeWeeklyAppRewards + distributeDBA now call the no-arg
variant and read `eligibleAppsForRound` for logging only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Agilulfo1820 Agilulfo1820 requested review from a team and davidecarpini as code owners June 8, 2026 10:56
@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 04040762-0c69-43b7-87f9-fb53db53cf33

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dba-pool-on-chain-filtering

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@davidecarpini davidecarpini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Severity: HIGH

File: packages/contracts/contracts/DBAPool.sol:274-277 (_buildEligibleApps, "rule 3" endorsement check)

Bug: on-chain rule 3 is dead code and silently changes who receives DBA rewards

V4 replaces the off-chain endorsement rule — which read isAppUnendorsed(appId) at the round's start/end blocks — with:

bool eligibleAtStart = $.x2EarnApps.isEligible(appId, snapshot);
bool eligibleAtEnd   = $.x2EarnApps.isEligible(appId, deadline);
if (!eligibleAtStart && !eligibleAtEnd) {
  continue; // exclude
}

Two compounding problems, both verified against the contracts:

  1. isEligible is not the inverse of isAppUnendorsed. During the endorsement grace period, EndorsementUtils._updateStatusIfThresholdNotMetWithVote keeps the app's eligibility checkpoint at 1 (it return trues and never calls _setVotingEligibility(false)) while simultaneously adding the app to the unendorsed set (_unendorsedAppsIndex[appId] > 0isAppUnendorsed == true). So an app that lost its endorser and sat unendorsed for the entire round reads isEligible == true at both boundaries. The off-chain V3 rule excluded it (wasUnendorsedAtStart && wasUnendorsedAtEnd); V4 now includes it and pays it DBA rewards, diluting the flat share for legitimate apps and shrinking the treasury overflow.

  2. eligibleAtStart is structurally always true for every candidate. xAllocationVoting.getAppIdsOfRound(roundId) is the snapshot of x2EarnApps.allEligibleApps() taken at roundSnapshot(roundId) (see XAllocationVoting.startNewRound), and isEligible(appId, snapshot) reads the eligibility checkpoint at that exact block. Every candidate is therefore eligible at the snapshot, so !eligibleAtStart is never true and the if (!eligibleAtStart && !eligibleAtEnd) branch can never execute. Rule 3 is a no-op.

This regression is untested: the PR's own Rule 3: excludes app unendorsed at BOTH boundaries test in test/dba-pool/v4-compatibility.test.ts voids its variables and only asserts both apps are eligible, with an inline comment admitting the exclusion scenario could not be constructed.

Suggested fix

isEligible cannot reproduce the rule because the relevant signal (unendorsed/grace) is not captured by the eligibility checkpoint. Read a timepoint-aware unendorsed status instead. Since isAppUnendorsed is not timepoint-parameterized on-chain, add a checkpointed getter to X2EarnApps (e.g. isAppUnendorsedAt(bytes32 appId, uint256 timepoint)) and use it:

// Rule 3: exclude only if unendorsed at BOTH round-start AND round-end
bool unendorsedAtStart = $.x2EarnApps.isAppUnendorsedAt(appId, snapshot);
bool unendorsedAtEnd   = $.x2EarnApps.isAppUnendorsedAt(appId, deadline);
if (unendorsedAtStart && unendorsedAtEnd) {
  continue;
}

Then add a test that puts an app into grace at round start, keeps it unendorsed through round end, and asserts it is excluded from eligibleAppsForRound / receives 0 DBA.

Fix prompt:

In packages/contracts/contracts/DBAPool.sol, the internal function _buildEligibleApps (around lines 254-282) implements DBA eligibility "rule 3" (endorsement) at lines 274-277 as:
    bool eligibleAtStart = $.x2EarnApps.isEligible(appId, snapshot);
    bool eligibleAtEnd   = $.x2EarnApps.isEligible(appId, deadline);
    if (!eligibleAtStart && !eligibleAtEnd) { continue; }
This is incorrect. It is meant to replicate the deleted off-chain rule (packages/lambda/src/helpers/dba/filterEligibleApps.ts) which excluded an app only if it was UNENDORSED at BOTH the round snapshot block AND the round deadline block, using X2EarnApps.isAppUnendorsed read at those historical blocks. isEligible(appId, timepoint) is NOT the inverse of isAppUnendorsed: during the endorsement grace period (see EndorsementUtils._updateStatusIfThresholdNotMetWithVote) an app's eligibility checkpoint stays 1 (isEligible == true) while it is in the unendorsed set (isAppUnendorsed == true). As a result, an app that was unendorsed/in-grace for the whole round is now wrongly INCLUDED and paid DBA rewards. Additionally, every app returned by xAllocationVoting.getAppIdsOfRound(roundId) is by construction isEligible at roundSnapshot, so eligibleAtStart is always true and the exclusion branch is dead code.
Fix the rule so it reproduces the off-chain semantics on-chain: exclude an app only when it was unendorsed at BOTH the round snapshot and the round deadline. Because isAppUnendorsed is not timepoint-parameterized, add a checkpointed, timepoint-aware unendorsed-status getter to X2EarnApps (e.g. isAppUnendorsedAt(bytes32 appId, uint256 timepoint), backed by an existing or new checkpoint of the unendorsed flag), expose it in IX2EarnApps, and call it from _buildEligibleApps. Then add a test in packages/contracts/test/dba-pool/v4-compatibility.test.ts that drives an app into the endorsement grace period at round start, keeps it unendorsed through the round deadline, and asserts the app is excluded from eligibleAppsForRound and receives 0 DBA rewards. Run the DBA suites: cd packages/contracts && yarn test --grep "DBA Pool".

@davidecarpini davidecarpini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Severity: HIGH

File: packages/contracts/contracts/DBAPool.sol:269 (_buildEligibleApps, "rule 2" rewarded-action check)

Bug: rule 2 data-source change can deny DBA rewards to apps that actually rewarded users

The off-chain V3 rule 2 (hasRewardedActions in the deleted filterEligibleApps.ts) counted RewardDistributed events emitted by X2EarnRewardsPool within the round. V4 instead uses:

if ($.veBetterPassport.appRoundActionCount(appId, _roundId) == 0) {
  continue; // exclude
}

These two signals are not equivalent. In X2EarnRewardsPool, RewardDistributed is emitted unconditionally at the top of the distribute functions (distributeReward L280, distributeRewardForRound L331, and via _emitProof for the proof variants), before _distributeReward / _distributeRewardForRound runs. Inside those internal functions, the passport registration that increments appRoundActionCountveBetterPassport.registerAction(...) / registerActionForRound(...) — is wrapped in a try/catch that swallows failures and emits RegisterActionFailed (L405-413, L450-457).

Consequence: an app that distributed rewards to users but whose passport registrations all failed (caught) has RewardDistributed events for the round — so V3 considered it eligible — yet appRoundActionCount == 0, so V4 excludes it from DBA entirely, denying funds it legitimately earned. The try/catch exists precisely because registration can fail (passport misconfiguration/pause, entity/threshold rejection, etc.), so this is a reachable path, not theoretical. The PR docstring at L251 even describes this rule as "at least 1 action with proof", which appRoundActionCount does not actually enforce (it counts proofless distributeReward/distributeRewardForRound registrations too) — a second, opposite-direction divergence.

Suggested fix

Decide which definition is canonical and make the on-chain check match it (and update the L251 docstring accordingly). If parity with the historical "distributed ≥1 reward in the round" rule is required, base rule 2 on an on-chain per-app-per-round distribution counter rather than the passport action count, since the passport count is decoupled from actual distributions by the try/catch:

// Option A: count actual reward distributions, not passport registrations
if ($.x2EarnRewardsPool.roundRewardCount(appId, _roundId) == 0) {
  continue;
}

(roundRewardCount would be a new counter in X2EarnRewardsPool, incremented unconditionally alongside the RewardDistributed emit.) If appRoundActionCount is in fact the intended source, document the behavior change explicitly and add a test asserting that an app with RewardDistributed events but failed passport registration is (intentionally) excluded.

Fix prompt:

In packages/contracts/contracts/DBAPool.sol, _buildEligibleApps line 269 implements DBA eligibility "rule 2" as:
    if ($.veBetterPassport.appRoundActionCount(appId, _roundId) == 0) { continue; }
This was meant to replicate the deleted off-chain rule (packages/lambda/src/helpers/dba/filterEligibleApps.ts -> hasRewardedActions), which counted X2EarnRewardsPool RewardDistributed events in the round's block range. The two are not equivalent: in X2EarnRewardsPool the RewardDistributed event is emitted unconditionally before _distributeReward/_distributeRewardForRound, but the passport registration that increments appRoundActionCount (registerAction / registerActionForRound) is wrapped in try/catch and swallows failures (emitting RegisterActionFailed). So an app that distributed rewards but whose passport registration failed has RewardDistributed events (eligible under V3) but appRoundActionCount == 0 (excluded under V4), losing DBA funds it earned. Also, the DBAPool docstring at line 251 says "at least 1 action with proof", but appRoundActionCount also counts proofless distributeReward/distributeRewardForRound registrations.
Decide the canonical definition with the team and make the on-chain rule 2 match it, updating the line 251 docstring. If parity with "distributed at least one reward in the round" is required, add an unconditional per-app, per-round reward counter to X2EarnRewardsPool (incremented next to the RewardDistributed emit, before the try/catch), expose it via the interface, and use it in _buildEligibleApps instead of appRoundActionCount. Add a test in packages/contracts/test/dba-pool/v4-compatibility.test.ts covering an app that distributes a reward while passport registration fails (mock the passport to revert in registerAction) and assert the intended inclusion/exclusion. Run: cd packages/contracts && yarn test --grep "DBA Pool".

@Agilulfo1820

Copy link
Copy Markdown
Member Author

Bug: on-chain rule 3 is dead code and silently changes who receives DBA rewards

V4 replaces the off-chain endorsement rule — which read isAppUnendorsed(appId) at the round's start/end blocks — with:

bool eligibleAtStart = $.x2EarnApps.isEligible(appId, snapshot);
bool eligibleAtEnd   = $.x2EarnApps.isEligible(appId, deadline);
if (!eligibleAtStart && !eligibleAtEnd) {
  continue; // exclude
}

Two compounding problems, both verified against the contracts:

  1. isEligible is not the inverse of isAppUnendorsed. During the endorsement grace period, EndorsementUtils._updateStatusIfThresholdNotMetWithVote keeps the app's eligibility checkpoint at 1 (it return trues and never calls _setVotingEligibility(false)) while simultaneously adding the app to the unendorsed set (_unendorsedAppsIndex[appId] > 0isAppUnendorsed == true). So an app that lost its endorser and sat unendorsed for the entire round reads isEligible == true at both boundaries. The off-chain V3 rule excluded it (wasUnendorsedAtStart && wasUnendorsedAtEnd); V4 now includes it and pays it DBA rewards, diluting the flat share for legitimate apps and shrinking the treasury overflow.
  2. eligibleAtStart is structurally always true for every candidate. xAllocationVoting.getAppIdsOfRound(roundId) is the snapshot of x2EarnApps.allEligibleApps() taken at roundSnapshot(roundId) (see XAllocationVoting.startNewRound), and isEligible(appId, snapshot) reads the eligibility checkpoint at that exact block. Every candidate is therefore eligible at the snapshot, so !eligibleAtStart is never true and the if (!eligibleAtStart && !eligibleAtEnd) branch can never execute. Rule 3 is a no-op.

This regression is untested: the PR's own Rule 3: excludes app unendorsed at BOTH boundaries test in test/dba-pool/v4-compatibility.test.ts voids its variables and only asserts both apps are eligible, with an inline comment admitting the exclusion scenario could not be constructed.

Good point.

We do not have isAppUnendorsedAt, so I will look at getScoreAtTimepoint(appId, timepoint), which is kind of the same thing.

@Agilulfo1820

Copy link
Copy Markdown
Member Author

Severity: HIGH

File: packages/contracts/contracts/DBAPool.sol:269 (_buildEligibleApps, "rule 2" rewarded-action check)

Bug: rule 2 data-source change can deny DBA rewards to apps that actually rewarded users

The off-chain V3 rule 2 (hasRewardedActions in the deleted filterEligibleApps.ts) counted RewardDistributed events emitted by X2EarnRewardsPool within the round. V4 instead uses:

if ($.veBetterPassport.appRoundActionCount(appId, _roundId) == 0) {
  continue; // exclude
}

These two signals are not equivalent. In X2EarnRewardsPool, RewardDistributed is emitted unconditionally at the top of the distribute functions (distributeReward L280, distributeRewardForRound L331, and via _emitProof for the proof variants), before _distributeReward / _distributeRewardForRound runs. Inside those internal functions, the passport registration that increments appRoundActionCountveBetterPassport.registerAction(...) / registerActionForRound(...) — is wrapped in a try/catch that swallows failures and emits RegisterActionFailed (L405-413, L450-457).

Consequence: an app that distributed rewards to users but whose passport registrations all failed (caught) has RewardDistributed events for the round — so V3 considered it eligible — yet appRoundActionCount == 0, so V4 excludes it from DBA entirely, denying funds it legitimately earned. The try/catch exists precisely because registration can fail (passport misconfiguration/pause, entity/threshold rejection, etc.), so this is a reachable path, not theoretical. The PR docstring at L251 even describes this rule as "at least 1 action with proof", which appRoundActionCount does not actually enforce (it counts proofless distributeReward/distributeRewardForRound registrations too) — a second, opposite-direction divergence.

Suggested fix

Decide which definition is canonical and make the on-chain check match it (and update the L251 docstring accordingly). If parity with the historical "distributed ≥1 reward in the round" rule is required, base rule 2 on an on-chain per-app-per-round distribution counter rather than the passport action count, since the passport count is decoupled from actual distributions by the try/catch:

// Option A: count actual reward distributions, not passport registrations
if ($.x2EarnRewardsPool.roundRewardCount(appId, _roundId) == 0) {
  continue;
}

(roundRewardCount would be a new counter in X2EarnRewardsPool, incremented unconditionally alongside the RewardDistributed emit.) If appRoundActionCount is in fact the intended source, document the behavior change explicitly and add a test asserting that an app with RewardDistributed events but failed passport registration is (intentionally) excluded.

Fix prompt:

In packages/contracts/contracts/DBAPool.sol, _buildEligibleApps line 269 implements DBA eligibility "rule 2" as:
    if ($.veBetterPassport.appRoundActionCount(appId, _roundId) == 0) { continue; }
This was meant to replicate the deleted off-chain rule (packages/lambda/src/helpers/dba/filterEligibleApps.ts -> hasRewardedActions), which counted X2EarnRewardsPool RewardDistributed events in the round's block range. The two are not equivalent: in X2EarnRewardsPool the RewardDistributed event is emitted unconditionally before _distributeReward/_distributeRewardForRound, but the passport registration that increments appRoundActionCount (registerAction / registerActionForRound) is wrapped in try/catch and swallows failures (emitting RegisterActionFailed). So an app that distributed rewards but whose passport registration failed has RewardDistributed events (eligible under V3) but appRoundActionCount == 0 (excluded under V4), losing DBA funds it earned. Also, the DBAPool docstring at line 251 says "at least 1 action with proof", but appRoundActionCount also counts proofless distributeReward/distributeRewardForRound registrations.
Decide the canonical definition with the team and make the on-chain rule 2 match it, updating the line 251 docstring. If parity with "distributed at least one reward in the round" is required, add an unconditional per-app, per-round reward counter to X2EarnRewardsPool (incremented next to the RewardDistributed emit, before the try/catch), expose it via the interface, and use it in _buildEligibleApps instead of appRoundActionCount. Add a test in packages/contracts/test/dba-pool/v4-compatibility.test.ts covering an app that distributes a reward while passport registration fails (mock the passport to revert in registerAction) and assert the intended inclusion/exclusion. Run: cd packages/contracts && yarn test --grep "DBA Pool".

I would say that is a minor and is up to apps (and actually an incentive) to correctly bump the gas limit of their transactions so that try catch does not revert.

Agilulfo1820 and others added 2 commits June 8, 2026 13:53
The caller no longer chooses who gets paid — the eligible set is derived
on-chain from XAllocationVoting + VeBetterPassport + X2EarnApps state.
Idempotency is enforced by dbaRewardsDistributed[roundId], so a permissionless
trigger has no exploitable surface. The DISTRIBUTOR_ROLE constant is kept for
storage / role-management compatibility, just no longer used as an entrypoint
guard. whenNotPaused + nonReentrant remain.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`isEligible(appId, snapshot)` was the wrong primitive for rule 3 ("exclude
when unendorsed at BOTH boundaries") for two compounding reasons:

  1. Apps in `xAllocationVoting.getAppIdsOfRound(roundId)` ARE the snapshot
     of `allEligibleApps()` taken at `roundSnapshot(roundId)`, so
     `isEligible(_, snapshot)` was structurally always true for every
     candidate. The AND short-circuit could never trigger — dead code.

  2. During the endorsement grace period,
     `EndorsementUtils._updateStatusIfThresholdNotMetWithVote` keeps the
     eligibility checkpoint at 1 while marking the app in the unendorsed
     set. An app that lost its endorser for the entire round read
     `isEligible == true` at both boundaries, so off-chain V3 excluded it
     while V4 was paying it DBA — diluting the flat share and shrinking
     the treasury overflow.

Switch to `getScoreAtTimepoint(appId, t) < endorsementScoreThreshold()`,
which is the on-chain equivalent of the off-chain `isAppUnendorsed @ t`
query (score checkpoint is updated on every endorse/unendorse).

Test: replace the dead-code placeholder with a positive assertion that
the score checkpoint sits at/above the threshold for fully endorsed apps
at both boundaries. The exclusion-at-both-boundaries scenario requires
the grace-period flow (`unendorseApp` mid-round-N then distribute for
round N+1 before anyone calls `checkEndorsement`); tracked as a
follow-up that needs IX2EarnApps mock scaffolding.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@davidecarpini davidecarpini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Severity: HIGH

File: packages/contracts/contracts/DBAPool.sol:174-184 (with packages/lambda/src/distributeWeeklyAppRewards/lambda.ts and packages/lambda/src/distributeDBA/lambda.ts)

Bug: an empty eligible set now irreversibly sweeps the entire round's DBA pool to the treasury, and the off-chain safety net that prevented this was removed

In V4 distributeDBARewards marks the round distributed before the empty-set check and then forwards the whole pool to the treasury:

$.dbaRewardsDistributed[_roundId] = true;          // line 174 — set first, irreversible

uint256 dbaPoolAmount = $.xAllocationPool.unallocatedFunds(_roundId);

if (eligibleCount == 0) {                            // line 178
  if (dbaPoolAmount > 0) {
    require($.b3tr.transfer($.treasuryAddress, dbaPoolAmount), "DBAPool: Transfer of overflow to treasury failed");
  }
  emit FundsDistributedToTreasury(dbaPoolAmount, _roundId);
  return;                                            // round can never be re-distributed
}

Two things make this dangerous compared to V3:

  1. The off-chain guard was deleted. Both lambdas previously did if (eligibleApps.length === 0) { ...post "no eligible apps"...; return { skipped: true } } and never sent the transaction, so an empty/incorrect set left the funds untouched and recoverable. The diff removes that block entirely — eligibleApps is now only logged, and distributeDBARewards is always called once canDistributeDBARewards is true.

  2. The eligible set now hinges on appRoundActionCount (Rule 2, line 287), which is populated only by a swallowed try/catch call to veBetterPassport.registerAction(...) inside X2EarnRewardsPool._distributeReward (X2EarnRewardsPool.sol:407 and :450). If that registration fails or the rewards pool ever loses ACTION_REGISTRAR_ROLE on the passport, appRoundActionCount stays 0 for every app, _buildEligibleApps returns an empty set, and the entire unallocated pool for that round is irreversibly sent to the treasury while the round is permanently marked distributed. The old event-based filter had no such dependency.

Because distribution is now permissionless and idempotency is one-way (dbaRewardsDistributed[_roundId] = true), there is no recovery path: the first call wins and the funds are gone to treasury for that round.

Suggested fix

Do not auto-sweep to treasury on an empty set; require an explicit, privileged decision (or at least don't mark the round distributed so it can be retried after the root cause is fixed):

if (eligibleCount == 0) {
  // Do NOT mark distributed and do NOT auto-route the whole pool to treasury.
  // An empty set almost always signals a mis-derivation (e.g. passport
  // registration failure), not a legitimate "nobody earned" round.
  revert("DBAPool: empty eligible set — refusing to sweep pool to treasury");
}
$.dbaRewardsDistributed[_roundId] = true; // only mark distributed on a real distribution

If sweeping an empty round to treasury is genuinely desired, gate it behind a separate admin-only function rather than the permissionless distribute path, and keep the lambda's explicit empty-set short-circuit + Slack alert so an operator reviews before funds move.

Fix prompt:

In packages/contracts/contracts/DBAPool.sol, the distributeDBARewards(uint256) function (around lines 174-184) sets $.dbaRewardsDistributed[_roundId] = true before the eligibleCount == 0 check and then transfers the entire dbaPoolAmount to the treasury and returns. Because distribution is permissionless and the round is marked distributed first, this is irreversible: if the on-chain eligible set is empty for any reason (e.g. veBetterPassport.registerAction was swallowed by the try/catch in X2EarnRewardsPool._distributeReward, or the rewards pool lost ACTION_REGISTRAR_ROLE, so appRoundActionCount is 0 for all apps), an entire round's DBA pool is swept to the treasury with no recovery. Change the behavior so an empty eligible set does NOT auto-sweep the pool and does NOT mark the round as distributed: revert with a clear message instead, and only set dbaRewardsDistributed[_roundId] = true on a real (non-empty) distribution. If sweeping an empty round to treasury must remain possible, move it into a separate DEFAULT_ADMIN_ROLE-gated function. Also restore the explicit empty-set short-circuit and Slack alert in packages/lambda/src/distributeWeeklyAppRewards/lambda.ts and packages/lambda/src/distributeDBA/lambda.ts so the lambda does not blindly call distribute when eligibleAppsForRound returns an empty array. Update/extend the tests in packages/contracts/test/dba-pool/v4-compatibility.test.ts (the "empty eligible set routes entire pool to treasury" test) to reflect the new revert behavior, and run the shard7e/shard7b DBA pool tests plus the lambda build (LAMBDA_ENV=mainnet yarn build:lambda).

@davidecarpini davidecarpini left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Severity: HIGH

File: packages/contracts/contracts/DBAPool.sol:292-294 (_buildEligibleApps, Rule 3), test gap in packages/contracts/test/dba-pool/v4-compatibility.test.ts:1578

Bug: the on-chain Rule 3 does not replicate the off-chain isAppUnendorsed predicate it claims to, and the exclusion branch has zero passing test coverage

The PR description says V4 "replicates the 3 off-chain rules." The deleted off-chain filter (filterEligibleApps.ts) implemented Rule 3 by calling isAppUnendorsed(appId) at the round-start and round-end blocks (via historical revision reads). On-chain, isAppUnendorsed is list-membership:

// EndorsementUtils.sol
function isAppUnendorsed(bytes32 appId, bool isBlacklisted) external view returns (bool) {
  if (isBlacklisted) return false;
  return $._unendorsedAppsIndex[appId] > 0;   // membership in the pending-endorsement list
}

V4 instead uses a score-vs-threshold comparison:

// DBAPool.sol:292-294
bool unendorsedAtStart = $.x2EarnApps.getScoreAtTimepoint(appId, snapshot) < threshold;
bool unendorsedAtEnd   = $.x2EarnApps.getScoreAtTimepoint(appId, deadline) < threshold;
if (unendorsedAtStart && unendorsedAtEnd) { continue; }

These are different predicates. An app can be below the score threshold at a block without being in the unendorsed list (the list is only mutated when checkEndorsement/endorse/unendorse runs), and it can be in the unendorsed list while a stale score checkpoint still reads at/above threshold. The two only coincide if list-membership flips exactly when the score crosses the threshold — which is precisely the grace-period behavior the code comment is unsure about.

Two concrete problems:

  1. The in-code justification argues against the wrong primitive. The doc-block on _buildEligibleApps explains why isEligible is unsuitable — but the off-chain code never used isEligible; it used isAppUnendorsed. The justification therefore never establishes that getScoreAtTimepoint < threshold equals the historical isAppUnendorsed it is replacing. (A switch to a score-based signal may well be a defensible/necessary choice on-chain since the unendorsed list is not historically queryable from a contract — but that argument has to be made against isAppUnendorsed, and the divergence in fund routing has to be signed off.)

  2. The exclusion path is completely untested. The only test for "unendorsed at BOTH boundaries → excluded" is it.skip(...) at v4-compatibility.test.ts:1578. So the one branch of Rule 3 that removes an app's B3TR and redirects it to the treasury overflow is never exercised. Rule 3's threshold direction (strict <), the snapshot-vs-deadline timepoints, and the current-vs-historical threshold read (endorsementScoreThreshold() is read at the current block, not at the round) are all unverified.

A wrong result here either denies a legitimately-endorsed app its DBA reward (funds → treasury overflow) or pays an app that should have been excluded.

Suggested fix

Add real coverage for the exclusion branch against a mockable IX2EarnApps, and confirm the score-based predicate is the intended (signed-off) replacement for the historical isAppUnendorsed:

// Unit test against an IX2EarnApps mock with scripted getScoreAtTimepoint:
//   score(snapshot) <  threshold && score(deadline) <  threshold  => EXCLUDED
//   score(snapshot) >= threshold && score(deadline) <  threshold  => included
//   score(snapshot) <  threshold && score(deadline) >= threshold  => included
//   exactly == threshold at both                                  => included (strict <)

Also consider reading the threshold consistently with the scores (i.e., whether endorsementScoreThreshold should be evaluated as-of the round rather than at distribution time), and update the _buildEligibleApps doc-block to justify the change relative to isAppUnendorsed, not isEligible.

Fix prompt:

In packages/contracts/contracts/DBAPool.sol, _buildEligibleApps (lines ~292-294) implements DBA Rule 3 as `getScoreAtTimepoint(appId, snapshot) < threshold && getScoreAtTimepoint(appId, deadline) < threshold => exclude`. The off-chain filter it replaces (deleted packages/lambda/src/helpers/dba/filterEligibleApps.ts) instead called x2EarnApps.isAppUnendorsed(appId) at the round-start and round-end blocks, which is list-membership (_unendorsedAppsIndex[appId] > 0 in EndorsementUtils.sol), not a score-threshold comparison. These are different predicates and can diverge, changing which apps receive B3TR vs. having it routed to the treasury overflow. First, write unit tests against a mock IX2EarnApps (with scripted getScoreAtTimepoint and endorsementScoreThreshold) that exercise all four corners of Rule 3, including the currently-skipped "unendorsed at BOTH boundaries => excluded" case at packages/contracts/test/dba-pool/v4-compatibility.test.ts:1578 — replace the it.skip with a real passing test. Verify the strict `<` direction, that snapshot and deadline are the correct block timepoints, and whether endorsementScoreThreshold should be read as-of the round rather than at distribution time. If the score-based predicate is confirmed as the intended replacement, update the _buildEligibleApps doc-block to justify it relative to isAppUnendorsed (not isEligible). Run the shard7e (v4-compatibility) and shard7b DBA pool test shards and report results.

Base automatically changed from feat/split-start-round-lambda to main June 8, 2026 13:21
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Governance Frontend – Preview

Name Status Preview Updated (UTC)
staging ✅ Ready (f61cb98) Custom Domain →Default URL → 2026-06-08 15:24:21
beta ✅ Ready (f61cb98) Custom Domain →Default URL → 2026-06-08 15:24:21

⏱️ Note: Custom domains may take 5-10 minutes to activate on first deployment. Use Default URL in the meantime.


Built with commit f61cb98 • Preview environments are automatically destroyed when PRs are closed

@Agilulfo1820 Agilulfo1820 temporarily deployed to AWS staging governance build-time June 8, 2026 15:03 — with GitHub Actions Inactive
@Agilulfo1820 Agilulfo1820 temporarily deployed to AWS beta governance build-time June 8, 2026 15:03 — with GitHub Actions Inactive
@Agilulfo1820 Agilulfo1820 added the increment:minor PR adds functionality in a backwards compatible manner label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

increment:minor PR adds functionality in a backwards compatible manner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants