Medium risk (M3): in `changeDistributor` the distributor for an active gauge can be changed without updating the rest of storage values

[petrovska-petro]

Severity: leads to revert in performUpkeep

Context: currently the owner has full freedom to update the distributor of one of the gauges within the list of the injector at any moment. see:

function changeDistributor(
    address gauge,
    address reward_token,
    address distributor
) external onlyOwner {
    IChildChainGauge(gauge).set_reward_distributor(
        reward_token,
        distributor
    );
}

Recommendation: changing distributor for an active gauge without updating ActiveGauges and/or GaugeConfigs will lead to revert when performUpkeep or injectFunds is executed.

It will reach the logic of injecting the token into the gauge calling:

gauge.deposit_reward_token(
    address(token),
    targetConfig.amountPerPeriod
);

Which after changing the distributor not to address(this) will simple revert during the loop and will be tricky revert to track down unless it is gracefully handled.

Pseudocode suggestion:

  function changeDistributor(
      address gauge,
      address reward_token,
      address distributor
  ) external onlyOwner {
+     if (ActiveGauges.contains(gauge) {
      IChildChainGauge(gauge).set_reward_distributor(
          reward_token,
          distributor
      );
+   1) remove from ActiveGauges ->> similar logic as in "removeRecipients"
+     } else {
+      IChildChainGauge(gauge).set_reward_distributor(
+          reward_token,
+          distributor
+      );
+    }
  }

Review stage

Balancer Maxis:

• [x] Acknowledge
• [ ] Fixed

Onchainification Labs:

• [x] Acknowledge correction/mitigation

[Tritium-VLK]

The schedule should not change unless a user explicitly changes it. There is also a UI view that shows what is going on. If one changes distributor, one may also want to copy the schedule for example, or see that it is there.

There is no real harm done in these reverts. There are other situatuiosn where this can happen as well I think (distributor wasn't setup in the first place, governance changes distributor).

This is a helper to automate some manual tasks (paying in every week on time), that is 100% owned by a single owner. It was designed to create flexibility.

Could consider adding a check in get ready gauges to make sure we are distributor, if it's not already there.

[petrovska-petro]

Checking getReadyGauges has a condition to check if gauge.reward_data(tokenAddress).distributor == address(this), which will be enough potentially to not revert in this case.

But, still it is a good practice to keep the storage EnumerableSet.AddressSet internal ActiveGauges; up to date if indeed a distributor is changed to another injector or other contract via changeDistributor.

Otherwise, after calling changeDistributor the ActiveGauges will return gauges where the injector it is not anymore an actual distributor

[Tritium-VLK]

If the injector were to become the distributor, this gauge would immediately pay out, so it is important that they are listed.

The decision to change a distributor is something that could also be made by governance/externally. A schedule line here is an instruciton to pay in funds as soon as there is no flows at is possible. If that schedule is no longer desired it should be removed. Again, we are the primary user of this. Perhaps it makes sense to go through these issues and write some docs/a faq if we intend to show this to external users. Most externals will be using a UI, that will not offer any of the extended capacity around changing distributors, manual injections, etc.

[petrovska-petro]

From what we gather from the feedback is that primarly the maxis will handle the ops of these admin methods and in the case that:

1. a changeDistributor is called an injector is not anymore the distributor for gauge[i]; it's not a concern since the team will handle 2)
2. will also remove irrelevant schedules from the existing storage avoiding reverts during a performUpkeep call

if that's the case this may not be a M3, but we consider that operational methodology can be more prone to error, instead of handling directly at the core logic in changeDistributor as we suggested in the initial *diff**

Indeed, adding these internal operational flows into the README/docs would provide value, since other external team not as familiar as the maxis running the injector can fall into operational mistakes

marking as "Acknowledge correction/mitigation" and closing the issue as we understand that internal operations will not lead into reverts ✅