`reward_rates` are not updated when the owner stops the farm manually

[hats-bug-reporter[bot]]

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0x410b39558de94864acf38519e588594fc1822a1230f664933c4e33a23bd92c97 Severity: medium

Description: Description\

When the owner of the farm calls stop_farm, the end is set to the block.timestamp. The problem is that the users deposit tokens into pool based on the reward_rate that is set in the initial params. And this param is determined by reward_amount of the reward token that is transferred by the owner divided by the duration. So in the case when the farm is stopped via stop_farm, the reward_rate actually stays the same and the users are supposed to withdraw only the amount that is accrued over the period when the farm was active.

Attack Scenario\

The reward_rate is set when the farm is deployed:

https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/blob/main/farm/contract/lib.rs#L227-232

 let reward_rate = reward_amount
                    .checked_div(duration)
                    .ok_or(FarmError::ArithmeticError(MathError::DivByZero(3)))?;

                reward_rates.push(reward_rate);
            }

duration is determined as this:

https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/blob/main/farm/contract/lib.rs#L209

 let duration = if let Some(duration) = end.checked_sub(start)

So when the owner calls stop_farm(), the duration is changed, but the users will only be able to claim the rewards based on the old reward_rate. Only the existence of this function allows for reward_rate and duration manipulation where the users come into the farm with one reward_rate but, in fact, it should be other if the farm is stopped before the actual &self.end:

https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/blob/main/farm/contract/lib.rs#L298-303

 fn owner_stop_farm(&mut self) -> Result<(), FarmError> {
            ensure!(self.env().caller() == self.owner, FarmError::CallerNotOwner);
            self.update()?;
            self.end = self.env().block_timestamp();
            Ok(())
        }

Recommendations

Change the stop_farm() implementation so that the reward_rate parameter is dynamically changed based on the duration of the farm.

[deuszx]

In update we check correct start and end dates for the reward accruing. In this particular case, after the farm is already stopped, the now will be always set to farm's end - not current block timestamp - so the rewards will be paid out only for the time when the farm was actually active.

[rodiontr]

@deuszx this is exactly the issue, the reward_rates are determined using the initial duration, so for lower duration, the users should have more tokens, according to the formula. But the possibility that the farm can stop before creates a situation where the users will have the rewards for the reward_rate that is set in the initial params but the duration is actually smaller. So this effectively allows to deceive the users by providing greater reward_rate in the beginning and then changing the duration

[deuszx]

Yes, that is true. But it is also correct, intended design. The point of allowing farm owner to stop the farm prematurely is to let him fix any issues he might have done when creating the farm initially. Since the farmers are rewarded for the time they were providing the liquidity (as per the initial conditions), they are not really loosing any rewards.

We consider this mechanism to be an "escape hatch for problems", not a way to steal from farmers.

[rodiontr]

@deuszx agreed that there is no explicit funds loss but it's still a low vulnerability at least as, in any case, the users will lose the interest not depending on whether the owner has good intentions or bad - calling this function just violates the parameters and reward_rates and this can be still considered a rewards loss

[deuszx]

Maybe, but that is a design choice we made on purpose.

[deuszx]

Adding the wontfix label as we see the behaviour, described in this submission, as not a bug but a conscious design choice. If you still disagree, provide a PoC for the issue.

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue is INVALID.

We hope you participate in the future audits of ink!.