hash - KinkedRateModel's `getInterestRate` rounds down favour of the borrower's

[sherlock-admin2]

hash

Medium

KinkedRateModel's getInterestRate rounds down favour of the borrower's

Summary

KinkedRateModel's getInterestRate rounds down favour of the borrower's

Vulnerability Detail

The interest rate else where is rounded up in favour of the protocol

Eg:

    function getInterestAccrued(uint256 lastUpdated, uint256 totalBorrows, uint256) external view returns (uint256) {
        // [ROUND] rateFactor is rounded up, in favor of the protocol

But KinkedRateModel rounds down the utilisation which favors the borrowers instead

    function getInterestRate(uint256 totalBorrows, uint256 totalAssets) public view returns (uint256) {
        uint256 util = (totalAssets == 0) ? 0 : totalBorrows.mulDiv(1e18, totalAssets, Math.Rounding.Up);

=>      if (util <= OPTIMAL_UTIL) return MIN_RATE_1 + SLOPE_1.mulDiv(util, OPTIMAL_UTIL, Math.Rounding.Down);
        else return MIN_RATE_2 + SLOPE_2.mulDiv((util - OPTIMAL_UTIL), MAX_EXCESS_UTIL, Math.Rounding.Down);

Impact

Protocol can loose dust amount of interest

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/irm/KinkedRateModel.sol#L58-L59

Tool used

Manual Review

Recommendation

If not intended, round up

[z3s]

low. loss is small

[0x3b33]

Escalate

This issue should be valid. The rounding may be small, but it is in favor of the borrowers, lowering the APR.

As stated in the readme SuperPool is strictly 4626, meaning all of it's rounding should be in favor of the system.

SuperPool.sol is strictly ERC4626 compliant Pool.sol is strictly ERC6909 compliant

However that's not the case here as a single SuperPool can have one or multiple pools, all of which use KinkedRateModel. In such cases the SuperPool will round in some instances towards the system (in it's deposit/withdraw) and in other instances towards the user/borrower (in KinkedRateModel's getInterestRate).

Because of this KinkedRateModel's getInterestRate needs to be changed to round in favor of the system.

[sherlock-admin3]

Escalate

This issue should be valid. The rounding may be small, but it is in favor of the borrowers, lowering the APR.

As stated in the readme SuperPool is strictly 4626, meaning all of it's rounding should be in favor of the system.

SuperPool.sol is strictly ERC4626 compliant Pool.sol is strictly ERC6909 compliant

However that's not the case here as a single SuperPool can have one or multiple pools, all of which use KinkedRateModel. In such cases the SuperPool will round in some instances towards the system (in it's deposit/withdraw) and in other instances towards the user/borrower (in KinkedRateModel's getInterestRate).

Because of this KinkedRateModel's getInterestRate needs to be changed to round in favor of the system.

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.

[cvetanovv]

I agree with the escalation.

In the readme, it is written that ERC4626 must be implemented strictly:

SuperPool.sol is strictly ERC4626 compliant Pool.sol is strictly ERC6909 compliant

Also, these small losses over time will accumulate into a more significant loss.

Together, these two things make this issue a valid Medium.

Planning to accept the escalation and make this issue Medium.

[WangSecurity]

Result: Medium Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0x3b33: accepted

[0xjuaan]

Hi @WangSecurity @cvetanovv, sorry for the late message.

1. The difference in interest rate is 1 wei. This is insignificant and does not meet the criteria of medium severity, which clearly states here that:

   The loss of the affected party must exceed 0.01% and 10 USD.

2. The ERC4626 standard does not require that rounding is done in the favour of suppliers, it is simply a security consideration mentioned, stating it is considered most secure to favor the Vault itself during calculations over its users. This means that the escalation comment is misleading.

Based on these points the issue should remain invalid.

[elhajin]

I agree with @0xjuaan ; and I think there's a misunderstanding here:

• The interest rate module shouldn't need to comply with ERC-2646 or ERC-6909.

Additionally, these small losses over time will accumulate into a more significant loss.

I don’t see this as a loss tbh. It’s simply rounding the interest rate down by 1 wei. For example, instead of a borrower paying a 10% interest rate, due to rounding, they would pay 9.99999999999...%.

In fact, the utilization rate (UR) is rounded up beforehand, so this situation doesn’t even apply. Rounding down is completely acceptable :

uint256 util = (totalAssets == 0) ? 0 : totalBorrows.mulDiv(1e18, totalAssets, Math.Rounding.Up);

[cvetanovv]

@0xjuaan @elhajin @0x3b33

The main reason I accept this issue as a valid Medium is that the protocol has specified in the Readme that they want it to be strictly compliant with ERC4626.

I agree that rounding itself is no more than Low severity. I wrote it as an addition.

I don't really see this rounding being a MUST, which is the main requirement to follow strictly compliant.

I'm planning to invalidate the issue.

[ruvaag]

Agree with second escalation. This is not a ERC4626 compliance issue and the impact from rounding is minimal. This should be a low.