Rounding direction In rewards_earned_by_shares cause less rewards do

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 0xmahdirostami Submission hash (on-chain): 0x15063520dc4c1b60746f5296c5b05060349c04074e93f838f1f988d3ba0430e0 Severity: medium

Description: Description\ one of the invariant of farm contract is:

        /// Total rewards that have been distributed but not yet claimed by users.
        pub farm_distributed_unclaimed_rewards: Vec<u128>,

that must be equal to all users' unclaimed reward. The rewards_earned_by_shares function has a rounding issue that leads to slight inaccuracies in reward distribution. This discrepancy results in the farm_distributed_unclaimed_rewards value not being equal to all rewards earned by users.

Impact\

likelihood: is high as it occurcs all the time for all farm contracts. Impact: Users may receive slightly lower rewards, and a small amount of rewards may be stuck in the contract. This issue is dependent on the number of users, and the frequency of calling the update_account function.

Proof of Concept (PoC) File

The provided test demonstrates that the farm_distributed_unclaimed_rewards value is not equal to the sum of all rewards earned by users. The discrepancy is due to the rounding issue in the rewards_earned_by_shares function.

        fn test_test(){
            let total_shares: u128 = 10u128.pow(20); // alice 3*10^19, bob 4*10^19, karen 2*10^19, mahdi 10^19 = 10^20
            let alice_shares: u128 = 3 * 10u128.pow(19);
            let bob_shares: u128 = 4 * 10u128.pow(19);
            let karen_shares: u128 = 2 * 10u128.pow(19);
            let mahdi_shares: u128 = 1 * 10u128.pow(19);

            assert_eq!(alice_shares + bob_shares + karen_shares + mahdi_shares, total_shares);

            let from: u128 = 0; 
            let to = 120; //120 sec
            let reward_rate= 10^16;
            let mut reward_per_share;
            let r_p_s = super::rewards_per_share_in_time_interval(reward_rate, total_shares, from, to);
            match r_p_s {
                Ok(rps) => reward_per_share = rps,
                _ => panic!(),
            }

            let total= super::rewards_earned_by_shares(total_shares, reward_per_share).unwrap_or(0);
            let alice= super::rewards_earned_by_shares(alice_shares, reward_per_share).unwrap_or(0);
            let bob= super::rewards_earned_by_shares(bob_shares, reward_per_share).unwrap_or(0);
            let karen= super::rewards_earned_by_shares(karen_shares, reward_per_share).unwrap_or(0);
            let mahdi= super::rewards_earned_by_shares(mahdi_shares, reward_per_share).unwrap_or(0);
            assert_eq!(alice + bob + karen + mahdi, total);

output:

failures:

---- farm::tests::test_test stdout ----
thread 'farm::tests::test_test' panicked at 'assertion failed: `(left == right)`
  left: `3116`,
 right: `3119`', farm/contract/lib.rs:514:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    farm::tests::test_test

Revised Code File (Optional)

Modify the rewards_earned_by_shares function to round up users' shares, addressing the rounding issue.

[deuszx]

This might look like a bug at first sight but consider the proposed alternative - rounding up - this would mean that we use more shares than the user actually has when calculating rewards. By doing that we would pay out more rewards than the user is eligible to and, in effect, "drain" the reward tokens (there wouldn't be enough rewards for everyone). Rounding down is done purposefully here, on blockchain we don't have fractions so we use big numbers to denote fractions (see the SCALING_FACTOR constant).

[0xmahdirostami]

@deuszx Thanks for your feedback, I wrote a revised code, let's review it.

    /// The formula is:
    /// rewards_per_share * shares / SCALING_FACTOR
    pub fn rewards_earned_by_shares( // @audit Rounding direction often matters when the accounting relies on user's shares.
        shares: u128,
        rewards_per_share: U256,
    ) -> Result<u128, MathError> {
        if (U256::from(shares) % U256::from(SCALING_FACTOR) == U256::from(0) ){
            rewards_per_share
                .checked_mul(U256::from(shares))
                .ok_or(MathError::Overflow(2))?
                .checked_div(U256::from(SCALING_FACTOR))
                .ok_or(MathError::DivByZero(2))?
                .try_into()
                .map_err(|_| MathError::CastOverflow)
    } else { 
        rewards_per_share
            .checked_mul(U256::from(shares))
            .ok_or(MathError::Overflow(2))?
            .checked_div(U256::from(SCALING_FACTOR))
            .ok_or(MathError::DivByZero(2))?
            .checked_add(U256::from(1)) // Add 1 to round up
            .ok_or(MathError::Overflow(3))? // Handle overflow
            .try_into()
            .map_err(|_| MathError::CastOverflow)   
    }}

First It checks (U256::from(shares) % U256::from(SCALING_FACTOR) == U256::from(0) ), if there is rounding down, it adds 1 to the results, in this way there will be no rewards stuck in the contract.

YOU said, " this would mean that we use more shares than the user actually has when calculating rewards. By doing that we would pay out more rewards than the user is eligible to and, in effect, "drain" the reward tokens (there wouldn't be enough rewards for everyone)."

Now in my revised code, users don't get more shares, they just get the share that will stuck in the contract, now farm_distributed_unclaimed_rewards is equal to all unclaimed rewards . And all users could get their shares.

[deuszx]

Hey @0xmahdirostami , thanks for the follow up comment.

I still think the rounding down is the correct behaviour here. We're loosing here at most 1, smallest unit, of the reward. Not 1 reward token, 1 smallest unit. For example, if rewards were paid out in USDC, which is 6 decimal points, we would be loosing 10^(-6) USDC per all shares - ie across all rewards being distributed to all farmers, we would be loosing 0.000001 USDC. For ETH token that would be 10^(-18) (decimal points, precision of the ETH token).

Note that the additional comparison you're making, which would need to be run on every call to this method, will incur additional cost as well. This needs to be measured to check whether it's more than what is being lost due to rounding.

Lastly, your fix does not work for all the cases. Maybe it's a coincidence that it worked for yours. Consider the simple case:

farm_start = 1
farm_duration = 100
rewards = 150

You'd expect that 150 reward tokens will be paid out as rewards, but in reality it will be just 101.

Check yourself:

#[drink::test]
fn calc_round_down(mut session: Session) {
    let now = get_timestamp(&mut session);
    set_timestamp(&mut session, now);

    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);
    // set up the farm with ICE as the pool token and WOOD as a reward token
    let farm = farm::setup(&mut session, ice.into(), vec![wood.into()], BOB);

    let deposit_amount = 100;
    psp22::increase_allowance(&mut session, ice.into(), farm.into(), deposit_amount, BOB);
    farm::deposit_to_farm(&mut session, &farm, deposit_amount, BOB).unwrap();

    // setting up start, end and the rewards amount
    let duration = 100;
    let farm_start = now;
    let farm_end = farm_start + duration;
    // 1.5 rewards per time unit
    let rewards_amount = 150;

    psp22::increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);
    farm::start(
        &mut session,
        &farm,
        farm_start,
        farm_end,
        vec![rewards_amount],
        BOB,
    )
    .unwrap();

    set_timestamp(&mut session, farm_end);

    let wood_rewards = farm::claim_rewards(&mut session, &farm, [0].to_vec(), BOB).unwrap();
    assert_eq!(wood_rewards, vec![rewards_amount]);
}

[0xmahdirostami]

@deuszx Thanks for feedback.

For example, if rewards were paid out in USDC, which is 6 decimal points, we would be loosing 10^(-6) USDC per all shares - ie across all rewards being distributed to all farmers, we would be loosing 0.000001 USDC.

The rounding issue is happening per call to rewards_earned_by_shares. So for example, for 1000 users that deposit or withdraw 10 times the rounding issue will be, 10_000 → 0.01 USDC.

Note that the additional comparison you're making, which would need to be run on every call to this method, will incur additional cost as well. This needs to be measured to check whether it's more than what is being lost due to rounding.

Yes, I'm not a gas expert. (I don't have any opinion on this) but you can handle this in a more optimized way.

You'd expect that 150 reward tokens will be paid out as rewards, but in reality it will be just 101.

This issue could happen because of all divisions in process, for example, rounding issue in reward rate --> reward rate will be 1, not 1.5. If you want to test the issue that we are discussing, you could test just the rewards_earned_by_shares function. Besides that you are testing for 1 user at the end of the farm, the actual impact comes up when a number of users is more than 1. As the number of users is higher and interaction is higher, the impact is higher.

[deuszx]

Yes, the difference grows as more interactions is made. Still - as you've shown yourself - you need 10_000 user interactions to loose 1 cent.

What I'm trying to say is that there's nothing wrong with the calculations in rewards_earned_by_shares function - it's necessary to round up or down and it's always safer to round down.

Now, to the promising part. I'm looking at your latest issue as the actual fix for the problem raised in this submissions.

They're pointing at different pieces of the contract but are touching the same core problem - users' rewards are imprecise due to invalid handling of reward_rate in the contract. The problem isn't to be fixed in rewards_earned_by_shares function but in how the reward_rate is initially computed, as you've raised yourself in the other issue.

[0xmahdirostami]

hey @deuszx , I wrote a test and tested it after solving issue #44, one time with rounding up and one time with the original one (rounding down):

when we rounded up the test failed.

output for rounding up (the revised code in this issue):

running 1 test
thread 'tests::testy' panicked at 'called `Result::unwrap()` on an `Err` value: TokenTransferFailed(AccountId([224, 68, 8, 42, 45, 167, 91, 200, 136, 83, 66, 217, 3, 82, 232, 52, 176, 25, 126, 67, 68, 118, 49, 94, 251, 62, 15, 238, 182, 54, 61, 64]), InsufficientBalance)', src/utils.rs:278:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
F

output for rounding down (the original code):

running 1 test
Diff in balance 1999999999
.
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.54s

test:

#[test]
fn testy() {
    let mut session: Session<MinimalRuntime> = Session::new().expect("Init new Session");
    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);
    let farm = setup_farm(
        &mut session,
        ice.into(),
        vec![wood.into()],
        BOB,
    );
    let deposit_amount = 1_000_000;
    session
    .sandbox()
    .mint_into(ALICE, 1_000_000_000u128)
    .unwrap();
    transfer_tokens(&mut session, ice.into(), alice(), deposit_amount, BOB).unwrap();
    assert!(balance_of(&mut session, ice.into(), alice()) == deposit_amount);

    // ALICE DEPOSIT
    let _ = 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());

    // BOB DEPOSIT
    let _ = 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());

    // SETUP FARM
    let now =  get_timestamp(&mut session);
    set_timestamp(&mut session, now);
    let farm_start = now;
    let farm_end = farm_start + 100000;
    let rewards_amount = 1_000_000_000; //1000 USDC
    let _ = increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);
    let call_result = setup_farm_start(
        &mut session,
        &farm,
        farm_start,
        farm_end,
        vec![rewards_amount],
        BOB,
    );
    assert!(call_result.is_ok());
    set_timestamp(&mut session, farm_end);

    // BOB deposit again
    let _ = 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());

    // SETUP FARM AGAIN
    let farm2_start = farm_end;
    let farm2_end = farm2_start + 100000;
    let _ = increase_allowance(&mut session, wood.into(), farm.into(), rewards_amount, BOB);
    let call_result = setup_farm_start(
        &mut session,
        &farm,
        farm2_start,
        farm2_end,
        vec![rewards_amount],
        BOB,
    );
    assert!(call_result.is_ok());
    set_timestamp(&mut session, farm2_end);

    // UNTIL NOW 1_000_000_000 * 2 REWARDS
    let farm_balance_before_claim = balance_of(&mut session, wood.into(), farm.into());

    // ALICE claims
    let call_result = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        ALICE,
    );
    assert!(call_result.is_ok());
    // BOB claims
    let call_result = claim_from_farm(
        &mut session,
        &farm,
        [0].to_vec(),
        BOB,
    );
    assert!(call_result.is_ok());

    let farm_balance_after_claim = balance_of(&mut session, wood.into(), farm.into());

    println!("Diff in balance {}", farm_balance_before_claim - farm_balance_after_claim);
}

Sorry for this submission, this submission is invalid .

[deuszx]

Hey @0xmahdirostami . Thank you for following up on this. I'm glad we agree on it now.

[deuszx]

Thank you for participation. After carefully reviewing the submission we've concluded that this issue particular is INVALID. As discussed above - we find the #40 to be the one properly pointing at the vulnerability.

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