BalancerOracle::update() can return stale price

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-loopfi/blob/57871f64bdea450c1f04c9a53dc1a78223719164/src/oracle/BalancerOracle.sol#L114-L136

Vulnerability details

Impact

Whenever block.timestamp - lastUpdate > updateWaitWindow and needs to update the price it will return a stale price because it will fetch the price from the lastUpdate not the currentUpdate.

Proof of Concept

In the update() function these are the lines we'll find

 //@audit lets say price is out of date and its getting updated won't it get the pric from one hour ago?
//problem is it uses a current timestamp but with an hour old price
        if (block.timestamp - lastUpdate < updateWaitWindow) revert BalancerOracle__update_InUpdateWaitWindow();
        // update the safe price first
        safePrice = safePrice_ = currentPrice;
        lastUpdate = block.timestamp;

        uint256[] memory weights = IWeightedPool(pool).getNormalizedWeights();
        uint256 totalSupply = IWeightedPool(pool).totalSupply();

        uint256 totalPi = WAD;
        uint256[] memory prices = new uint256[](weights.length);
        // update balances in 18 decimals
        for (uint256 i = 0; i < weights.length; i++) {
            // reverts if the price is invalid or stale
            prices[i] = _getTokenPrice(i);
            uint256 val = wdiv(prices[i], weights[i]);
            uint256 indivPi = uint256(wpow(int256(val), int256(weights[i])));

            totalPi = wmul(totalPi, indivPi);
        }

        currentPrice = wdiv(wmul(totalPi, IWeightedPool(pool).getInvariant()), totalSupply);

from the code we can see that the currentPrice is the last thing updated.

and whenever there updateWindow reaches or passes for us to fetch a new price the safePrice is updated first which is the value from the lastUpdate which is "stale".

it can be argued that its a design decision meaning the updateWindow is just time it needs to fetch a new price but it doesn't mean the price is old. However the updateWindow can be passed and not updated right after meaning the price is two times back because it wasn't updated right away.

Tools Used

manual review

Recommended Mitigation Steps

Revisit the logic to be able to fetch fresh price whenever there need to be a new price fetched

Assessed type

Oracle

[c4-sponsor]

@0xtj24 Sponsors are not allowed to close, reopen, or assign issues or pull requests.

[0xtj24]

That logic updates the price only after a certain updateWaitWindow has passed, storing the oldest safe price. If not updated with the keeper it will return stale price depending on the stalePeriod. This is an expected behaviour

[c4-judge]

koolexcrypto changed the severity to QA (Quality Assurance)

[c4-judge]

koolexcrypto marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by koolexcrypto

[koolexcrypto]

Since updateWaitWindow is immutable, it can't be changed. Therefore, upgrading the severity to Med again.

[c4-judge]

koolexcrypto marked the issue as satisfactory

[c4-judge]

koolexcrypto marked the issue as selected for report