0x52 - stETH/ETH chainlink oracle has too long of heartbeat and deviation threshold which can cause loss of funds

[sherlock-admin]

0x52

high

stETH/ETH chainlink oracle has too long of heartbeat and deviation threshold which can cause loss of funds

Summary

getTknOhmPrice uses the stETH/ETH chainlink oracle to calculate the current price of the OHM token. This token valuation is used to determine the amount of stETH to skim from the user resulting from oracle arb. This is problematic since stETH/ETH has a 24 hour heartbeat and a 2% deviation threshold. This deviation in price could easily cause loss of funds to the user.

Vulnerability Detail

BLVaultManagerLido.sol#L458-L473

function getTknOhmPrice() public view override returns (uint256) {
    // Get stETH per wstETH (18 Decimals)
    uint256 stethPerWsteth = IWsteth(pairToken).stEthPerToken();

    // Get ETH per OHM (18 Decimals)
    uint256 ethPerOhm = _validatePrice(ohmEthPriceFeed.feed, ohmEthPriceFeed.updateThreshold);

    // Get stETH per ETH (18 Decimals)
    uint256 stethPerEth = _validatePrice(
        stethEthPriceFeed.feed,
        stethEthPriceFeed.updateThreshold
    );

    // Calculate wstETH per OHM (18 decimals)
    return (ethPerOhm * 1e36) / (stethPerWsteth * stethPerEth);
}

getTknOhmPrice uses the stETH/ETH oracle to determine the price which as stated above has a 24 hour hearbeat and 2% deviation threshold, this means that the price can move up to 2% or 24 hours before a price update is triggered. The result is that the on-chain price could be much different than the true stETH price.

BLVaultLido.sol#L232-L240

    uint256 wstethOhmPrice = manager.getTknOhmPrice();
    uint256 expectedWstethAmountOut = (ohmAmountOut * wstethOhmPrice) / _OHM_DECIMALS;

    // Take any arbs relative to the oracle price for the Treasury and return the rest to the owner
    uint256 wstethToReturn = wstethAmountOut > expectedWstethAmountOut
        ? expectedWstethAmountOut
        : wstethAmountOut;
    if (wstethAmountOut > wstethToReturn)
        wsteth.safeTransfer(TRSRY(), wstethAmountOut - wstethToReturn);

This price is used when determining how much stETH to send back to the user. Since the oracle can be up to 2% different from the true price, the user can unfairly lose part of their funds.

Impact

User will be unfairly penalized due large variance between on-chain price and asset price

Code Snippet

https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L440-L455

https://github.com/sherlock-audit/2023-03-olympus/blob/main/sherlock-olympus/src/policies/BoostedLiquidity/BLVaultManagerLido.sol#L458-L473

Tool used

Manual Review

Recommendation

Use the stETH/USD oracle instead because it has a 1-hour heartbeat and a 1% deviation threshold.

[0xLienid]

Fix Implementation: https://github.com/0xLienid/sherlock-olympus/pull/6/files

[cducrest]

Escalate for 10 USDC

Disagree with severity, probably medium or low. The sherlock docs for high severity states: "The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team." The provided fix arguably lowers the risk by 2: we go from 2% deviation threshold to 1% by changing oracle.

If having 2% deviation is unacceptable, I don't see how having 1% is acceptable.

Additionally, the user is able to notice when the price oracle deviate from the real value of the asset, and this value cannot be influenced by an attacker.

[sherlock-admin]

Escalate for 10 USDC

Disagree with severity, probably medium or low. The sherlock docs for high severity states: "The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team." The provided fix arguably lowers the risk by 2: we go from 2% deviation threshold to 1% by changing oracle.

If having 2% deviation is unacceptable, I don't see how having 1% is acceptable.

Additionally, the user is able to notice when the price oracle deviate from the real value of the asset, and this value cannot be influenced by an attacker.

You've created a valid escalation for 10 USDC!

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.

[IAm0x52]

Escalate for 10 USDC

Disagree with the comment above. Sponsor has clearly accepted issue and has not disagreed with severity, which indicates they do not consider it an acceptable risk

[sherlock-admin]

Escalate for 10 USDC

Disagree with the comment above. Sponsor has clearly accepted issue and has not disagreed with severity, which indicates they do not consider it an acceptable risk

You've created a valid escalation for 10 USDC!

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.

[hrishibhat]

Escalation accepted

Accepting the first escalation as the severity of this impact can be considered medium based on the escalation

[sherlock-admin]

Escalation accepted

Accepting the first escalation as the severity of this impact can be considered medium based on the escalation

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[IAm0x52]

Fix looks good. Now uses steth/usd and eth/usd oracles in place of steth/eth oracles to reduce delay and deviation