[audit] 1. Rewards are lost if the distribution is funded after the e…

…xpiry (#862)

* fix cargo errors

* reorganized fund logic and fixed rewards being lost when funding a previously expired discontinuous linear distribution

contracts/distribution/dao-rewards-distributor/src/contract.rs

@@ -279,36 +279,106 @@ fn execute_fund_native(
 }
 
 fn execute_fund(
+    deps: DepsMut,
+    env: Env,
+    distribution: DistributionState,
+    amount: Uint128,
+) -> Result<Response, ContractError> {
+    match distribution.active_epoch.emission_rate {
+        EmissionRate::Paused {} => execute_fund_paused(deps, distribution, amount),
+        EmissionRate::Immediate {} => execute_fund_immediate(deps, env, distribution, amount),
+        EmissionRate::Linear { .. } => execute_fund_linear(deps, env, distribution, amount),
+    }
+}
+
+/// funding a paused distribution simply increases the funded amount.
+fn execute_fund_paused(
+    deps: DepsMut,
+    mut distribution: DistributionState,
+    amount: Uint128,
+) -> Result<Response, ContractError> {
+    distribution.funded_amount += amount;
+
+    DISTRIBUTIONS.save(deps.storage, distribution.id, &distribution)?;
+
+    Ok(Response::new()
+        .add_attribute("action", "fund")
+        .add_attribute("id", distribution.id.to_string())
+        .add_attribute("denom", distribution.get_denom_string())
+        .add_attribute("amount_funded", amount))
+}
+
+/// funding an immediate distribution instantly distributes the new amount.
+fn execute_fund_immediate(
+    deps: DepsMut,
+    env: Env,
+    mut distribution: DistributionState,
+    amount: Uint128,
+) -> Result<Response, ContractError> {
+    distribution.funded_amount += amount;
+
+    // for immediate distribution, update total_earned_puvp instantly since we
+    // need to know the change in funded_amount to calculate the new
+    // total_earned_puvp.
+    distribution.update_immediate_emission_total_earned_puvp(deps.as_ref(), &env.block, amount)?;
+
+    DISTRIBUTIONS.save(deps.storage, distribution.id, &distribution)?;
+
+    Ok(Response::new()
+        .add_attribute("action", "fund")
+        .add_attribute("id", distribution.id.to_string())
+        .add_attribute("denom", distribution.get_denom_string())
+        .add_attribute("amount_funded", amount))
+}
+
+/// funding a linear distribution requires some complex logic based on whether
+/// or not the distribution is continuous and whether or not it's expired.
+///
+/// expired continuous distributions experience backfill with the new funds,
+/// whereas expired discontinuous distributions begin anew (and all past rewards
+/// must be taken into account before restarting distribution).
+fn execute_fund_linear(
     deps: DepsMut,
     env: Env,
     mut distribution: DistributionState,
     amount: Uint128,
 ) -> Result<Response, ContractError> {
-    // will only be true if emission rate is linear and continuous is true
     let continuous =
         if let EmissionRate::Linear { continuous, .. } = distribution.active_epoch.emission_rate {
             continuous
         } else {
             false
         };
+    let previously_funded = !distribution.funded_amount.is_zero();
+    let was_expired = distribution.active_epoch.ends_at.is_expired(&env.block);
+    let discontinuous_expired = !continuous && was_expired;
 
     // restart the distribution from the current block if it hasn't yet started
-    // (i.e. never been funded), or if it's expired (i.e. all funds have been
-    // distributed) and not continuous. if it is continuous, treat it as if it
-    // weren't expired by simply adding the new funds and recomputing the end
-    // date, keeping start date the same, effectively backfilling rewards.
-    let restart_distribution = if distribution.funded_amount.is_zero() {
-        true
-    } else {
-        !continuous && distribution.active_epoch.ends_at.is_expired(&env.block)
-    };
+    // (i.e. never been funded) OR if it's both discontinuous and expired (i.e.
+    // all past funds should have been distributed and we're effectively
+    // beginning a new distribution with new funds). this ensures the new funds
+    // start being distributed from now instead of from the past.
+    //
+    // if already funded and continuous or not expired (else block), just add
+    // the new funds and leave start date the same, backfilling rewards.
+    if !previously_funded || discontinuous_expired {
+        // if funding an expired discontinuous distribution that's previously
+        // been funded, ensure all rewards are taken into account before
+        // restarting, in case users haven't claimed yet, by adding the final
+        // total rewards earned to the historical value.
+        if discontinuous_expired && previously_funded {
+            let final_total_earned_puvp =
+                get_active_total_earned_puvp(deps.as_ref(), &env.block, &distribution)?;
+            distribution.historical_earned_puvp = distribution
+                .historical_earned_puvp
+                .checked_add(final_total_earned_puvp)
+                .map_err(|err| ContractError::DistributionHistoryTooLarge {
+                    err: err.to_string(),
+                })?;
+            // last updated block is reset to the new start block below
+        }
 
-    // if necessary, restart the distribution from the current block so that the
-    // new funds start being distributed from now instead of from the past, and
-    // reset funded_amount to the new amount since we're effectively starting a
-    // new distribution. otherwise, just add the new amount to the existing
-    // funded_amount
-    if restart_distribution {
+        // reset all starting fields since a new distribution is starting
         distribution.funded_amount = amount;
         distribution.active_epoch.started_at = match distribution.active_epoch.emission_rate {
             EmissionRate::Paused {} => Expiration::Never {},
@@ -318,10 +388,14 @@ fn execute_fund(
                 Duration::Time(_) => Expiration::AtTime(env.block.time),
             },
         };
+        distribution.active_epoch.total_earned_puvp = Uint256::zero();
+        distribution.active_epoch.last_updated_total_earned_puvp =
+            distribution.active_epoch.started_at;
     } else {
         distribution.funded_amount += amount;
     }
 
+    // update the end block based on the new funds and potentially updated start
     let new_funded_duration = distribution
         .active_epoch
         .emission_rate
@@ -331,26 +405,16 @@ fn execute_fund(
         None => Expiration::Never {},
     };
 
-    // if immediate distribution, update total_earned_puvp instantly since we
-    // need to know the delta in funding_amount to calculate the new
-    // total_earned_puvp.
-    if (distribution.active_epoch.emission_rate == EmissionRate::Immediate {}) {
-        distribution.update_immediate_emission_total_earned_puvp(
-            deps.as_ref(),
-            &env.block,
-            amount,
-        )?;
-
-    // if continuous, meaning rewards should have been distributed in the past
-    // but were not due to lack of sufficient funding, ensure the total rewards
-    // earned puvp is up to date.
-    } else if !restart_distribution && continuous {
+    // if continuous, funds existed in the past, and the distribution was
+    // expired, some rewards may not have been distributed due to lack of
+    // sufficient funding. ensure the total rewards earned puvp is up to date
+    // based on the original start block and the newly updated end block.
+    if continuous && previously_funded && was_expired {
         distribution.active_epoch.total_earned_puvp =
             get_active_total_earned_puvp(deps.as_ref(), &env.block, &distribution)?;
+        distribution.active_epoch.bump_last_updated(&env.block);
     }
 
-    distribution.active_epoch.bump_last_updated(&env.block);
-
     DISTRIBUTIONS.save(deps.storage, distribution.id, &distribution)?;
 
     Ok(Response::new()

contracts/distribution/dao-rewards-distributor/src/testing/tests.rs

@@ -2361,3 +2361,89 @@ fn test_query_info() {
         }
     );
 }
+
+#[test]
+fn test_rewards_not_lost_after_discontinuous_restart() {
+    let mut suite = SuiteBuilder::base(super::suite::DaoType::Native)
+        .with_rewards_config(RewardsConfig {
+            amount: 3_000,
+            denom: UncheckedDenom::Native(DENOM.to_string()),
+            duration: Duration::Height(1),
+            destination: None,
+            continuous: false,
+        })
+        .build();
+
+    suite.assert_amount(3_000);
+    suite.assert_ends_at(Expiration::AtHeight(33_333));
+    suite.assert_duration(1);
+
+    // skip to end
+    suite.skip_blocks(33_333);
+
+    // check pending rewards
+    suite.assert_pending_rewards(ADDR1, 1, 49999500);
+    suite.assert_pending_rewards(ADDR2, 1, 24999750);
+    suite.assert_pending_rewards(ADDR3, 1, 24999750);
+
+    // before user claim rewards, someone funded
+    suite.fund_native(1, coin(1u128, DENOM));
+
+    // pending rewards should still exist
+    suite.assert_pending_rewards(ADDR1, 1, 49999500);
+    suite.assert_pending_rewards(ADDR2, 1, 24999750);
+    suite.assert_pending_rewards(ADDR3, 1, 24999750);
+}
+
+#[test]
+fn test_fund_while_paused() {
+    let mut suite = SuiteBuilder::base(super::suite::DaoType::CW4).build();
+
+    suite.assert_amount(1_000);
+    suite.assert_ends_at(Expiration::AtHeight(1_000_000));
+    suite.assert_duration(10);
+
+    // skip 1/10th
+    suite.skip_blocks(100_000);
+
+    // check pending rewards
+    suite.assert_pending_rewards(ADDR1, 1, 5_000_000);
+    suite.assert_pending_rewards(ADDR2, 1, 2_500_000);
+    suite.assert_pending_rewards(ADDR3, 1, 2_500_000);
+
+    // pause
+    suite.pause_emission(1);
+
+    // pending rewards should still exist
+    suite.assert_pending_rewards(ADDR1, 1, 5_000_000);
+    suite.assert_pending_rewards(ADDR2, 1, 2_500_000);
+    suite.assert_pending_rewards(ADDR3, 1, 2_500_000);
+
+    // fund during pause the amount that's already been distributed
+    suite.fund_native(1, coin(10_000_000, DENOM));
+
+    // restart
+    suite.update_emission_rate(1, Duration::Height(10), 1_000, true);
+
+    // expect it to last as long as it was initially going to
+    suite.assert_ends_at(Expiration::AtHeight(1_000_000 + 100_000));
+
+    // skip 1/10th
+    suite.skip_blocks(100_000);
+
+    // check pending rewards
+    suite.assert_pending_rewards(ADDR1, 1, 2 * 5_000_000);
+    suite.assert_pending_rewards(ADDR2, 1, 2 * 2_500_000);
+    suite.assert_pending_rewards(ADDR3, 1, 2 * 2_500_000);
+
+    // pause and fund more
+    suite.pause_emission(1);
+    suite.fund_native(1, coin(100_000_000, DENOM));
+
+    // restart
+    suite.update_emission_rate(1, Duration::Height(10), 1_000, true);
+
+    // expect the start and end to adjust again
+    suite.assert_started_at(Expiration::AtHeight(200_000));
+    suite.assert_ends_at(Expiration::AtHeight(1_000_000 + 100_000 + 1_000_000));
+}