`stop_farm()` doesn't check whether the farm is already stopped

[hats-bug-reporter[bot]]

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

Description: Description\

stop_farm function allows the owner to stop the farming at any given moment. It makes end of the farm to be set to the current block.timestamp. The problem is that, due to improper input validation, the stop_farm() can be called repeatedly and the farm end can be repeatedly updated meaning that the farm is not actually stopped by using this function. This also makes the params that set in the owner_start_new_farm being senseless as they can be changed at any given moment.

Attack Scenario\

Let's say the end is set to the current block.timestamp. So the owner calls stop_farm and this happens:

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(())
        }

Now, 2 days later, the owner can call this func again, and, due to insufficient validation, he can do it and end timestamp is updated to the current block_timestamp meaning the farm is active again. This could cause different inconveniences in the future if there'd be the checks for the farm being active.

Recommendation

Make sure that the stop_farm eventually stops the farm and it could not be redeployed again with the new block_timestamp:

+ensure!(is_stopped(), FarmError::AlreadyStopped)

[deuszx]

Stopping the farm sets the new end timestamp for the farm. The point of stopping the farm is to stop rewards payout/accrue. The window for which rewards are paid out is defined by farm.start and farm.end. In the update function, we calculate the correct time window for which the rewards are paid for. The fact that owner can stop farm repeatedly does not affect the protocol's security or correctness. Unless you can prove that it does. If so, please provide a PoC file with a test that reproduces the problem.

[rodiontr]

@deuszx yes but it also doesn't update the farm.start to the date when it's repeatedly called meaning that the window will just increase and increase and the deltaReward may have some significantly great values which the owner will be unable to fill in with tokens

so it's better to set a new start and end if the farm is repeatedly deployed than infinitely update the end timestamp

[deuszx]

Let's walk through what would happen:

• Farm start 10, farm end 200, current block 100.
• Owner calls stop_farm:
  • (in update) the timestamp_at_last_update = 100.
  • (in owner_stop_farm) the self.end = 100.
• Farm start 10, farm end 100, current block 110
  • (here) let prev = core::cmp::max(self.timestamp_at_last_update, self.start); resolves to 100
  • (here) let now = core::cmp::min(current_timestamp, self.end); resolves to 100
  • (here) since prev == now we will short circuit, return Ok(()) and exit the function, without updating rewards earned etc.
  • (here) we would update the self.end = 110

In conclusion, we will not pay any more rewards that were originally meant for the liquidity providers (w/o updating cumulative_reward_rate we're not actually paying out the rewards).

We do seem, however, to unnecessarily update the farm's end (after the farm is finished) which can cause "informational confusion" (for example when presenting this in the frontend). If you agree with the analysis above - please resubmit as minor w/ the new description. If you still disagree, please provide a PoC file with a test that reproduces the issue.

[deuszx]

Please provide the PoC demonstrating the vulnerability.

[rodiontr]

@deuszx this is invalid too but check out new issue #40 - seems to work for me

[deuszx]

Thank you for the submission. After carefully reviewing it we've decided to accept it as VALID and award it the MINOR severity level.

Submission correctly points out inconsistency in the contract but it does not affect the users' funds (ie no funds are at loss, risk of freezing or protocol insolvency).

We hope to see you in the future challenges of ink! codebase.