LMPStrategy.sol#verifyLSTPriceGap() - Some Chainlink price feeds have a long heartbeat and a high deviation threshold, causing verifyLSTPriceGap to return an incorrect value

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xfe5fbdd4b5d7e311a16e24d88e6430b1f49dca089bb4c7647cea693ec5d9b343 Severity: medium

Description: Description\ verifyLSTPriceGap is used to that the difference between the safe price and the spot price isn't higher than lstPriceGapTolerance.

function verifyLSTPriceGap(IStrategy.RebalanceParams memory params, uint256 tolerance) internal returns (bool) {
        // Pricer
        IRootPriceOracle pricer = systemRegistry.rootPriceOracle();

        IDestinationVault dest;
        address[] memory lstTokens;
        uint256 numLsts;
        address dvPoolAddress;

        // Out Destination
        if (params.destinationOut != address(lmpVault)) {
            dest = IDestinationVault(params.destinationOut);
            lstTokens = dest.underlyingTokens();
            numLsts = lstTokens.length;
            dvPoolAddress = dest.getPool();
            for (uint256 i = 0; i < numLsts; ++i) {
                uint256 priceSafe = pricer.getPriceInEth(lstTokens[i]);
                uint256 priceSpot = pricer.getSpotPriceInEth(lstTokens[i], dvPoolAddress);
                // For out destination, the pool tokens should not be lower than safe price by tolerance
                console2.log("tolerance: ", tolerance);
                if ((priceSafe == 0) || (priceSpot == 0)) {
                    return false;
                } else if (priceSafe > priceSpot) {
                    if (((priceSafe * 1.0e18 / priceSpot - 1.0e18) * 10_000) / 1.0e18 > tolerance) {
                        return false;
                    }
                }
            }
        }

        // In Destination
        dest = IDestinationVault(params.destinationIn);
        lstTokens = dest.underlyingTokens();
        numLsts = lstTokens.length;
        dvPoolAddress = dest.getPool();
        for (uint256 i = 0; i < numLsts; ++i) {
            uint256 priceSafe = pricer.getPriceInEth(lstTokens[i]);
            uint256 priceSpot = pricer.getSpotPriceInEth(lstTokens[i], dvPoolAddress);
            // For in destination, the pool tokens should not be higher than safe price by tolerance
            if ((priceSafe == 0) || (priceSpot == 0)) {
                return false;
            } else if (priceSpot > priceSafe) {
                if (((priceSpot * 1.0e18 / priceSafe - 1.0e18) * 10_000) / 1.0e18 > tolerance) {
                    return false;
                }
            }
        }

        return true;
    }

To get both priceSafe and priceSpot we use the RootPriceOracle which will forward the call to the correct oracle provider, so that that specific oracle provider can return the price.

Specifically ChainlinkOracle can be used to retrieve the prices.

Each Chainlink price feed has 2 trigger parameters: Deviation threshold and heartbeat.

You can see that for example for the rETH/ETH price feed, the heartbeat is 24 hours, meaning that the price will be updated every 24 hours and the deviation threshold is 2%, meaning that if the price moves up/down more than 2%, the oracle will refresh and return the new price.

A new answer is written when the offchain data moves more than the deviation threshold or 86400 seconds have passed since the last answer was written onchain.

Many other price feeds on mainnet have 2% deviation and 24 hour heartbeats.

Knowing this, if lstPriceGapTolerance is set to something like 1% which is currently used in the tests, Chainlink will not update unless the price moves by 2%, which means there can be situations when the actual price of the asset surpasses lstPriceGapTolerance and should cause verifyRebalance to revert, but doesn't because getPriceInEth returns an answer from Chainlink that is old, and the opossite can happen too, the price of the asset doesn't surpass lstPriceGapTolerance, but because Chainlink hasn't updated it's price and getPriceInEth returns an older value, lstPriceGapTolerance isn't surpassed and verifyRebalance doesn't revert.

At best this will cause verifyLSTPriceGap to return true and a rebalance that shouldn't have taken place, takes place.

At worst, this will cause a DoS on verifyRebalance, until Chainlink's answer gets updated, which for most assets is 24 hours.

Keep in mind that lstPriceGapTolerance is immutable, so it can't be changed and there is no way to skip verifyLSTPriceGap, unless params.destinationIn == address(lmpVault).

Attack Scenario

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional) Either make lstPriceGapTolerance mutable, add a new variable that forces a skip on verifyLSTPriceGap or enforce inside LMPStrategyConfig that lstPriceGapTolerance is at least 20 bps or more.

[PlamenTSV]

Oracles were marked as already audited and thus, OOS

[0xdeth]

The issue isn't with the oracles themselves, it's how LMPStrategy, verifyLSTPriceGap and lstPriceGapTolerance integrate with said oracles is the issue.

[codenutt]

The default in the test .1%. We are currently testing out this value starting at .05%. Having this low of a value is key to ensure those possible price deviations do not have a material impact on the vault. If the execution is delayed while Chainlink price is updated this is OK. Should there be consistent poor pricing or execution the strategy will tighten its execution in other ways to recuperate. This is its design.

[0xdeth]

Hey there,

From what I understand you are ok with Chainlink giving you incorrect prices and stopping the execution of verifyRebalance? If such a low tolerance is used, the function will almost always revert, that's why I recommended a minimum of a 2% tolerance (since most price feeds on mainnet have a 2% deviation threshold), allowing for such a low tolerance will make the function almost unusable.

Maybe I don't understand something, if so, please correct me.

Cheers.