Token updating can lead to incorrect price evaluation

[hats-bug-reporter[bot]]

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

Description:

Description

token rates are updated through the update_rate function:

  pub fn update_rate(&mut self, current_time: u64) -> bool {
        if self.is_outdated(current_time) {
            self.update(current_time)
        } else {
            false
        }
    }

Inside this function, a check is performed to determine if the rate is outdated. If the rate is outdated, it gets updated; otherwise, no action is taken.

fn is_outdated(&self, current_time: u64) -> bool {
        current_time.saturating_sub(self.last_token_rate_update_ts) >= self.expiration_duration_ms
    }

To consider if a rate is outdated or not, the current_time is substracted from the last time the token was updated. If this is >= the expiration_duration_ms it means the price is outdated.

The problem however is that expiration_duration_ms is set to 1 value:

pub struct ExternalTokenRate {
    cached_token_rate: u128,
    last_token_rate_update_ts: u64,
    token_rate_contract: AccountId,
->    expiration_duration_ms: u64,
}

This will lead to problems for tokens that are updated < expiration_duration_ms:

• expiration_duration_ms is set to 12 hours
• Token A rate gets updated every 3 hours
• update_rate is called upon Token A 11 hours from the last update
• function will return false since the token is not oudated yet due to the assumption that every token is updated every expiration_duration_ms
• Token A will return an incorrect price reflection since it has been updated 3 times already ( Hour 3, Hour 6, Hour 9), but will return the price from 12 hours ago

This allows users to trade Token A to their advantage. However, on the flip side, users might also lose money by receiving an outdated price that is higher than the current market price.

Recommendation

Handle the logic for tokens that update more frequently

[JanKuczma]

Thank you for your submission.

The assumption in your PoC is that the expiration_duration_ms is not set properly (Token A gets updated every 3h but expiration_duration_ms is set to 12h). This misconfiguration is a mistake made by the contract deployer. In case the rate of any of the rated tokens in the pool is highly volatile (changes significantly over a short period of time) the expiration_duration_ms should be set to 0 to ensure the rates are updated with every contract call.

[whoismxuse]

hi @JanKuczma, this fix will not solve the problem. the crux of the problem is that 1 value is set for all the tokens, lets take the example assume 1 token is highly volatile hence we set expiration_duration_ms to a low number, in this case 0.

The issue now is that for every token used by that contract deployer, there will always be a call to check for an update since the token will always be outdated. As mentioned in the comments, when using an oracle token, it will send a cross-contract call to the RateProvider, which can vary in gas consumption.

        /// Update cached rates if expired.
        ///
        /// NOTE:
        /// If the pool contains a token with rate oracle, this function makes
        /// a cross-contract call to the `RateProvider` contract if the cached rate is expired.
        /// This means that the gas cost of the contract methods that use this function may vary,
        /// depending on the state of the expiration timestamp of the cached rate.
        fn update_rates(&mut self) {
            let current_time = self.env().block_timestamp();
            let mut rate_changed = false;
            for rate in self.pool.token_rates.iter_mut() {
                rate_changed |= rate.update_rate(current_time);
            }
            if rate_changed {
                Self::env().emit_event(RatesUpdated {
                    rates: self.pool.token_rates.clone(),
                });
            }
        }

Since every token will be outdated on each swap, the oracle will be called for every token during every swap, even though some tokens only need updating every 6 or 12 hours. This will result in a significant amount of unnecessary gas consumption due to the excessive external calls required each time as stated in the comments.

Mitigation

A simple solution is to introduce additional expiration variables. This allows a contract deployer to easily set one for volatile tokens and another for tokens that do not update frequently.

Note this should not be HIGH, MEDIUM sounds plausible

[JanKuczma]

This is a design choice, not a vulnerability. Here is some more context and justification.

StablePool smart contract is meant for stable tokens, pegged to some common value (e.g. fiat currency) or pegged to each other (e.g. baseToken x liquidStakingBaseToken).

The RateProvider (Rate Oracle) is used for the latter type of tokens, which allows adjusting the exchange rate according to the exchange value (rate) offered by the protocol that stands behind the rated token. This rate should change slowly over time (a few percent annually) in a predictable way.

The RateProviders are NOT price oracles that provide prices of some token in terms of some other unrelated token.

The combination of stable swap invariant with the rate mechanism allows for a low price slippage when swapping the tokens while mitigating permanent loss caused by the increasing value of the rated token in terms of the base token.

Assuming that there’s a pool with two or more rated tokens and one of the rates is volatile this can mean 2 things:

• the tokens are not pegged in any way, meaning that the stableswap pool is not suitable for them
• if the rate of one token is volatile and the tokens are pegged, all the rates should be updated at the same time to ensure the most fair exchange ratio