New users receive more shares than users who stakes earlier

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x864b14e70109f0c9c64fb2a5cdf323b42326abfff70720934ba4b19e491d7f0d Severity: high

Description: Description\

When staking with Vault/lib.rs:stake() function, the following function will calculate how much shares to mint to stakers:

Vault/lib.rs:get_shares_from_azero:

/// Calculate the value of AZERO in terms of sAZERO
        #[ink(message)]
        pub fn get_shares_from_azero(&self, azero: Balance) -> Balance {
            let total_pooled_ = self.data.total_pooled; // shadow
            if total_pooled_ == 0 {
                // This happens upon initial stake
                // Also known as 1:1 redemption ratio
                azero
            } else {
                azero * self.get_total_shares() / total_pooled_ //erictee-issue: later user get more share.
            }
        }

Vault/lib.rs:get_total_shares():

#[ink(message)]
        pub fn get_total_shares(&self) -> Balance {
            self.data.total_shares_minted + self.get_current_virtual_shares()
        }

Users who stakes at a later stages will receive more shares than users who stake earlier. As a result, this is an unfair distribution of shares issue that cause users to lose rewards.

Attack Scenario\

Unfair shares distribution causes early stakers to lose rewards.

Attachments

NA

1. Proof of Concept (PoC) File

Add the following ink! test to drink_tests/lib.rs:

fn test_mint_too_many_shares_to_new_users() -> Result<(), Box<dyn Error>> {
        let ctx: TestContext = setup().unwrap();
        const STAKE_AMOUNT: u128 = 10_000e10 as u128;

        let (_, sess) = helpers::call_stake(ctx.sess, &ctx.vault, &ctx.share_token, &ctx.bob, STAKE_AMOUNT).unwrap();
        let sess = helpers::update_days(sess, 1);
        let (shares_bob, sess) = helpers::query_token_balance(sess, &ctx.share_token, &ctx.bob).unwrap();

        let (_, sess) = helpers::call_stake(sess, &ctx.vault, &ctx.share_token, &ctx.alice, STAKE_AMOUNT).unwrap();
        let (shares_alice, sess) = helpers::query_token_balance(sess, &ctx.share_token, &ctx.alice).unwrap();

        assert_eq!(shares_alice,shares_bob);

        Ok(())
    }

Result:

---- tests::test_mint_too_many_shares_to_new_users stdout ----
Calling: get_registry_contract()
Calling: get_share_token_contract()
Calling: add_agent()
Calling: add_agent()
Calling: stake()
LOG: Dust: 0
LOG: Depositing 50000000000000 into agent #0
LOG: Depositing 50000000000000 into agent #1
Calling: stake()
LOG: Dust: 0
LOG: Depositing 50000000000000 into agent #0
LOG: Depositing 50000000000000 into agent #1
thread 'tests::test_mint_too_many_shares_to_new_users' panicked at lib.rs:160:9:
assertion `left == right` failed
  left: 100005479452054
 right: 100000000000000
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests::test_mint_too_many_shares_to_new_users

2. Revised Code File (Optional)

   The virtual shares which will increase overtime will cause late stakers to receive more shares.

[coreggon11]

The original shares accrue some fees and staking rewards, I think this is the correct behavior

[0xEricTee]

@coreggon11 But the second user just stake to the protocol and he shouldn't subject to the fees and staking rewards right

[0xmahdirostami]

Thank you for your submission. It is working as intended. Virtual shares are just like normal shares and must be considered. In other DAPPs, they are minted in the update function, but I think here, for gas purposes, they are minted in the withdraw-fee function.

the values of shares is important, see the process, if i miss something let me know: the value of each share decreases in each update fee.

user A 100 share                         :user A Owns 100 azero
owner get 1 share                        :user A Owns 99 azero (give some fee)
user B 100*101/100 = 101 share           :user A owns 99 azero, user B ownes 100 azero(doesn't give fee yet)
owner get 2 shares                       :user A owns 98 azero (give some fee again), user B ownes 99 azero(give some fee)

Remember, earlier users pay higher fees but receive greater rewards. (staking APY is more than fee APY)

[0xEricTee]

so the share values is inflationary? Not sure if staking APY is less than fee APY scenario is possible because if many users stake into the protocol and resulting in earlier stakers losing money as the share they are holding is less valuable than the time when they first staked.

[0xmahdirostami]

so the share values is inflationary? Not sure if staking APY is less than fee APY scenario is possible because if many users stake into the protocol and resulting in earlier stakers losing money as the share they are holding is less valuable than the time when they first staked.

The fee is about 2 percent for a whole year, and the APY is more than that, besides that, there is a compounding mechanism that increases revenue even more.

So early stakers will earn more than later stakers.

[bgibers]

Please see here https://docs.kintsu.xyz/gitbook/the-kintsu-protocol/earning-yield