If there are 2 transactions in the same block, fees will be not accumulated

[hats-bug-reporter[bot]]

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

Description: Description\

The users use stake() function to stake their AZERO tokens and get sAZERO in return. The function also updates the fees but the problem is that it checks the time since last update and if the transaction happened in the same block (equal block.timestamp) they will not be accumulated and will show incorrect value as total_shares_minted will be updated but the fees will not be accumulated.

Attack Scenario\

There is a requirement before the function update_fees() that says:

https://github.com/hats-finance/Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2/blob/main/src/vault/data.rs#L412

/// Must be called before changing: `total_shares_minted`, `fee_percentage`

So when the user1 calls stake() and this transaction is only one in the block, everything works correctly as the time is > 0:

  if time > 0 {
            let virtual_shares = self.total_shares_minted * self.fee_percentage as u128 / BIPS as u128;
            let time_weighted_virtual_shares = virtual_shares * time as u128 / YEAR as u128;

            self.total_shares_virtual += time_weighted_virtual_shares;
            self.last_fee_update = current_time;
        }

Now user2 stakes in the same transaction but the update_fees() are not called as time == 0. total_minted_shares are updated so the specification requirement is bypassed. And the fees are also not accumulated for this amount of total_minted_sharesas the shares should be taken on every deposit according to the current functionality. And there can be many more transactions in the same block that will not be accounted at all.

Recommendation

Change the fees mechanism how the fees are accounted. Preferably it's better not to use time difference but rather have an orientation on the change of total_shares_minted.

[0xmahdirostami]

Thank you for your submission. Intended behavior. If there is no time difference, there is no reward.

[rodiontr]

Thank you for your submission. Intended behavior. If there is no time difference, there is no reward.

that's not only about rewards for users, that's about loss of value for protocol when fees are not taken when they are supposed to be taken

[0xmahdirostami]

that's about loss of value for protocol when fees are not taken when they are supposed to be taken

it's not supposed to be taken if time difference is 0.

Scenario:

1. block.timestamp is 100 and the Total share is 1000.
2. block.timestamp is 120 and user A first transaction (stake 100 tokens), the fee is taken from 1000 shares until this time(120-100).
3. The total share becomes 1100.
4. block.timestamp is 120 and the user A second transaction (stake 50 tokens again). There is no need to take a fee from 1100 shares as until this time fee is taken from first 1000 shares and no time pass for users 100 token.

[rodiontr]

that's about loss of value for protocol when fees are not taken when they are supposed to be taken

it's not supposed to be taken if time difference is 0.

Scenario:

1. block.timestamp is 100 and the Total share is 1000.
2. block.timestamp is 120 and user A first transaction (stake 100 tokens), the fee is taken from 1000 shares until this time(120-100).
3. The total share becomes 1100.
4. block.timestamp is 120 and the user A second transaction (stake 50 tokens again). There is no need to take a fee from 1100 shares as until this time fee is taken from first 1000 shares and no time pass for users 100 token.

it's supposed to be taken when new shares are minted not when the time difference is non-zero, the shares will still be minted regardless to the user, it just collects the fee for the owner

/// Must be called before changing: `total_shares_minted`, `fee_percentage`

total_shares_minted still changes in stake():

https://github.com/hats-finance/Kintsu-0x7d70f9442af3a9a0a734fa6a1b4857f25518e9d2/blob/main/src/vault/lib.rs#L145

self.data.total_shares_minted += amount;

And in your example the same user stakes twice in the same tx. But in my described scenario there are 2 different users and the fees are not taken from the second user's amount

[bgibers]

@rodiontr Fees are calculated over time. No fees are taken from newly staked tokens, rather, the fees are taken from all the shares that were minted before this last update.

Lets say user1 and user2 each stake 1000 AZERO in the same block (block 5 for example sake), and the total shares minted before this was 5000.

We are only calculating the fees on the 5000 that was previously staked. We ignore the newly staked 2000 until fees have accumulated over time.

So when user3 comes along and stakes in block 6, fees will be calculated on 7000 shares now. The previous 5000, plus user1 and user2s stakes