Medium risk (M2): it is possible to remove recipients half-way during the rewards distribution in `removeRecipients(address[])`

[petrovska-petro]

Severity: medium risk. It may lead to not distributing the expected amount to specific gauge if it is removed by the owner without noticing that whole epoch was not finalised.

Context: under the current architecture there is not way to stop a owner of removing a gauge half-way during an reward distribution epoch, which may not be desired.

function removeRecipients(address[] calldata recipients) public onlyOwner {
    for (uint i = 0; i < recipients.length; i++) {
        if (ActiveGauges.remove(recipients[i])) {
            GaugeConfigs[recipients[i]].isActive = false;
            emit RecipientRemoved(recipients[i]);
        } else {
            revert RemoveNonexistentRecipient(recipients[i]);
        }
    }
}

Simply checks if it is within the storage list and removes or revert otherwise.

Recommendation: check before removing a specific gauge, that indeed the whole epoch or expected rewards were indeed distributed already. Otherwise, it may lead to mismanagement at the operational level on the integrator, since removing from storage and mark it as non active, during the checkUpkeep will not be retrieve as a ready gauge and nothing will be distributed to that gauge.

A potential solution is to leverage the existing information within the struct Target to determine if indeed all periods were finalised succesfully and then proceed to remove it from storage (adding a method such as _isGaugePeriodsDistributed(address gauge)), and/or consider adding a parameter if it's desired by design to remove a gauge from the list half-way such as:

+ function removeRecipients(address[] calldata recipients, bool forceRemoval) public onlyOwner {
    for (uint i = 0; i < recipients.length; i++) {
        if(ActiveGauges.contains(recipients[i])) {
+           if(forceRemoval) {
+               ActiveGauges.remove(recipients[i])
+                GaugeConfigs[recipients[i]].isActive = false;
+                emit RecipientRemoved(recipients[i]);
+            } else if(_isGaugePeriodsDistributed(recipients[i])) {
+               ActiveGauges.remove(recipients[i])
+               GaugeConfigs[recipients[i]].isActive = false;
+               emit RecipientRemoved(recipients[i]);
+            }
+        }
    }
}

Review stage

Balancer Maxis:

• [x] Acknowledge
• [ ] Fixed

Onchainification Labs:

• [x] Acknowledge correction/mitigation

[Tritium-VLK]

Rewards are paid out once, and then stream for a week on the gauges. If a schedule is removed, the current epoch will continue to pay out until it is over, but the injector will not fire again to renew the next Epoch.

In this case the owner should have full access to change the schedule/manage whatever tokens are not already paid out to gauges.

Maybe this is something to think about documenting somewhere in the UI or the readme. Will assign to @Xeonus. Maybe can make another ticket and/or we can discuss.

[gosuto-inzasheru]

@Tritium-VLK i think you misunderstand. rewards are not paid out once; they are paid out once per period. if a schedule lasts for three periods, it is possible to remove a recipient during the first period, making it such that the rewards for the second and third period will not be paid out.

[Tritium-VLK]

That is by intention/design.

[Tritium-VLK]

Would renaming this function to removeSchedules() make it more clear?

[petrovska-petro]

@Tritium-VLK the least we can do it is to describe the secondary effects properly in the Natspec, so we can ensure that nobody integrating with it gets into some surprise. Naming convention still looks better as is

[Tritium-VLK]

This contract is an admin helper that is not meant to be integrated with. If you want to suggest a PR with documentation changes I'm happy to consider it. I think it's fine as is though.

[petrovska-petro]

Feedback gathered across the board in previous issues signals that the contract will be handle mostly by the maxis who know all the intricacies and internal/design decisions such as the issue explained on this issue.

Then, our advise will be to consider better documentation once this tool is more widely use by external members who are not part of the maxis, and may require better guidance on all these intricacies and design choices along the way to have smooth ops.

Marking this issue as "Acknowledge correction/mitigation" in light of feedback in https://github.com/BalancerMaxis/ChildGaugeInjectorV2/issues/22#issuecomment-2345694960 and closing it