hash - Possible outdated price for tokens in Balancer stable pools due to cached rate

[sherlock-admin]

hash

medium

Possible outdated price for tokens in Balancer stable pools due to cached rate

Summary

While computing the token price from a balancer stable pool, cached rates may be used leading to incorrect prices.

Vulnerability Detail

When computing the token price inside getTokenPriceFromStablePool the current scalingFactors are fetched from the Balancer pool in order to scale the token amounts by calling the getScalingFactors function

    function getTokenPriceFromStablePool(
        address lookupToken_,
        uint8 outputDecimals_,
        bytes calldata params_
    ) external view returns (uint256) {

        ......

                try pool.getLastInvariant() returns (uint256, uint256 ampFactor) {
                    // Upscale balances by the scaling factors
                    uint256[] memory scalingFactors = pool.getScalingFactors();
                    uint256 len = scalingFactors.length;
                    for (uint256 i; i < len; ++i) {
                        balances_[i] = FixedPoint.mulDown(balances_[i], scalingFactors[i]);
                    }

        .......

For stable pools with rate providers, the scaling factors also include the rate of the token which is provided by the rateProvider periodically.

https://vscode.blockscan.com/ethereum/0x1e19cf2d73a72ef1332c882f20534b6519be0276

    function _scalingFactors() internal view virtual override returns (uint256[] memory scalingFactors) {

        .....

        scalingFactors = super._scalingFactors();
        scalingFactors[0] = scalingFactors[0].mulDown(_priceRate(_token0));
        scalingFactors[1] = scalingFactors[1].mulDown(_priceRate(_token1));
    }

If the time gap b/w two swaps in the pool is large, the scalingFactors can be using the older rate. In Balancer this is mitigated by checking for the cached duration before any swap is made

https://vscode.blockscan.com/ethereum/0x1e19cf2d73a72ef1332c882f20534b6519be0276

    function onSwap(
        SwapRequest memory request,
        uint256 balanceTokenIn,
        uint256 balanceTokenOut
    ) public virtual override onlyVault(request.poolId) returns (uint256) {
        _cachePriceRatesIfNecessary();
        return super.onSwap(request, balanceTokenIn, balanceTokenOut);
    }

Impact

Incorrect calculation of token prices

Code Snippet

using possible outdated token rates https://github.com/sherlock-audit/2023-11-olympus/blob/9c8df76dc9820b4c6605d2e1e6d87dcfa9e50070/bophades/src/modules/PRICE/submodules/feeds/BalancerPoolTokenPrice.sol#L811-L827

Tool used

Manual Review

Recommendation

To obtain the latest rates for metastable pools, check whether the current rates are expired by calling the getPriceRateCache function. If expired, call the updatePriceRateCache() function before fetching the scaling factors.

https://vscode.blockscan.com/ethereum/0x1e19cf2d73a72ef1332c882f20534b6519be0276 MetaStable.sol

    function getPriceRateCache(IERC20 token)
        external
        view
        returns (
            uint256 rate,
            uint256 duration,
            uint256 expires
        )
    {
        if (_isToken0(token)) return _getPriceRateCache(_getPriceRateCache0());
        if (_isToken1(token)) return _getPriceRateCache(_getPriceRateCache1());
        _revert(Errors.INVALID_TOKEN);
    }

    function updatePriceRateCache(IERC20 token) external {
        if (_isToken0WithRateProvider(token)) {
            _updateToken0PriceRateCache();
        } else if (_isToken1WithRateProvider(token)) {
            _updateToken1PriceRateCache();
        } else {
            _revert(Errors.INVALID_TOKEN);
        }
    }

[0xJem]

This is a valid issue and highlights problems with Balancer's documentation.

We are likely to drop both the Balancer submodules from the final version, since we no longer have any Balancer pools used for POL and don't have any assets that require price resolution via Balancer pools.

[IAm0x52]

Escalate

Impact of this is negligible. Supported tokens have VERY small rates of accumulation (~5% APR). Even after a full week (which the pool is never inactive for this length) at 5% APR the rate would only be off by ~0.1% which is entirely negligible.

[sherlock-admin2]

Escalate

Impact of this is negligible. Supported tokens have VERY small rates of accumulation (~5% APR). Even after a full week (which the pool is never inactive for this length) at 5% APR the rate would only be off by ~0.1% which is entirely negligible.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@10xhash Could you provide a PoC/numerical example for your finding for the supported tokens? If not I will have to agree with @IAm0x52, though I think even a slight difference in prices could lead to some unintended profits/impacts which could possibly suffice medium severity.

[10xhash]

As commented in #179, Metastable pools may be used in future for the above tokens. Considering weth-wstEth pool and wstEth having ~4% equivalent interest rate for a week

price of wstEth = 2833.47799747
scaling factor wstEth = 1153222004578747471 input of 1 wstEth gives 1.15257225 eth price calculated eth = 2833.47799747 / 1.15257225 == 2458.39512227

if no swap for 1 week, same scaling factor of wstEth used and hence same output eth for 1wstEth keeping same usd price for eth, price of wstEth ~= 2833.47799747 * (1+ 4/100/52) == 2835.65759593 price calculated eth = 2835.65759593 / 1.15257225 == 2460.286195447

diff = (2460.286195447 - 2458.39512227)/2458.39512227 == 0.076923077%

If newer metastable pools are created, the interest rate could be higher (sdai ~5%)

[nevillehuang]

@10xhash wstETH wasn't mentioned as a possibly integrated token in the contest details though, so I'm not sure if your comment applies.

[10xhash]

@10xhash wstETH wasn't mentioned as a possibly integrated token in the contest details though, so I'm not sure if your comment applies.

For this, the token need not interact with the smart contract. Since solely based on the above list of tokens there is only a single stable pool possible ie. DAI-sDai MetastablePool which has not been deployed and likely will never be, it could be deduced that other tokens can also be involved and this still doesn't break the contest requirement that only the above set of tokens are supposed to interact with the contracts.

[Czar102]

What is the impact of a slightly wrong price reading on the codebase?

[10xhash]

Don't know. Haven't looked into it as the usage of obtained price occur in out-of-scope contracts

[nevillehuang]

@0xJem Was there ever any plans of olympus deploying their own pool, e.g. DAI-sDAI? If not I believe this issue is invalid on context of future integrations not explicitly mentioned.

[0xJem]

@0xJem Was there ever any plans of olympus deploying their own pool, e.g. DAI-sDAI? If not I believe this issue is invalid on context of future integrations not explicitly mentioned.

No plans

[Czar102]

Planning to accept the escalation because there is no reason to think the value divergence will surpass any price inaccuracy related to inefficient/relatively volatile markets. There is no evidence the price feed needs to be extremely accurate, hence planning to consider this a low severity issue.

[Czar102]

Result: Low Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• IAm0x52: accepted