There's no guarantee that the period updates continuously in LootCreator

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xd60b2a57fbcee8f5b277914c96b660c20fe206b1650244efb1a31badd49c38cd Severity: medium

Description: Description\ We have an important function called _updatePeriod in LootCreator. Within this function, we update the current period budget, handle pending budgets, and update some critical variables. It's essential to execute these updates correctly to ensure receiving PAL and extra token rewards.

Attack Scenario\

• We have a variable called nextBudgetUpdatePeriod, and we initialize its initial value in the constructor.

  constructor(address _loot, address _lootVoteController, address _holyPower) {
  nextBudgetUpdatePeriod = (block.timestamp + WEEK) / WEEK * WEEK;   // ===> @audit
  }

• There's no guarantee that this value is always up to date. For instance, it might not be updated after deployment until the first reward claiming occurs, or there could be a gap of several weeks between rewards claiming, and so on.
• When the distributor notifies the LootCreator, it invokes the _updatePeriod function.

  function notifyDistributedQuestPeriod(uint256 questId, uint256 period, uint256 totalRewards) external onlyAllowedDistributor nonReentrant {
  _pullBudget();
  _updatePeriod();  // ==> @audit, here
  }

• In the _updatePeriod function, we cannot be sure that nextBudgetUpdatePeriod is larger than the period.

  function _updatePeriod() internal {
  if(block.timestamp < nextBudgetUpdatePeriod) return;
  periodBlockCheckpoint[nextBudgetUpdatePeriod] = block.number;
  
  periodBudget[nextBudgetUpdatePeriod] = pending;
  nextBudgetUpdatePeriod += WEEK;
  }

This can lead to several vulnerabilities.

• Because the period is larger than nextBudgetUpdatePeriod, the periodBudget[period] is 0, so gaugeBudgetPerPeriod is also 0. And we only allocate the gauge for the period once. This means that users cannot claim rewards for this gauge and period.

  function notifyDistributedQuestPeriod(uint256 questId, uint256 period, uint256 totalRewards) external onlyAllowedDistributor nonReentrant {
  if(isGaugeAllocatedForPeriod[gauge][period]) return;   // ==> @audit, only once
  isGaugeAllocatedForPeriod[gauge][period] = true;
  
  Budget memory budget = periodBudget[period];  // ==> @audit, 0
  
  uint256 palAmount = uint256(budget.palAmount) * gaugeWeight / UNIT;
  uint256 extraAmount = uint256(budget.extraAmount) * gaugeWeight / UNIT;
  
  gaugeBudgetPerPeriod[gauge][period] = Budget(
      uint128(palAmount),
      uint128(extraAmount)
  );  // ==> @audit, also 0
  }

• And also, the _createLoot function may not work correctly because periodBlockCheckpoint[period] is not updated yet.

  function _createLoot(address user, address distributor, uint256 questId, uint256 period) internal {
  vars.totalPower = IHolyPowerDelegation(holyPower).total_locked_at(periodBlockCheckpoint[period]);
  vars.lockedRatio = (vars.userPower * UNIT) / vars.totalPower;
  }

Of course, there is an external function called updatePeriod, and if we trust that a trusted entity always ensures the proper update, there is no need to call this function when the distributor notifies the LootCreator. Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

   Modify _updatePeriod function.

   
   function _updatePeriod() internal {
   -     if(block.timestamp < nextBudgetUpdatePeriod) return;

• while (nextBudgetUpdatePeriod <= block.timestamp) { // Save the current block number for checkpointing periodBlockCheckpoint[nextBudgetUpdatePeriod] = block.number;

      // Update the current period budget
      Budget memory pending = pengingBudget;
      pengingBudget = Budget(0, 0);
  
      // 2 weeks difference to not impact the current distribution and allocations
      uint256 lastFinishedPeriod = nextBudgetUpdatePeriod - (WEEK * 2);
      Budget memory previousBudget = periodBudget[lastFinishedPeriod];
      Budget memory previousSpent = allocatedBudgetHistory[lastFinishedPeriod];
      pending.palAmount += previousBudget.palAmount - previousSpent.palAmount;
      pending.extraAmount += previousBudget.extraAmount - previousSpent.extraAmount;
  
      // Save the new set budget
      periodBudget[nextBudgetUpdatePeriod] = pending;
  
      nextBudgetUpdatePeriod += WEEK;

• } }

[Kogaroshi]

My issue with the proposed fix is that the mapping periodBlockCheckpoint will be filled with incorrect block.number to fetch data in the case of allocations.

[GTH1235]

Make _findBlockNumberForTimestamp function in HolyPalPower as public and use it.

function _findBlockNumberForTimestamp(uint256 timestamp) internal view returns(uint256) {
      uint256 deltaBlocks = block.number - ANCHOR_BLOCK;
      uint256 deltaTs = block.timestamp - ANCHOR_TIMESTAMP;

      uint256 secPerBlock = (deltaTs * SCALE_UNIT) / deltaBlocks;

      return block.number - (((block.timestamp - timestamp) * SCALE_UNIT) / secPerBlock);
  }

Or make the same function in LootCreator.

function _updatePeriod() internal {
    while (nextBudgetUpdatePeriod <= block.timestamp) {
-         periodBlockCheckpoint[nextBudgetUpdatePeriod] = block.number;
+         periodBlockCheckpoint[nextBudgetUpdatePeriod] = _findBlockNumberForTimestamp(nextBudgetUpdatePeriod);
    }
}

@Kogaroshi , what do you think?

[Kogaroshi]

Since we also use _findBlockNumberForTimestamp to find the user lock for a given timestamp, we can use the same logic to find the block.number at which we want to get the totalLocked, since in both case we'd have the same block.number so a better consistency. I'll work on a fix with those changes.

[Kogaroshi]

Changes made in https://github.com/PaladinFinance/Vote-Flywheel/pull/2/commits/3b0fe63eeb38718fb96d06329012cde349de8c38 All methods are not fully tested now, will add new tests later on.