Changing producer in `PirexRewards` leaves some rewards unclaimable

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L93-L99 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L346-L347

Vulnerability details

Impact

In case a new producer doesn't support all producer and reward tokens of a previous producer, some rewards can be left unclaimed when producer is changed. The amount of unclaimed rewards depends on how much rewards were accumulated between the last call to harvest and the call to setProducer.

Proof of Concept

A producer is a contract that lets users deposit tokens to earn staking rewards. At the moment, there's only one producer: PirexGmx. The contract is linked to PirexRewards via the producer state variable (PirexRewards.sol#L28), which can be changed by the owner (PirexRewards.sol#L93-L99). A producer is called by PirexRewards to harvest rewards (PirexRewards.sol#L346-L347) before claiming them (PirexRewards.sol#L377).

If a new producer (that's set by the owner in the setProducer function) doesn't support all producer and reward tokens from the previous producer, it cannot be used to claim such reward tokens. The call to claimRewards will not claim rewards in some reward tokens of the previous producer and will return different lists of producer and reward tokens, which won't allow to update the reward states of the missing tokens (PirexRewards.sol#L361). As a result, the rewards in the missing producer/reward tokens won't be harvested and distributed to users.

Tools Used

Manual review

Recommended Mitigation Steps

In the setProducer function, consider calling harvest before setting a new producer to ensure all current rewards were harvested:

--- a/src/PirexRewards.sol
+++ b/src/PirexRewards.sol
@@ -93,6 +93,7 @@ contract PirexRewards is OwnableUpgradeable {
     function setProducer(address _producer) external onlyOwner {
         if (_producer == address(0)) revert ZeroAddress();

+        harvest();
         producer = IProducer(_producer);

         emit SetProducer(_producer);

[c4-sponsor]

drahrealm marked the issue as disagree with severity

[drahrealm]

Related to issue #271, provided that the full migration procedure is correctly performed, there should be no need to call harvest before updating procedure as it should have been done prior to the call itself.

[Picodes]

Downgrading to low as:

• the issue is a potential configuration error in an update
• the issue is related to another issue by the same warde,

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-11-redactedcartel-findings/issues/277