Users can get less rewards if there is a huge deposit after the farm has ended

[hats-bug-reporter[bot]]

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

Description: Description\

The users can get less rewards than they should if there is a huge deposit made by anybody after the farm ends. This problem is possible due to reliance on total_shares

Attack Scenario\

Let's say the user1 (BOB) deposited money at the start of the farm and after a while it's current_timestamp = end so the farm basically ended. However, deposit() function allows to deposit into the farm that's already finished. User2 (ALICE) will not get any additional rewards right away by doing this, as update() and update_account() are called before the total_shares update. So ALICE just deposits and waits. BOB, however, will call claim_rewards() and, at this time, total_shares variable is different and deltaReward will be recalculated basically for the same period of time (start - end). By doing this, the rewards for the BOB will be artificially decreased by ALICE even though she didn't even participate in the farming. After BOB claims his rewards, she can just wait a bit and then withdraw her rewards.

Attachments

PoC:

add this to your test.rs:

use crate::*;
use utils::*;

use farm::Farm as _;

use drink::{runtime::MinimalRuntime, session::Session};
use ink_wrapper_types::Connection;

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

    // 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);

    // 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;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        BOB);

    assert!(call_result.is_ok());

     // 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;
     increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);
    // starting the new farm
    // duration = 86400
    // reward_rate = 1.157 tokens

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

    let bob_wood_balance_before = balance_of(&mut session, wood.into(), bob());

    // when it stops, how many rewards the user should have ?
    // 10 000 (has passed) * 1.157 / 1000 = 11.57 tokens - delta reward
    // delta_reward_distributed = 11.57 * 1000 = 11570 
    set_timestamp(&mut session, now + farm_end);

    let alice_wood_balance_before = balance_of(&mut session, wood.into(), alice());

    let deposit_amount_2 = 50000;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, ALICE);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        ALICE);

    assert!(call_result.is_ok());

    let call_result = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        BOB,
    );

    assert!(call_result.is_ok());

    let bob_wood_balance_after = balance_of(&mut session, wood.into(), bob());

    // only the rewards after setting up the farm again are accrued
    // the rewards for BOB should be less than expected if there were no any deposits after the farm ended
    println!("{}", bob_wood_balance_after - bob_wood_balance_before);

}

And also this to your utils.rs:

pub fn claim_from_farm(
    session: &mut Session<MinimalRuntime>,
    farm: &Farm,
    tokens: Vec<u8>,
    caller: drink::AccountId32,
) -> Result<()> {
    let _ = session.set_actor(caller);

    session
        .execute(farm.claim_rewards(tokens))
        .unwrap()
        .result
        .unwrap()
        .unwrap();

    Ok(())
}

pub fn deposit_to_farm(
    session: &mut Session<MinimalRuntime>,
    farm: &Farm,
    amount: u128,
    caller: drink::AccountId32,
) -> Result<()> {

    let _ = session.set_actor(caller);

    session
        .execute(farm.deposit(amount))
        .unwrap()
        .result
        .unwrap()
        .unwrap();

    Ok(())
}

And also the second caller:

pub const ALICE: drink::AccountId32 = AccountId32::new([2u8; 32]);

[deuszx]

Thanks for the submission @rodiontr . Again, your test has no assertions checking whether it run correctly, it's just a println! at the end. Please add the correct assertions to the test so that it's clear what is expected vs what actually happens.

[rodiontr]

@deuszx

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

    // 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);

    // 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;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        BOB);

    assert!(call_result.is_ok());

     // 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;
     increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);
    // starting the new farm
    // duration = 86400

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

    // fetch bob balance right after the deposit
    let bob_wood_balance_before = balance_of(&mut session, wood.into(), bob());

    let deposit_amount_2 = 50000;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, ALICE);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        ALICE);

    assert!(call_result.is_ok());

    let call_result = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        BOB,
    );

    assert!(call_result.is_ok());

    // BOB balance after he claims the rewards
    let bob_wood_balance_after = balance_of(&mut session, wood.into(), bob());

    // only the rewards after setting up the farm again are accrued
    // the rewards for BOB should be less than expected if there were no any deposits after the farm ended
    println!("{}", bob_wood_balance_after - bob_wood_balance_before);

    // the bob should have accumulated 86400 tokens for the whole period
    // so this should be equal to this amount as ALICE didn't participate in farming
    assert!((bob_wood_balance_after - bob_wood_balance_before) == 86400);

}

[deuszx]

@rodiontr you never update the timestamp to farm_end so the farm never really starts.

[rodiontr]

@rodiontr you never update the timestamp to farm_end so the farm never really starts.

sorry, what do you mean, don't get the point

UPD: got it

[rodiontr]

@rodiontr you never update the timestamp to farm_end so the farm never really starts.

forgot about this, updated:

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

    // 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);

    // 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;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        BOB);

    assert!(call_result.is_ok());

     // 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;
     increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);
    // starting the new farm
    // duration = 86400

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

    // fetch bob balance right after the deposit
    let bob_wood_balance_before = balance_of(&mut session, wood.into(), bob());

    set_timestamp(&mut session, farm_end);

    let deposit_amount_2 = 50000;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount_2, ALICE);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount_2,
        ALICE);

    assert!(call_result.is_ok());

    let call_result = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        BOB,
    );

    assert!(call_result.is_ok());

    // BOB balance after he claims the rewards
    let bob_wood_balance_after = balance_of(&mut session, wood.into(), bob());

    // only the rewards after setting up the farm again are accrued
    // the rewards for BOB should be less than expected if there were no any deposits after the farm ended
    println!("{}", bob_wood_balance_after - bob_wood_balance_before);

    // the bob should have accumulated 86400 tokens for the whole period
    // so this should be equal to this amount as ALICE didn't participate in farming
    assert!((bob_wood_balance_after - bob_wood_balance_before) == 86400);

}

[deuszx]

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

[rodiontr]

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

yes, but rewards_amount are actually 100 000. And I don't know why but if you run the test with only BOB, this is would be the rewards_amount he'll get eventually (after this duration) if there is no interruption. Maybe that's something about how reward rates are calculated but i can provide you with PoC only for BOB

[deuszx]

You get these inconsistencies b/c your test is written badly - you're minting huge amounts of WOOD to BOB and then make him the farmer, that earns rewards in ... WOOD as well. At the end you're checking his balance, instead of how much rewards he's actually claming (btw claim_rewards returns that info), and it includes both his rewards and the WOOD balance from initial mint.

[rodiontr]

You get these inconsistencies b/c your test is written badly - you're minting huge amounts of WOOD to BOB and then make him the farmer, that earns rewards in ... WOOD as well. At the end you're checking his balance, instead of how much rewards he's actually claming (btw claim_rewards returns that info), and it includes both his rewards and the WOOD balance from initial mint.

but i fetch his balance right after the farm is set up so the rewards_amount are substracted from his balanceOf() as they are transferred to the contract, then at the end of the period he claims and the result should be his previous balance from initial mint:

  let wood = setup_psp22(&mut session, WOOD.to_string(), WOOD.to_string(), BOB);

and + total rewards that he's earned because he's the only one in the pool

[rodiontr]

so he just sends rewards and the after balanceOf() should be his previous balance + this rewards amount again if it makes sense because all the tokens go to him

[deuszx]

claim_from returns rewards earned, check if that's your expected 10_000 - b/c that's how much reawrds he would have earned if he was the only farmer for the whole farm duration:

    let deposit_amount = 1000;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        BOB);

[rodiontr]

claim_from returns rewards earned, check if that's your expected 10_000 - b/c that's how much reawrds he would have earned if he was the only farmer for the whole farm duration:

    let deposit_amount = 1000;

    increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);

    let call_result = deposit_to_farm(
        &mut session,
        &farm,
        deposit_amount,
        BOB);

but rewards_amount == 100_000 and not 10_000 so if he's the only farmer he claims all the rewards. Why 10_000 ?

[deuszx]

You're right, 100_000. Check if that's what you get out of claim_rewards:

let rewards = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        BOB,
    ).unwrap();

    assert!(rewards == vec![rewards_amount]);

[rodiontr]

You're right, 100_000. Check if that's what you get out of claim_rewards:

let rewards = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        BOB,
    ).unwrap();

    assert!(rewards == vec![rewards_amount]);

nah still fails

[deuszx]

Did you run your test? It's missing crucial calls to make it even run until the end:

1. You're not seeding ALICE with native tokens, so on the first increase_allowance call from her account it's failing with StorageDepositLimitExhausted.
2. Second, nowhere you're transferring WOOD tokens to her, so ALICE cannot deposit to the farm.

EDIT: after I fixed it, I removed the ALICE's deposit and it doesn't affect the outcome of the test, so it definitely isn't the problem you're submitting - i.e. "huge deposit after farm ends DOES NOT affect the rewards". The test passes with and without Alice's deposit.

[rodiontr]

Did you run your test? It's missing crucial calls to make it even run until the end:

1. You're not seeding ALICE with native tokens, so on the first increase_allowance call from her account it's failing with StorageDepositLimitExhausted.
2. Second, nowhere you're transferring WOOD tokens to her, so ALICE cannot deposit to the farm.

EDIT: after I fixed it, I removed the ALICE's deposit and it doesn't affect the outcome of the test, so it definitely isn't the problem you're submitting - i.e. "huge deposit after farm ends DOES NOT affect the rewards". The test passes with and without Alice's deposit.

why when I asked you yesterday about this error, you said that everything was fine with my test?

[deuszx]

This issue is 4hours old so I couldn't have answered to you yesterday. You asked me about a different one.

Still, you're not answering my question - did you even run your test?

[rodiontr]

This issue is 4hours old so I couldn't have answered to you yesterday. You asked me about a different one.

Still, you're not answering my question - did you even run your test?

i ran it but it failed due to "StorageDeposit" error

[rodiontr]

so i just provided you with PoC so you merely have the idea of the attack because when I provide you with numbers and calculations it's hard to grasp. If you told me that I needed to mint the native tokens and it'd fix the error, you could save me so much time man. ok whatever it's invalid

[deuszx]

I never would have guessed that you're dishonest, submitting a PoC that is not working - which you didn't mention - and acted as if it was working but failing, exposing an issue with the contract. My mistake.

[rodiontr]

I never would have guessed that you're dishonest, submitting a PoC that is not working - which you didn't mention - and acted as if it was working but failing, exposing an issue with the contract. My mistake.

I never would have guessed that you would fake the interest in securing the protocol and don't help the auditors at least with writing tests, with something there is so little info about. Yesterday when I was talking to you, I asked:

however, this may be some session issues as I still don't know how to prank the second caller (ALICE) using drink. And because of that, the test is not incorrectly executed. It seems to always fail due to "DepositLimit" error

And you answered:

Check this PoC for reference https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/issues/37 . It does what you're looking for.

How should I know that I need to mint native tokens to pass this error ? Why you just don't say this straight ? And when I asked you on discord about this error like 5 days ago - you said "i am not going to do the job for you". That's not doing the job for somebody else that's just saying general info you cannot find anywhere else because you didn't write any tests that cover up the whole functionality of the protocol.

And I was genuinely interested in your protocol because that's my first rust audit at all and you can see this by how many issues I submitted - it was not just about money, it was pure interest

So I don't blame you as I am guilty as well but don't talk like I am the baddest person in the world

[rodiontr]

And I was not trying to deceive you, I was trying to provide you with the way you understand the attack vector

[deuszx]

I never would have guessed that you would fake the interest in securing the protocol and don't help the auditors at least with writing tests, with something there is so little info about.

You've opened 9 issues, under which I've left dozens of comments, advices, fixed your code snippets multiple times, pointed at code in contracts and other issues that are already doing what you wanted. I think no one will say that was a fake interest.

---

however, this may be some session issues as I still don't know how to prank the second caller (ALICE) using drink. And because of that, the test is not incorrectly executed. It seems to always fail due to "DepositLimit" error

Check this PoC for reference https://github.com/hats-finance/AlephZeroAMM-0x0d88a9ece90994ecb3ba704730819d71c139f60f/issues/37 . It does what you're looking for.

Yes, that is a complete answer. I think you didn't read the submission carefully b/c if you did, you would have figured out that it's using multiple agents - like you needed to. It also has this clear line with a comment:

    // feed charlie some native tokens

followed by the code yours was missing.

---

you said "i am not going to do the job for you".

Yes, you're the warden that's trying to get a reward. Guidelines of the audit clearly state that a working PoC is required.

[rodiontr]

I never would have guessed that you would fake the interest in securing the protocol and don't help the auditors at least with writing tests, with something there is so little info about.

You've opened 9 issues, under which I've left dozens of comments, advices, fixed your code snippets multiple times, pointed at code in contracts and other issues that are already doing what you wanted. I think no one will say that was a fake interest.

however, this may be some session issues as I still don't know how to prank the second caller (ALICE) using drink. And because of that, the test is not incorrectly executed. It seems to always fail due to "DepositLimit" error

Check this PoC for reference #37 . It does what you're looking for.

Yes, that is a complete answer. I think you didn't read the submission carefully b/c if you did, you would have figured out that it's using multiple agents - like you needed to. It also has this clear line with a comment:

    // feed charlie some native tokens

followed by the code yours was missing.

you said "i am not going to do the job for you".

Yes, you're the warden that's trying to get a reward. Guidelines of the audit clearly state that a working PoC is required.

Yes, but there is no info about this error - maybe I don't understand the role of the sponsors in this protocol, but on other platforms they try to help with any questions regarding the functionality or testing environment. I was not asking to write a submission for me or find the issue for me, I was just asking about testing environment that's basically new and it's not something like Foundry where there are tons of examples and videos. I spent about 2 days trying to find out what's wrong with that issue - that's just time without taking into consideration time spent finding the vulnerabilities. So I really tried my best. Sorry it turned out like this. And thank you for answering on all the questions (I do appreciate it) but you could have just said straight and we wouldn't have these long long discussions

[coreggon11]

@rodiontr if you read the panic message of the test and write it to google you would find out that the actor needs native tokens…

[rodiontr]

@coreggon11 yeah I saw that issue on stackexchange that you are probably talking about. It just says "your contract doesn't have enough funds". So i am writing my submission and trying to increaseAllowance() and deposit tokens from ALICE. So I have a question "what funds is this issue talking about ?". Moreover, I am completely new to rust and never did this before, so everything is kinda new to me. But maybe i am just dumb lol

[coreggon11]

@rodiontr I’m not at all saying you are dumb I’m just saying that if you want to be a security RESEARCHER then you have to RESEARCH (and that is also about the tools used for development of the app) :) And it’s not about Rust, when I saw the error first thing that came to my mind was “Does the sender have enough native token?” and the answer was no:) And regarding rust (and its crates like drink for example) you have often online docs regarding all the functionality of the specific crate (with drink having quite extensive one, where you would also find how tosend native balance etc. I’m writing this to be helpful, not to be mean :)

[rodiontr]

@rodiontr I’m not at all saying you are dumb I’m just saying that if you want to be a security RESEARCHER then you have to RESEARCH (and that is also about the tools used for development of the app) :) And it’s not about Rust, when I saw the error first thing that came to my mind was “Does the sender have enough native token?” and the answer was no:) And regarding rust (and its crates like drink for example) you have often online docs regarding all the functionality of the specific crate (with drink having quite extensive one, where you would also find how tosend native balance etc. I’m writing this to be helpful, not to be mean :)

thank you very much for the advice. I guess I am able to find the right answers but sometimes I just don't understand what's going on especially when it comes to something new. will try to improve that research part

[deuszx]

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

Please note that all submissions are required to include a working PoC - at the time of submission - these are the rules of the challenge. 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.