Lost Fees due to Precision Loss during Fee Calculation in `Vault/data.rs:update_fees()`.

[hats-bug-reporter[bot]]

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

Description: Description

In fees calculation, division is being used in the midst of the calculation, not at the end of it. This leads to lost precision in fee amount (as rust doesn't save remainder of division). Division should happen at the end to maintain precision.

Attack Scenario

Lost of fees.

Division in the midst of a calculation:

In Vault/data.rs:update_fees():

            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;

Attachments

NA

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

Do all multiplication first before division to avoid precision loss, consider implement the following changes in Vault/data.rs:update_fees():

 pub fn update_fees(&mut self, current_time: Timestamp) {
        // Time since last update
        let time = current_time - self.last_fee_update;

        // Calculate fee accumulation since last update
        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;
++            let time_weighted_virtual_shares = self.total_shares_minted * self.fee_percentage as u128 * time as u128 / YEAR as u128 / BIPS as u128;
            self.total_shares_virtual += time_weighted_virtual_shares;
            self.last_fee_update = current_time;
        }
    }

[0xmahdirostami]

Thanks for submitting the issue. Could you please provide me with a POC that shows how much is lost? You could also demonstrate the impact with a scenario using real-world values.

Example:

• fee = 200
• BIPS = 10_000
• self.total_shares_minted =
• time =
• in current code: time_weighted_virtual_shares =
• in mitigated code: time_weighted_virtual_shares =

[0xEricTee]

Hi @0xmahdirostami , think about the following scenarios:

Scenario 1: Configurations: fee = 200 BIPS = 10000 self.total_shares_minted = 49 (Although unlikely but minimum_stake is set to 0 in constructor therefore possible.) time = 32179591836.734693877 YEAR = 31536000000

current code: virtual_shares = 49 200(fee_percentage) / 10_000 (BIP) (Result: 0.98) will be rounded down to 0. time_weighted_virtual_shares = 0 time as u128 / YEAR as u128;

In current code, the time_weighted_virtual_shares will stay 0 regardless how big the time is when total_shares_minted is <=49.

mitigated code: time_weighted_virtual_shares = 49 200 32179591836.734693877 / 31536000000 (YEAR) / 10000 (BIP) ; (Result: 1)

In mitigated code, the time_weighted_virtual_shares will increase 1 when time >= 32179591836.734693877.

Scenario 2:

Configurations: fee = 200 BIPS = 10000 self.total_shares_minted = 99. time = 15927272728 YEAR = 31536000000

current code: If total_shares_minted is for example = 99. virtual_shares = total_shares_minted 200 (fee_percentage) / 10_000 (BIP) ---Simplify--> 99 1 / 50 (Result: 1.98). Noticed that 0.98 is lost due to the round down mechanism.

time_weighted_virtual_shares = 1 * 15927272728 / 31536000000 (YEAR); (Result: 0.5...) rounded down to 0.

mitigated code: time_weighted_virtual_shares = 99 200 15927272728 / 31536000000 (YEAR) / 10000 (BIP); ---Simplify-->

time_weighted_virtual_shares = 99 1 15927272728 / 1576800000000; (Result: 1)

Noticed that mitigated_code result in 1 while current_code stays 0.

[0xEricTee]

Scenario 3:

If the update_fee() get called frequently, both current code and mitigated code actually loses precision, see the following examples:

Configurations: fee = 200 BIPS = 10000 self.total_shares_minted = 999999999999. time = 3 YEAR = 31536000000

current code: virtual_shares = 999999999999 200 (fee_percentage) / 10_000 (BIP) ---Simplify--> 999999999999 1 / 50 (Result: 19999999999.98). Noticed that 0.98 is lost due to the round down mechanism.

time_weighted_virtual_shares = 19999999999 * 3 (time) / 31536000000 (YEAR); (Result: 1.9...) rounded down to 1. (value of 0.9 is lost).

mitigated code: time_weighted_virtual_shares = 999999999999 200 3 / 31536000000 (YEAR) / 10000 (BIP); ---Simplify-->

time_weighted_virtual_shares = 999999999999 1 3 / 1576800000000; (Result: 1.9...) rounded down to 1. (value of 0.9 is lost).

Therefore, protocol losing 0.9 precision on each 3 seconds.

[0xmahdirostami]

First scenario: impact: 1 share if the update fee hasn't been called for more than a year. Second scenario: impact: 1 share third scenario: impact: 1 share.

If you find a higher impact, please comment here.