A2-security - Inconsistent Interest Accrual Mechanism in `FixedRateModel` and `LinearRateModel` Contracts

[sherlock-admin3]

A2-security

Medium

Inconsistent Interest Accrual Mechanism in FixedRateModel and LinearRateModel Contracts

Summary

The FixedRateModel and LinearRateModel contracts in the Sentiment protocol implement an inconsistent interest accrual mechanism. The current implementation combines aspects of simple and compound interest, leading to random interest amounts based on the frequency of interest updates.

Vulnerability Detail

• The Sentiment protocol's interest rate calculation mechanism in both FixedRateModel and LinearRateModel contracts contains a flaw in how interest is accrued over time. This issue stems from an incorrect implementation of interest calculation combined with the accumulation of interest into the totalBorrows parameter.

Let's examine the relevant code from the FixedRateModel contract:

function getInterestAccrued(uint256 lastUpdated, uint256 totalBorrows, uint256) external view returns (uint256) {
    uint256 rateFactor = ((block.timestamp - lastUpdated)).mulDiv(RATE, SECONDS_PER_YEAR, Math.Rounding.Up);
    return totalBorrows.mulDiv(rateFactor, 1e18, Math.Rounding.Up);
}

• The issue lies in how the getInterestAccrued function calculates interest. It uses a simple interest formula applied on random periods:

Interest = Principal * Rate * Time

Where:

• Principal is the totalBorrows
• Rate is the interest rate
• Time is the deltaTime since last update

This formula, when applied repeatedly with the interest being added to the principal (totalBorrows):

• creates a random compound interest effect. this compounding is inconsistent and The total interest accrued over a fixed period can vary significantly based on how frequently the getInterestAccrued function is called.

• The crucial part of this issue is that the calculated interest is added to the totalBorrows parameter after each accrual. This means that subsequent interest calculations are performed on a principal that includes previously accrued interest, creating a compounding effect that was not correctly designed or implemented.

  
  function accrue(PoolData storage pool, uint256 id) internal {
      (uint256 interestAccrued, uint256 feeShares) = simulateAccrue(pool);
  
      if (feeShares != 0) _mint(feeRecipient, id, feeShares);
      pool.totalDepositShares += feeShares;
      pool.totalBorrowAssets += interestAccrued;
  >>     pool.totalDepositAssets += interestAccrued;
      pool.lastUpdated = uint128(block.timestamp);
  }

  Mathematically, this can be represented as:

For n periods:

totalBorrows_n = totalBorrows_0 * (1 + R(t))^n

Where:

• totalBorrows_0 is the initial borrowed amount
• R(t) is the rateFactor
• n is the number of periods

This formula results in different total interest amounts based on the number of compounding periods, which is determined by how frequently the getInterestAccrued function is called.

The correct approach should either:

1. Implement true compound interest:

   A = P * (1 + r)^t

   Where A is the final amount, P is the principal, r is the interest rate, and t is the time in years.

2. Implement a non-compounding method: Keep a separate totalBorrowPrincipal that remains doesn't include intrestRate accrual, and calculate interest based on this fixed principal:

   Interest = totalBorrowPrincipal * Rate * Time

The current implementation falls between these two approaches, creating an inconsistent interest accrual mechanism. This issue lead to unfair and unpredictable interest charges for users, as the total interest paid can vary based on factors outside of their control, such as how often the function accrue get called.

example poc :

Consider a scenario with a fixed interest rate of 20% APR and an initial borrowed amount of 1,000,000 units:

• we will be calculating the intrest accrual for 1 year , in three cases (this unrealistic but just to simplify the issue) :
  1. case 1 : Annual update (accrue function called once a year)
     • Interest after 1 year: 1,000,000 (0.2 1) = 200,000
     • Total borrowed after 1 year: 1,200,000

2. case 2 : Semi-annual updates (accrued function called twice a year)

   • Interest after 6 months: 1,000,000 (0.2 0.5) = 100,000
   • Total borrowed after 6 months: 1,100,000
   • Interest for next 6 months: 1,100,000 (0.2 0.5) = 110,000
   • Total borrowed after 1 year: 1,210,000

3. case 3 : Quarterly updates (accrued function called four times a year)

   • First quarter: 1,000,000 (0.2 0.25) = 50,000
   • Second quarter: 1,050,000 (0.2 0.25) = 52,500
   • Third quarter: 1,102,500 (0.2 0.25) = 55,125
   • Fourth quarter: 1,157,625 (0.2 0.25) = 57,881
   • Total borrowed after 1 year: 1,215,506

This example demonstrates how the same loan over the same period results in different total interest amounts based solely on the frequency of updates, highlighting the inconsistency in the current implementation.

Impact

1. Inconsistent interest accrual for borrowers based on update frequency.
2. Undermined financial predictability and fairness of the lending protocol.
3. Potential for manipulation of interest payments through timed interactions.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/25a0c8aeaddec273c5318540059165696591ecfb/protocol-v2/src/irm/FixedRateModel.sol#L30C1-L38C6

Tool used

Manual Review

Recommendation

To address the interest calculation issue, the protocol should implement one of the following approaches:

1. True Compound Interest: Implement a proper compound interest formula using exponential calculations. This ensures consistent interest accrual regardless of update frequency.

2. Simple Interest with Fixed Principal: Maintain a separate totalBorrowPrincipal that remains constant. Calculate interest based on this fixed principal without compounding.

Either approach should be consistently applied in both FixedRateModel and LinearRateModel contracts.

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

z3s commented:

Design decision

[elhajin]

We disagree with the judge's decision. The issue is not a design choice but an implementation flaw ,The issue at hand cannot be a design decision for the following reasons:

• The protocol inconsistently mixes linear and compound interest, leading to unpredictable results. This is not a deliberate design but a contradiction within the current implementation. A design decision would involve a clear, consistent approach, either true compound interest or linear interest, not a hybrid causing random results.

• Given an interest rate and a borrowed amount, the total interest accrued cannot be determined for a fixed period because it varies based on how frequently the accrue function is called. This contradicts the fundamental principles of interest rates, which should provide a consistent and predictable calculation based on time and rate, not on the frequency of function calls which make it manipulatable .

• There is no design specification or documentation indicating that this mixed mechanism was intended. Design decisions are usually documented and well-defined; this inconsistency suggests an implementation error rather than an intentional choice.

[obou07]

Escalate, per comment

[sherlock-admin3]

Escalate, per comment

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]

@z3s can I have your opinion here?

[z3s]

I still think:

Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational.

[cvetanovv]

We don't have a specific bug here, and as the Lead Judge has stated, this is a design recommendation.

Also, these draft examples that are given are unrealistic because the accrued function is called constantly every day, not several times a year.

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

[aliX40]

We don't have a specific bug here, and as the Lead Judge has stated, this is a design recommendation.

Also, these draft examples that are given are unrealistic because the accrued function is called constantly every day, not several times a year.

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

Hi @cvetanovv and @z3s, the point is there is not a loss of fund, but a loss of yield. Also assuming that the rates will be updated constantly each day, is not optimal considering the permissionlless design of the protocol. Every one can create a pool, and it is the design of the protocol to have multiple pools for the a single asset.

To summarize our points:

• There is a demonstratable loss of yield (because they don't compound)
• With the permessionless design of the protocol, the damage will affect the pools that have less interactions more
• Even if it is a design choice for pools with less frequent interactions to receive less yield. Where is the evidence or proof that proves they made this on purpose.

[cvetanovv]

The example that is given in the issue accrue() function is called 1, 2, or 4 times a year. But this is not true. It is called on every important interaction in the pool, which makes the issue invalid.

My decision to reject the escalation remains.

[WangSecurity]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• obou07: rejected