farm/lib.rs : when the farm is closed, deposit, deposit_all will not accrue any rewards

[hats-bug-reporter[bot]]

Github username: -- Twitter username: ak1 Submission hash (on-chain): 0x73f5776c4fa4eea1c3002c9af3a87c04e7d003f074d59791cb5693bfbff63c18 Severity: high

Description: Description\

The functions deposit and deposit_all is allowed to call even if the farm is closed. An user who deposit when the farm is closed will not accrue any rewards.

Attack Scenario\

User who is depositing their money will not be awarded. In case the contract is destroyed when the farm is closed, these user will lost their funds.

Attachments

1. Proof of Concept (PoC) File

below, both deposit, deposit_all allows deposit even if farm is closed

https://github.com/Cardinal-Cryptography/common-amm/blob/bf4e48e3257894dcc8e6ab359321d1406533ad8b/farm/contract/lib.rs#L371-L387

    #[ink(message)]
    fn deposit(&mut self, amount: u128) -> Result<(), FarmError> {
        let account = self.env().caller();
        self.deposit(account, amount)?;
        FarmContract::emit_event(self.env(), Event::Deposited(Deposited { account, amount }));
        Ok(())
    }

    #[ink(message)]
    fn deposit_all(&mut self) -> Result<(), FarmError> {
        let account = self.env().caller();
        let pool: contract_ref!(PSP22) = self.pool_id.into();
        let amount = pool.balance_of(account);
        self.deposit(account, amount)?;
        FarmContract::emit_event(self.env(), Event::Deposited(Deposited { account, amount }));
        Ok(())
    }

https://github.com/Cardinal-Cryptography/common-amm/blob/bf4e48e3257894dcc8e6ab359321d1406533ad8b/farm/contract/lib.rs#L241-L253

    fn deposit(&mut self, account: AccountId, amount: u128) -> Result<(), FarmError> {
        if amount == 0 {
            return Err(FarmError::InsufficientShares);
        }
        self.update()?;
        self.update_account(account);
        let mut pool: contract_ref!(PSP22) = self.pool_id.into();
        pool.transfer_from(account, self.env().account_id(), amount, vec![])?;
        let shares = self.shares.get(account).unwrap_or(0);
        self.shares.insert(account, &(shares + amount));
        self.total_shares += amount;
        Ok(())
    }

the update function will not update the rewards. https://github.com/Cardinal-Cryptography/common-amm/blob/bf4e48e3257894dcc8e6ab359321d1406533ad8b/farm/contract/lib.rs#L111-L126

    fn update(&mut self) -> Result<(), FarmError> {
        let current_timestamp = self.env().block_timestamp();
        if self.timestamp_at_last_update >= current_timestamp {
            return Ok(());
        };

        let prev = core::cmp::max(self.timestamp_at_last_update, self.start);
        let now = core::cmp::min(current_timestamp, self.end);
        if prev >= now {
            self.timestamp_at_last_update = current_timestamp;
            return Ok(());--------- @@ audit -- will return here.
        }

        // At this point we know [prev, now] is the intersection of [self.start, self.end] and [self.timestamp_at_last_update, current_timestamp]

POC : use this script.

#[drink::test]
fn deposit_after_farm_closed(mut session: Session) {

    // Set up the necessary tokens.
    // ICE - LP token
    // WOOD and SAND - reward tokens
    let ice = psp22::setup(&mut session, ICE.to_string(), ICE.to_string(), BOB);
    let wood = psp22::setup(&mut session, WOOD.to_string(), WOOD.to_string(), BOB);
    let sand = psp22::setup(&mut session, SAND.to_string(), SAND.to_string(), BOB);

    const FARM_OWNER : drink::AccountId32 = BOB;
    const FARMER : drink:: AccountId32 = ALICE;

    let farm = farm::setup(
        &mut session,
        ice.into(),
        vec![wood.into(), sand.into()],
        FARM_OWNER,
    );

    // Seed the farmer with some tokens to execute txns.
    session
        .sandbox()
        .mint_into(ALICE, 1_000_000_000u128)
        .unwrap();

    // deposits lp token
    let deposit_amount = 1000000;

    //inc_timestamp(&mut session); //------------>> need to update

    let nowA = get_timestamp(&mut session);
    set_timestamp(&mut session, nowA);

    // Deposit LP tokens as Alice, not Bob.
    psp22::transfer_ak(&mut session, ice.into(), alice(), deposit_amount, BOB).unwrap();

    psp22::increase_allowance(
        &mut session,
        ice.into(),
        farm.into(),
        deposit_amount,
        FARMER,
    );
    assert_eq!(
        farm::deposit_to_farm_ak(&mut session, &farm, deposit_amount, FARMER),
        Ok(())
    );

    // Start the first farm
    let now = get_timestamp(&mut session);
    set_timestamp(&mut session, now);

    let farm_duration = 100;
    let farm_start = now + 10;
    let farm_end = farm_start + farm_duration;
    let rewards_amount = 100000000000000;

    psp22::increase_allowance(
        &mut session,
        wood.into(),
        farm.into(),
        rewards_amount,
        FARM_OWNER,
    );
    psp22::increase_allowance(
        &mut session,
        sand.into(),
        farm.into(),
        rewards_amount,
        FARM_OWNER,
    );
    assert_eq!(
        farm::start(
            &mut session,
            &farm,
            farm_start,
            farm_end,
            vec![rewards_amount, rewards_amount],
            FARM_OWNER,
        ),
        Ok(())
    );

    // set timestamp to farm end so users earn some reward in this farm but AND contract has balance
    set_timestamp(&mut session, farm_end);
    // There's one farmer that owns all LPs and participates in both farms for the whole duration.
    // Owner withdrew WOOD tokens from the contract before the second farm started.
    // So there are no WOOD rewards for the second farm.

    set_timestamp(&mut session, farm_end + farm_duration);
    // Deposit to farm after it's finished.
    psp22::increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);
    assert_eq!(
        farm::deposit_to_farm_ak(&mut session, &farm, deposit_amount, BOB),
        Ok(())
    );
    //inc_timestamp(&mut session);
    set_timestamp(&mut session, farm_end + farm_duration + farm_duration);
    psp22::increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);
    assert_eq!(
        farm::deposit_to_farm_ak(&mut session, &farm, deposit_amount, BOB),
        Ok(())
    );
    set_timestamp(&mut session, farm_end + farm_duration + farm_duration + farm_duration);

    let result : Result<Vec<u128>, FarmError> = farm::claim_rewards_ak(&mut session, &farm, vec![0,1], BOB);

    assert_eq!(
        Ok(vec![1, 2]),
        result,
        "Rewards should be zero"
    );

}

failures:

---- tests::deposit_after_finish stdout ----
thread 'tests::deposit_after_farm_closed' panicked at 'assertion failed: `(left == right)`
  left: `Ok([1, 2])`,
 right: `Ok([0, 0])`: Rewards should be zero', src/tests.rs:219:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

2. Revised Code File (Optional)

We would suggest to not to allow sort of deposit when the farm is closed.

[deuszx]

Thanks for the report. I have to say I don't understand the issue. You're depositing to a farm when it's not active - i.e. will not be paying out any rewards - so it is expected that claim_rewards would return [0, 0]. Why is your test expecting [1, 2]?

[aktech297]

Hi @deuszx thanks for your reply.

I just wanted to show the output rewards would be zero. So, I just compared with a random non zero value.

User would not be knowing that farm is closed and would deposit to earn some rewards . Unfortunately they would not receive any rewards. One way could be not allowing deposit when then farm is closed. So that the user can deposit into another farm which is active and this would benefit to both the user and to the protocol.

[deuszx]

Let me see if I understand your submission correctly: You're saying that right now users are able to deposit when farm is closed. Correct?

If that's the only issue then it's NOT an issue. This is by design, I've explained it already but this is a design of the farm where it can be activated multiple times by the farm owner adding more rewards to it and farmers don't need to moving their LPs.

[aktech297]

Let me see if I understand your submission correctly: You're saying that right now users are able to deposit when farm is closed. Correct?

If that's the only issue then it's NOT an issue. This is by design, I've explained it already but this is a design of the farm where it can be activated multiple times by the farm owner adding more rewards to it and farmers don't need to moving their LPs.

It's interesting .. thanks for sharing.

But still this could be cause of concern for user who wants to deposit and earn rewards.

Some of the issues that a user would encounter are,

First they will not be updated with any rewards since the farm is closed. . Second, this deposit would influence the user rewards when calculating the user_rewards_per_share where the total share is used to calculate. Note that the total share is still accrued whether the farm is open or closed. I beleive the second point would really cause of concern.

The way I would see to fix is, maintain the deposit in seperate variable and use it when the farm is opened again or simply not allowing the deposits.

[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!.

[aktech297]

@deuszx thanks for your feedback.. However I am wondering to see such a behaviour where the user will not be awarded any rewards for their staking.

[coreggon11]

@aktech297 users are supposed to deposit when they earn rewards. If a user thinks they will get reward although the farm ended, it is not an error/vuln of the protocol. My opinion.

[deuszx]

@aktech297 , Users are earning rewards within farm activity period. Deposits are allowed even when farm is inactive b/c there may be a future farm planned so they are allowed to deposit now, at their convenience, and not required to wait until the farm actually starts.

Imagine the case where you know the farm starts today at 2AM - you don't want to wait until 2:01AM to deposit if you can do it now.

[deuszx]

Users are expected to understand what they're doing and not use contracts blindly. Frontend will show the necessary info to the user. If user wants to interact with contract directly then it's expected to understand the protocol.

[aktech297]

I think this case would be really cause of concern.

Second, this deposit would influence the user rewards when calculating the user_rewards_per_share where the total share is used to calculate. Note that the total share is still accrued whether the farm is open or closed. I beleive the second point would really cause of concern.

Overall the rewards value will be influenced by the deposits which is made during the inactive period. I think, less rewards would be calculated

[aktech297]

I think this case would be really cause of concern.

Second, this deposit would influence the user rewards when calculating the user_rewards_per_share where the total share is used to calculate. Note that the total share is still accrued whether the farm is open or closed. I beleive the second point would really cause of concern. @deuszx any thoughts on this issue?

[deuszx]

I'm not sure what's the concern here - users are expected to interact with the protocol in a certain why. Misuse is not a vulnerability.

To your second point - it wasn't the core of the original submission so I don't have to discuss it here, there was a time to submit vulnerabilities.

[aktech297]

Interesting to see your thoughts on the issues . At one time it was towards the implementation and in other time it was towards user error. Really confused here with your points

I am not sure how it would be misusing.

When there is a function to notify the activeness of the pool, still the contract is allowing user to deposit. I would say that the protocol is misusing the user .. not the user misusing the protocol. I believe you would get the difference.

Note: the second point is already made when the contest is ongoing.

[deuszx]

@aktech297 if you're disagreeing with the decision, please take it up with HatsFinance directly. There's a mediation process for this.