Root + Impact

Description

• Describe the normal behavior in one or more sentences

• Explain the specific issue or problem in one or more sentences

Risk

Likelihood:

• Reason 1 // Describe WHEN this will occur (avoid using "if" statements)

• Reason 2

Impact:

• Impact 1

• Impact 2

Proof of Concept

Recommended Mitigation

Severity
Medium–High
Likelihood
Medium–High
Impact
Medium–High
Description
_clampUserSums is meant to ensure that a user's stake-time accounting correctly reflects deposits made before the risk window opened — promoting pre-risk timestamps up to riskWindowStart so users can't claim credit for exposure they never actually had. But it operates on the user's aggregate balance and aggregate time-sum, not per-deposit, which produces a bug when a single user has both an early and a late deposit.

Root cause: the clamp is an all-or-nothing overwrite gated by a threshold that mixes early and late deposits together. It cannot distinguish "some of this user's stake needs promoting to start" from "all of it does."

Impact: Any staker who deposits a small amount before the risk window and a larger amount just after it can retroactively inflate their bonus share relative to stakers who deposited the same total entirely and honestly post-risk. This is a bonus-accounting integrity bug, not a cosmetic one — it misallocates real token value between stakers, systematically rewarding a specific, easily-reproducible deposit-timing pattern over honest late deposits.

Likelihood: Requires no special privilege or attack — any ordinary staker can trigger this simply by depositing twice with the right timing relative to riskWindowStart, which is public information once the risk window opens. The condition (early deposit relatively small, later deposit only slightly past start) is a plausible, easily-achievable pattern rather than a narrow edge case.

Mitigation
Track per-deposit entries (or at minimum split "pre-risk-window residual" from "post-risk-window residual" per user) rather than clamping a single aggregated sum. The per-deposit clamp already present in stake() is correct in isolation — the fix is to remove the later aggregate re-clamp entirely, so no step ever re-touches or overwrites a sum that already reflects individually-accurate timestamps.

Bad

function clampUserSums(address u) internal {
uint256 stake = eligibleStake[u];

if (riskWindowStart != 0 && userSumStakeTime[u]  start, so stake()'s own clamp correctly
    // does NOT fire — the real, later timestamp is recorded honestly):
    pool.stake(user, 100, t1);

    uint256 accurateSum = 100 * t0 + 100 * t1; // = 40,000 + 101,000 = 141,000
    assertEq(pool.userSumStakeTime(user), accurateSum, "sum accurately reflects both deposits pre-clamp");

    uint256 totalStake = pool.eligibleStake(user); // 200
    assertTrue(
        accurateSum < totalStake * START,
        "aggregate clamp condition holds even though half the stake genuinely entered post-risk"
    );

    uint256 scoreTermBefore = pool.negativeScoreTerm(user);

    // Claim-time clamp fires and destroys the accurate sum.
    pool.clampUserSums(user);

    uint256 clampedSum = pool.userSumStakeTime(user);
    assertEq(clampedSum, totalStake * START, "entire balance pretended to enter exactly at start");
    assertLt(clampedSum, accurateSum, "clamp overwrote a smaller value, destroying the real t1 contribution");

    uint256 scoreTermAfter = pool.negativeScoreTerm(user);
    assertLt(scoreTermAfter, scoreTermBefore, "the subtracted term shrank -> final score is inflated");
}

/// @notice Comparative control: an honest staker who deposits the same
/// total (200) entirely post-risk, at the same t1, ends up with a LOWER
/// score than the dishonest mixed-timing staker after the buggy clamp —
/// even though the honest staker's stake was, if anything, committed to
/// risk for longer on average in spirit of the design.
function test_Bug_MixedDepositorEndsUpWithBetterScoreThanFullyHonestLateDepositor() public {
    BonusShareHarnessBuggy pool = new BonusShareHarnessBuggy();
    pool.setRiskWindowStart(START);
    pool.setPoolEndTime(T);

    uint256 t0 = 400;
    uint256 t1 = 1_010;

    // Dishonest / mixed-timing staker.
    pool.setRiskWindowStart(0);
    pool.stake(user, 100, t0);
    pool.setRiskWindowStart(START);
    pool.stake(user, 100, t1);
    pool.clampUserSums(user);

    // Fully honest staker: entire 200 deposited post-risk, at the same t1.
    pool.stake(honestUser, 200, t1);
    pool.clampUserSums(honestUser); // no-op: honest sum already >= stake*start

    uint256 mixedScoreTerm = pool.negativeScoreTerm(user);
    uint256 honestScoreTerm = pool.negativeScoreTerm(honestUser);

    // Lower subtracted term => higher final score. The mixed depositor
    // (who genuinely had less "at risk" exposure, having deposited half
    // before the risk window even opened) ends up scoring BETTER than the
    // staker who committed the full amount honestly post-risk.
    assertLt(mixedScoreTerm, honestScoreTerm, "mixed-timing staker's score is inflated above the honest staker's");
}

function test_Fix_NoAggregateOverwritePreservesAccurateSum() public {
    BonusShareHarnessFixed pool = new BonusShareHarnessFixed();
    pool.setRiskWindowStart(0);
    pool.setPoolEndTime(T);

    uint256 t0 = 400;
    uint256 t1 = 1_010;

    pool.stake(user, 100, t0);
    pool.setRiskWindowStart(START);
    pool.stake(user, 100, t1);

    uint256 accurateSum = 100 * t0 + 100 * t1;
    assertEq(pool.userSumStakeTime(user), accurateSum, "sum remains accurate, nothing overwrites it later");

    // Honest staker with the same post-risk timing now scores consistently
    // relative to the mixed depositor — no perverse inflation.
    pool.stake(honestUser, 200, t1);
    assertLt(
        pool.negativeScoreTerm(user),
        pool.negativeScoreTerm(honestUser),
        "mixed depositor's subtracted term is still LOWER (they genuinely had less at-risk time), consistent with reality"
    );
}

}