Rewards are not fully distributed for the farming period

[hats-bug-reporter[bot]]

Github username: @rodiontr Twitter username: -- Submission hash (on-chain): 0x5d2df285a3916c2d5140a328b4d7e96bd0063997ed04c121a19fd8411b7f0f61 Severity: high

Description: Description\

The rewards for the farming are not fully distributed to the users as the reward amount will be equal to the duration and not to the actual rewards amount.

Attack Scenario\

Let's say the owner starts farm with 100000 rewards and makes the duration equal to 86_400 (one day). The problem is that the farmer (ALICE, see PoC below), will get only 86400 tokens after the end of the period instead of getting the whole 100000 amount as she should being the only farmer.

Attachments

PoC:

Add this to your test.rs

#[drink::test]
fn rewards_distribution_fail() {

    // initialize the session
    let mut session: Session<MinimalRuntime> = Session::new().expect("Init new Session");

    // set up the necessary tokens (ICE(lp), WOOD(reward))
    let ice = setup_psp22(&mut session, ICE.to_string(), ICE.to_string(), BOB);
    let wood = setup_psp22(&mut session, WOOD.to_string(), WOOD.to_string(), BOB);

    // transfer native tokens to ALICE so she can execute txs
        session
        .sandbox()
        .mint_into(ALICE, 1_000_000_000u128)
        .unwrap();

    // set up the farm with ICE as the pool token and WOOD as a reward token
    let farm = setup_farm(
        &mut session,
        ice.into(),
        vec![wood.into()],
        BOB,
    );

    let deposit_amount = 1000;

    // transfer tokens to ALICE so he can deposit
    transfer_tokens(&mut session, ice.into(), alice(), deposit_amount, BOB).unwrap();
    assert!(balance_of(&mut session, ice.into(), alice()) == deposit_amount);

     // fetching up the timestamp
     let now =  get_timestamp(&mut session);
     set_timestamp(&mut session, now);

     // setting up start, end and the rewards amount
     let farm_start = now;
     let farm_end = farm_start + 86400;
     let rewards_amount = 100000;

     // deposit WOOD tokens into the farm
     increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);

    // starting farming
    let call_result = setup_farm_start(
        &mut session,
        &farm,
        farm_start,
        farm_end,
        vec![rewards_amount],
        BOB,
    );
    assert!(call_result.is_ok());

    // farm ends
    set_timestamp(&mut session, farm_end);

    // fetch the balanceOf() WOOD token of ALICE before the claiming
    println!("Balance of ALICE before claiming: {}", balance_of(&mut session, wood.into(), alice()));

    // ALICE claims the rewards
    let call_result = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        ALICE);

    // fetch the balanceOf() WOOD token of ALICE after the claiming
    println!("Balance of ALICE after claiming: {}", balance_of(&mut session, wood.into(), alice()));

}

The test gives the following output:

This means that ALICE had 0 tokens before the claim and only 86400 after that but the amount claimed should be equal to 100000.

[rodiontr]

Sorry, the output didn't appear for some reason:

running 1 test
Balance of ALICE before claiming: 0
Balance of ALICE after claiming: 86400
.
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.88s

[deuszx]

Thanks for the submission. Similarly to my comment on #45 - this is a duplicate finding of #44 .

Advise for the future - when you write a test, it should contain assertions that fail if the output is not what's expected. In your case, it ends with printlns. If you replace it with:

// ALICE claims the rewards
    let rewards_claimed = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        ALICE).unwrap();

assert_eq!(vec![100000], rewards_claimed);

it will fail with a mismatch. If you move your test on top of https://github.com/Cardinal-Cryptography/common-amm/pull/84 it will pass.

EDIT: you'll need to fix your test first as in the one attached Alice is not a farmer:

Balance of ALICE before claiming: 0
thread 'hats::rewards_distribution_fail' panicked at 'called `Result::unwrap()` on an `Err` value: CallerNotFarmer', src/hats.rs:908:81

[rodiontr]

Thanks for the submission. This is a duplicate finding of #44 .

Advise for the future - when you write a test, it should contain assertions that fail if the output is not what's expected. In your case, it ends with printlns. If you replace it with:

// ALICE claims the rewards
    let rewards_claimed = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        ALICE).unwrap();

assert_eq!(vec![100000], rewards_claimed);

it will fail with a mismatch. If you move your test on top of Cardinal-Cryptography#84 it will pass.

EDIT: you'll need to fix your test first as in the one attached Alice is not a farmer:

Balance of ALICE before claiming: 0
thread 'hats::rewards_distribution_fail' panicked at 'called `Result::unwrap()` on an `Err` value: CallerNotFarmer', src/hats.rs:908:81

thanks for the advice, i've tried doing assertEq! with claimed rewards but it always returns 0 for me for some reason I don't know so i just use println! hope it's ok. Concerning ALICE being not a farmer: I don't know what the hell happened when I uploaded submission but it was a deposit from ALICE in my initial test, otherwise I wouldn't get this output and the test would fail

[rodiontr]

@deuszx lol we silently missed #44 yesterday when we discussed #43:

Bob should have accumulated rewards_amount of rewards, not 86400. rewards_amount = 10_000.

[deuszx]

@deuszx lol we silently missed #44 yesterday when we discussed #43:

Bob should have accumulated rewards_amount of rewards, not 86400. rewards_amount = 10_000.

I think the two are unreleated. #43 states that a huge deposit after the farm ends affects the previous farms' rewards. That statement is untrue. Maybe you've observed something that in its core is really #44 but only #44 correctly pointed out the problem.

[deuszx]

Thank you for the submission. After carefully reviewing it we've decided to mark it as INVALID.

PoC should refer to the actual point where the issue lies in and why. It's not enough to describe one of many scenarios where things break, without pointing out why.

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