X12 - LTV of 98% would be extremely dangerous

[sherlock-admin3]

X12

High

LTV of 98% would be extremely dangerous

Summary

Having an LTV of 98% that pools can set is really dangerous as it doesn't take into account that oracle prices have the so called deviation, which can be anywhere from 0.25% to 2%. Meaning that the actual LTV would be LTV + oracle1 deviation + oracle2 deviation, which can result in > 100% LTV.

Vulnerability Detail

The README gives us a range for the possible LTV.

Min LTV = 10% = 100000000000000000
Max LTV = 98% = 980000000000000000

However this range reaches up to 98% which is extremely dangerous, no matter the asset, even if the supply-borrowing pair is stable coins.

Example oracles:
stETH : ETH - 0.5% deviation
DAI : ETH - 1% deviation
USDC : ETH - 1% deviation
USDT : ETH - 1% deviation

Both assets may be denominated in ETH, but their value is compared one to one, meaning that a user can deposit USDC to his position and borrow USDT from a pool, where both prices would be compared in terms of ETH. They will not take effect from the price of ETH, but will be effected by the extra oracle deviation, as ETH is generally around 1% - 2% and stable coins to USD are around 0.1% (DAI : USD, USDC : USD, and so on... )

However with the above example we can see such a pool having actual LTV of 100%, as USDC can be 0.99 and USDT 1.01 with the oracle reporting both prices as 1.00 USD. In this case the pool will have 100% LTV allowing borrowers to borrow 100% of the pool causing a DOS and potentially adding some bad debt to the system. This would also distinctiveness liquidators a they won't have any profit from liquidating these positions (once the price normalizes) and may even be on a loss.

Example of similar scenario is the recent depeg on ezETH causing Mrpho to socialize some bad debt, even with reasonable LTV parameters - link.

Impact

LTV of 100% or even above would result in lenders losing their funds, as borrowers would not be incentivized to pay of their loans or would prefer to get liquidated if the price moves to their favor. Liquidators will not liquidate as they would be in a loss.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/RiskEngine.sol#L190

    function acceptLtvUpdate(uint256 poolId, address asset) external {
        if (msg.sender != pool.ownerOf(poolId)) revert RiskEngine_OnlyPoolOwner(poolId, msg.sender);

        LtvUpdate memory ltvUpdate = ltvUpdateFor[poolId][asset];

        // revert if there is no pending update
        if (ltvUpdate.validAfter == 0) revert RiskEngine_NoLtvUpdate(poolId, asset);

        // revert if called before timelock delay has passed
        if (ltvUpdate.validAfter > block.timestamp) revert RiskEngine_LtvUpdateTimelocked(poolId, asset);

        // revert if timelock deadline has passed
        if (block.timestamp > ltvUpdate.validAfter + TIMELOCK_DEADLINE) {
            revert RiskEngine_LtvUpdateExpired(poolId, asset);
        }

        // apply changes
        ltvFor[poolId][asset] = ltvUpdate.ltv;
        delete ltvUpdateFor[poolId][asset];
        emit LtvUpdateAccepted(poolId, asset, ltvUpdate.ltv);
    }

Tool used

Manual Review

Recommendation

Have a lower max LTV.

[iamnmt]

Escalate

Invalid.

The pool owner can always set the LTV in range of [minLtv, maxLtv]. The pool owner is trusted to set the LTV to a correct value that not cause any problems.

Per the Sherlock rules:

5. (External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.
   If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

The contest README is only specifying the value for Max LTV, but it does not specify any value for the LTV of a pool.

[sherlock-admin3]

Escalate

Invalid.

The pool owner can always set the LTV in range of [minLtv, maxLtv]. The pool owner is trusted to set the LTV to a correct value that not cause any problems.

Per the Sherlock rules:

5. (External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.
   If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

The contest README is only specifying the value for Max LTV, but it does not specify any value for the LTV of a pool.

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.

[0x3b33]

Since a range with valid LTV rations is provided everything in this range will be used for different pools. The issue shows how this range is flawed, if pool owner are not supposed to use the values in the rage what is the purpose of having it.

The docs clearly state the the README defines custom restrictions, which were proven wrong in the issue above.

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality.

The issue clearly describes a flaw in the system, while your escalation doesn't justify why it should be invalid. Just twisting the rules around won't be sufficient to invalidate issues.

[cvetanovv]

I disagree with escalation.

@0x3b33 Explain very well how setting a Loan-to-Value (LTV) ratio of 98% is significant because it brings the system dangerously close to a situation where the collateral provided by borrowers could be insufficient to cover the borrowed amount, leading to potential losses for lenders and instability in the protocol.

Also, #122 will be duplicated with this issue. You can see this comment: #122 (comment)

Planning to reject the escalation and leave the issue as is.

[iamnmt]

@cvetanovv

Why is the quoted rule not applicable in this case?

I want to elaborate on the escalation. There are two actors in this issue:

• The protocol, who set the Min LTV, and Max LTV
• The pool owner, who set the LTV of their pool in the range of [Min LTV, Max LTV]

The pool owner will not blindly set any LTV to their asset. Let's say there is a high-volatility asset, then setting the LTV to a high value (e.g. 90%) that is not the Max LTV will cause problems, so the pool owner is expected to set a lower LTV to these assets. In this case, the pool owner is expected to carefully examine the price deviation and set the LTV according to that. Why the same reasoning can not be applied to this issue? The pool owner has to examine the two oracle deviations and set the LTV to not cause any problems.

If this issue is valid, then should an issue about setting a high LTV to a high-volatility asset cause problems to be valid?

[0x3b33]

Why then we have TVL limit caps and not let them be settable to any value ?

I am not saying the owner is not trusted. What I am saying is since that value is provided in a range and this is one of the possible ranges (98%) then it is expected to be used for some pools, however it's use will be extremely dangerous (the why is explained above). Because of that this core feature (the TVL range) is wrong and it's liquidation threshold is too close to bad debt, that even 1 small even can flip a healthy position into a bad debt one.

[cvetanovv]

I agree with @0x3b33 comment.

Because of such situations, this question has been added to the Readme to make it clear what values the protocol will use:

https://github.com/sherlock-audit/2024-08-sentiment-v2?tab=readme-ov-file#q-are-there-any-limitations-on-values-set-by-admins-or-other-roles-in-the-codebase-including-restrictions-on-array-lengths

Are there any limitations on values set by admins (or other roles) in the codebase, including restrictions on array lengths?

Expected launch values for protocol params: Min LTV = 10% = 100000000000000000 Max LTV = 98% = 980000000000000000

We currently have issues with the value provided by the protocol(98%). That's exactly the reason this question is being asked on the protocol. To know the TRUSTED Аdmin, what values will be used. That is why this issue is valid.

My decision to reject the escalation remains.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• iamnmt: rejected

[10xhash]

@cvetanovv Sorry for the late reply

We currently have issues with the value provided by the protocol(98%). That's exactly the reason this question is being asked on the protocol. To know the TRUSTED Аdmin, what values will be used

As per the earlier escalation comment, a pool owner can set any value b/w 10% and 98%. LTV is a risk parameter. If they are choosing to set LTV to 98% it is their chosen risk for the pool and lender's who are not comfortable with the risk are not required to deposit into it.
And it is not required for all configurations to even have issues when the LTV is set to 98%. There are fixed price oracles being used which will have no issues even with setting LTV to 98%. So an admin is trusted to use this value only when such problems won't arise

[cvetanovv]

@10xhash, thanks for the comment.

My decision is based on the admin trust assumptions rule:

Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If there were no values specified in the Readme, this issue would be invalid. However, since a trusted admin would use an LTV of 98%, I think this issue is valid.