Nihavent

Medium

Pool supply interest compounds randomly based on the frequency of `ReserveLogic::updateState()` calls, resulting in inconsistant and unexpected returns for suppliers

Summary

_updateIndexes() considers the previously earned interest on supplies as principle on the next update, resulting in a random compounding effect on interest earned. Borrowers may be incentivized to call forceUpdateReserve() frequently if gas costs are low relative to the upside of compounding their interest.

Root Cause

Any time an action calls ReserveLogic::updateState(), ReserveLogic::_updateIndexes() is called. Note this occurs before important state-changing pool actions as well as through public/external functions such as Pool::forceUpdateReserve() and Pool::forceUpdateReserves().

_updateIndexes() calculates lienar interest since the last update and uses this to scale up the _cache.currLiquidityIndex and assigns to _cache.nextLiquidityIndex:

  function _updateIndexes(DataTypes.ReserveData storage _reserve, DataTypes.ReserveCache memory _cache) internal {
    ... SKIP!...

    if (_cache.currLiquidityRate != 0) {
@>    uint256 cumulatedLiquidityInterest = MathUtils.calculateLinearInterest(_cache.currLiquidityRate, _cache.reserveLastUpdateTimestamp);
      _cache.nextLiquidityIndex = cumulatedLiquidityInterest.rayMul(_cache.currLiquidityIndex).toUint128();
      _reserve.liquidityIndex = _cache.nextLiquidityIndex;
    }

    ... SKIP!...
  }

This creates a compounding-effect on an intended simple interest calculated, based on the frequency it's called. This is due to the interest accumulated on the last refresh being considered principle on the next refresh.

Internal pre-conditions

Pool functions normally with supply/borrow/repay calls

External pre-conditions

No response

Attack Path

Supplier supplies assets
Borrower takes debt against supplier's assets and pays interest
Indexes are updated at some frequency
Supplier earns compound-like interest on their supplied assets

Impact

Suppliers receive compound-interest on their supplied assets instead of the intended fixed/linear interest.

PoC

Create a new file in /test/forge/core/pool and paste the below contents.

Run command forge test --mt test_POC_InterestRateIndexCalc -vv to see the following logs which show the more frequently the update, the more compounding-like the liquidityIndex becomes.

Ran 3 tests for test/forge/core/pool/RandomCompoundingInterestPOC.t.sol:PoolSupplyRandomCompoundingEffect
[PASS] test_POC_InterestRateIndexCalc_1_singleUpdate() (gas: 740335)
Logs:
  Updates once after a year: liquidityIndex 1.333e27
  Updates once after a year: borrowIndex 1.446891914398940457716504e27

[PASS] test_POC_InterestRateIndexCalc_2_monthlyUpdates() (gas: 1094439)
Logs:
  Updates monthly for a year: liquidityIndex 1.388987876426245531179679454e27
  Updates monthly for a year: borrowIndex 1.447734004896004725108257228e27

[PASS] test_POC_InterestRateIndexCalc_3_dailyUpdates() (gas: 65326938)
Logs:
  Updates every four hours for a year: liquidityIndex 1.395111981380752339971733874e27
  Updates every four hours for a year: borrowIndex 1.447734611520782405003308237e27

// SPDX-License-Identifier: BUSL-1.1
pragma solidity 0.8.19;

import {console2} from 'forge-std/src/Test.sol';
import {PoolLiquidationTest} from './PoolLiquidationTests.t.sol';

contract PoolSupplyRandomCompoundingEffect is PoolLiquidationTest {

  function test_POC_InterestRateIndexCalc_1_singleUpdate() public {
    _mintAndApprove(alice, tokenA, 10_000e18, address(pool)); // alice 10k tokenA
    _mintAndApprove(bob, tokenB, mintAmountB, address(pool)); // bob 2000 tokenB

    vm.startPrank(alice);
    pool.supplySimple(address(tokenA), alice, 10_000e18, 0); // 10k tokenA alice supply
    vm.stopPrank();

    vm.startPrank(bob);
    pool.supplySimple(address(tokenB), bob, supplyAmountB, 0); // 750 tokenB bob supply
    vm.stopPrank();

    vm.startPrank(alice);
    pool.borrowSimple(address(tokenB), alice, supplyAmountB, 0); // 750 tokenB borrow for 100% utilization
    vm.stopPrank();

    vm.warp(block.timestamp + 365 days);

    pool.forceUpdateReserves(); // updates indicies

    console2.log('Updates once after a year: liquidityIndex %e', pool.getReserveData(address(tokenB)).liquidityIndex);
    console2.log('Updates once after a year: borrowIndex %e', pool.getReserveData(address(tokenB)).borrowIndex);
  }

  function test_POC_InterestRateIndexCalc_2_monthlyUpdates() public {
    _mintAndApprove(alice, tokenA, 10_000e18, address(pool)); // alice 10k tokenA
    _mintAndApprove(bob, tokenB, mintAmountB, address(pool)); // bob 2000 tokenB

    vm.startPrank(alice);
    pool.supplySimple(address(tokenA), alice, 10_000e18, 0); // 10k tokenA alice supply
    vm.stopPrank();

    vm.startPrank(bob);
    pool.supplySimple(address(tokenB), bob, supplyAmountB, 0); // 750 tokenB bob supply
    vm.stopPrank();

    vm.startPrank(alice);
    pool.borrowSimple(address(tokenB), alice, supplyAmountB, 0); // 750 tokenB borrow for 100% utilization
    vm.stopPrank();

    for(uint256 i = 0; i < 12; i++) {
        vm.warp(block.timestamp + (30 * 24 * 60 * 60));
        pool.forceUpdateReserves(); // updates indicies
    }
    vm.warp(block.timestamp + (5 * 24 * 60 * 60)); // Final 5 days
    pool.forceUpdateReserves(); // updates indicies

    console2.log('Updates monthly for a year: liquidityIndex %e', pool.getReserveData(address(tokenB)).liquidityIndex);
    console2.log('Updates monthly for a year: borrowIndex %e', pool.getReserveData(address(tokenB)).borrowIndex);

  }

  function test_POC_InterestRateIndexCalc_3_dailyUpdates() public {
    _mintAndApprove(alice, tokenA, 10_000e18, address(pool)); // alice 10k tokenA
    _mintAndApprove(bob, tokenB, mintAmountB, address(pool)); // bob 2000 tokenB

    vm.startPrank(alice);
    pool.supplySimple(address(tokenA), alice, 10_000e18, 0); // 10k tokenA alice supply
    vm.stopPrank();

    vm.startPrank(bob);
    pool.supplySimple(address(tokenB), bob, supplyAmountB, 0); // 750 tokenB bob supply
    vm.stopPrank();

    vm.startPrank(alice);
    pool.borrowSimple(address(tokenB), alice, supplyAmountB, 0); // 750 tokenB borrow for 100% utilization
    vm.stopPrank();

    for(uint256 i = 0; i < (6 * 365); i++) {
        vm.warp(block.timestamp + (4 * 60 * 60));
        pool.forceUpdateReserves(); // updates indicies
    }

    console2.log('Updates every four hours for a year: liquidityIndex %e', pool.getReserveData(address(tokenB)).liquidityIndex);
    console2.log('Updates every four hours for a year: borrowIndex %e', pool.getReserveData(address(tokenB)).borrowIndex);
  }
}

Mitigation

Track the principle and the interest separately. Accrue simple interest on the principle only.

Escalate

This issue does not account for a potential scenario. All core functions always call the updateState() function. This PoC and report assume the pool will not be used by more than two users. However, this creates a paradox: if only two users use the pool, the issue presents itself, but the problem becomes negligible because, with just two users, there will be virtually no loss due to the lack of liquidity. Conversely, if there are many users in the pool, the state updates will occur frequently as expected, and the issue will not manifest in that scenario.

Furthermore, the impact is minimal. The compared update rates are unrealistic. ZeroLend is also a fork of Aave, and this implementation remains unchanged in ZeroLend; it is identical to the one currently used in Aave (see: Aave). This is not an overlooked issue in Aave, and it does not cause any problems in the protocol's indexes.

EDIT:

I want to give a scenario in order to express my statement.

Suppose the interest rate is 5%. Alice supplies money to the pool in 2024, and let's assume the index starts at 1.0. One year later, Bob supplies money to the pool with the same 5% interest rate. For simplicity, we assume the rate remains constant. When Bob supplies his money, the index will have grown to 1.05, so Bob effectively buys shares at the 1.05 index price. Then, after another year, the index will be 1.05 * 1.05 = 1.1025 (the report claims this number should be 1.10).

If we were to use 1.10 instead of 1.1025, Bob's one-year investment would not yield 5% annually. He would have purchased shares at the 1.05 price, but after a year, his shares would be valued at 1.10, which would result in only a 4.76% yield instead of the expected 5%.

Escalate

This issue does not account for a potential scenario. All core functions always call the updateState() function. This PoC and report assume the pool will not be used by more than two users. However, this creates a paradox: if only two users use the pool, the issue presents itself, but the problem becomes negligible because, with just two users, there will be virtually no loss due to the lack of liquidity. Conversely, if there are many users in the pool, the state updates will occur frequently as expected, and the issue will not manifest in that scenario.

Furthermore, the impact is minimal. The compared update rates are unrealistic. ZeroLend is also a fork of Aave, and this implementation remains unchanged in ZeroLend; it is identical to the one currently used in Aave (see: Aave). This is not an overlooked issue in Aave, and it does not cause any problems in the protocol's indexes.

EDIT:

I want to give a scenario in order to express my statement.

Suppose the interest rate is 5%. Alice supplies money to the pool in 2024, and let's assume the index starts at 1.0. One year later, Bob supplies money to the pool with the same 5% interest rate. For simplicity, we assume the rate remains constant. When Bob supplies his money, the index will have grown to 1.05, so Bob effectively buys shares at the 1.05 index price. Then, after another year, the index will be 1.05 * 1.05 = 1.1025 (the report claims this number should be 1.10).

If we were to use 1.10 instead of 1.1025, Bob's one-year investment would not yield 5% annually. He would have purchased shares at the 1.05 price, but after a year, his shares would be valued at 1.10, which would result in only a 4.76% yield instead of the expected 5%.

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.

Escalate

This issue does not account for a potential scenario. All core functions always call the updateState() function. This PoC and report assume the pool will not be used by more than two users. However, this creates a paradox: if only two users use the pool, the issue presents itself, but the problem becomes negligible because, with just two users, there will be virtually no loss due to the lack of liquidity. Conversely, if there are many users in the pool, the state updates will occur frequently as expected, and the issue will not manifest in that scenario.

Furthermore, the impact is minimal. The compared update rates are unrealistic. ZeroLend is also a fork of Aave, and this implementation remains unchanged in ZeroLend; it is identical to the one currently used in Aave (see: Aave). This is not an overlooked issue in Aave, and it does not cause any problems in the protocol's indexes.

EDIT:

I want to give a scenario in order to express my statement.

Suppose the interest rate is 5%. Alice supplies money to the pool in 2024, and let's assume the index starts at 1.0. One year later, Bob supplies money to the pool with the same 5% interest rate. For simplicity, we assume the rate remains constant. When Bob supplies his money, the index will have grown to 1.05, so Bob effectively buys shares at the 1.05 index price. Then, after another year, the index will be 1.05 * 1.05 = 1.1025 (the report claims this number should be 1.10).

If we were to use 1.10 instead of 1.1025, Bob's one-year investment would not yield 5% annually. He would have purchased shares at the 1.05 price, but after a year, his shares would be valued at 1.10, which would result in only a 4.76% yield instead of the expected 5%.

The first two sentences of the 'Root Cause' section of the report explicitly acknowledge that updateState() is indeed called before all critical state-changing functions. Therefore, I agree that the impact is minimal in a scenario where many users transact frequently for a given asset on a pool. However, the scenario presented in the report, which considered only two users, was used as a simplified example to highlight the issue clearly.

At no point did the report claim that this issue would significantly impact every asset in every pool. For this issue to be valid it does not need to significantly impact every asset in every pool, it can occur in edge cases.

While you correctly pointed out that the impact on Aave is minimal, Aave and ZeroLendOne differ:

In Aave, liquidity is concentrated into single reserves per chain and asset combinations, and new assets are only added through governance proposals. This means Aave typically expects a large number of users interacting with each pool.
In contrast, ZeroLendOne allows anyone to deploy a pool, and pool owners can add virtually any assets. This leads to fragmented liquidity across the protocol, with many pools each having a set of assets which potentially have low user counts. Consequently, it is unrealistic to assume that every pool/asset combination will have a high number of users.

Given this difference, we cannot dismiss the possibility of numerous pools/assets having low user activity, which by default would lead to infrequent updateState() calls. Note there may be assets added to a pool that are not interacted with frequently for example USDC on a given pool might experience several transactions per day, but tokenA which was added permissionlessly by the pool owner may experience one transaction per month (or less). Under such circumstances, as shown in the POC, a user could increase their claim on the pool's assets by calling updateState() more frequently.

To mitigate this, as suggested in the report, ZeroLend could properly apply simple or fixed interest by separating the principal from the interest, ensuring that the interest rate only applies to the principal, thus preventing any incentive for user intervention to manipulate their returns.

Alternatively, this could be partially mitigated if ZeroLend made forceUpdateReserve() and forceUpdateReserves() permissioned which forces the user to make a much more expensive supply() or withdraw() call to manipulate the supplyIndex.

It looks like you didn't read my given scenario about this. First of all, thank you for stating the differences between ZeroLend and Aave. You're correct about those differences. But you're still incorrect about your statement. This is not compounded interest rate. Compounded interest rate is implemented using exponential formula which is applied in calculation of borrow index. This is compounded interest rate which is increasing exponentially based on the elapsed time:

  function calculateCompoundedInterest(uint256 rate, uint40 lastUpdateTimestamp, uint256 currentTimestamp) internal pure returns (uint256) {
    //solium-disable-next-line
    uint256 exp = currentTimestamp - uint256(lastUpdateTimestamp);

    if (exp == 0) {
      return WadRayMath.RAY;
    }

    uint256 expMinusOne;
    uint256 expMinusTwo;
    uint256 basePowerTwo;
    uint256 basePowerThree;
    unchecked {
      expMinusOne = exp - 1;

      expMinusTwo = exp > 2 ? exp - 2 : 0;

      basePowerTwo = rate.rayMul(rate) / (SECONDS_PER_YEAR * SECONDS_PER_YEAR);
      basePowerThree = basePowerTwo.rayMul(rate) / SECONDS_PER_YEAR;
    }

    uint256 secondTerm = exp * expMinusOne * basePowerTwo;
    unchecked {
      secondTerm /= 2;
    }
    uint256 thirdTerm = exp * expMinusOne * expMinusTwo * basePowerThree;
    unchecked {
      thirdTerm /= 6;
    }

    return WadRayMath.RAY + (rate * exp) / SECONDS_PER_YEAR + secondTerm + thirdTerm;
  }

It's increasing exponentially. In linear interest rate, let say index is 1.05 at timestamp X which means the price of shares is 1.05. If the interest rate is 5% annually. At timestamp X + 10, the new index should should be:

$$1.05 ((5 10 / 31536000) + 100) / 100 = 1.05000001664764079...$$ Note: 31536000 = 365 days

And the current implementation is doing exactly same thing right now. You can say "Then why calling forceUpdate() is increasing the index ?". The answer is simple: every forceUpdate() is accruing the interest between the updates and this interest generate more revenue for the next update. But this is not compounding interest rate. In compounding interest rate, the revenue is increasing exponentially based on the elapsed time. For instance, between the timestamp X and X + 1, it returns A yield but between timestamp X + 1 and X + 2 it returns 2A yield. That's the difference between linear interest rate and compounding interest rate.

Secondly, actually forceUpdate() attack is really beneficial for the users, not the attacker due to gas cost of this attack. Everybody want the pools which have higher usage ratio including Aave. They're using this implementation for their pool and their pool is really active right now. For instance in USDC pool 1.49B liquidity. There are many off-chain bots currently calling supply, borrow, repay, liquidate etc... And every call is updating the indexes in Aave's pool. The thing that I am trying to express here, based on your statement Aave is under attack because of this frequent calls but they're not.

This code is completely safe for both suppliers and borrowers and there is nothing wrong in here. It's just simple math. If something is compounding then there should be something exponential.

Call based compounding is a common design for lending protocols, including ones utilizing isolated pools. Common fix to infrequent updates is running a keeper bot that ensures some minimal update period (not too small to control the costs, not too big to have enough impact). Simple interest is useful for term loans only, whenever depositor can claim the loan anytime economically it has to be compounding interest since the claim increases.

@DemoreXTess It seems like we agree on most points, but perhaps we're talking past each other in some areas.

Your scenario

"It looks like you didn't read my given scenario about this."

Apologies I did not respond to your scenario because I failed to see how it applied to the submitted report. Your scenario says "Then, after another year, the index will be 1.05 * 1.05 = 1.1025 (the report claims this number should be 1.10)", but the report made no such claim. To be clear the report does not indicate you can achieve perfect fixed/simple interest with the current implementation. As mentioned in the mitigation, this would require a separation of principal and interest.

Compound interest

"You can say "Then why calling forceUpdate() is increasing the index ?". The answer is simple: every forceUpdate() is accruing the interest between the updates and this interest generate more revenue for the next update."

You just described the definition of compound interest, because interest is being generated on previous interest. I think we agree on this point and this is exactly what the report says:

"This creates a compounding-effect on an intended simple interest calculated, based on the frequency it's called. This is due to the interest accumulated on the last refresh being considered principle on the next refresh."

As shown in the POC, the more frequently _updateIndexes() is called, the closer the supplyIndex gets to the borrowIndex (which you acknowledge is calculated using compound interest rate). This is what I mean when I say frequently calling _updateIndexes() will create a compounding-effect. Interest doesn't need to compound every second to be 'compounding', it simply requires the interest to be considered as principal on subsequent updates.

As such, the supplyIndex does compound. The rate at which it compounds is a function of the frequency of updates. For example, with a 33% IR, starting supplyIndex of 1e27, we can validate the POC by applying the compound interest formula to determine the supplyIndex after 1 year for three different scenarios:

supplyIndex( Single Update Per Annum ) = 1e27 (1 + (1/3) / 1) ^ (1) ~ 1.33e27 supplyIndex( Monthly Updates ) = 1e27 (1 + (1/3) / 12) ^ (12) ~ 1.39e27 supplyIndex( Six-hourly updates ) = 1e27 (1 + (1/3) / (365246060)) ^ (3652460*60) ~ 1.40e27

Note these results match the results shown in the POC.

AAVE vs. Zerolend

"The thing that I am trying to express here, based on your statement Aave is under attack because of this frequent calls but they're not."

I believe you misunderstand my position, I am not saying AAVE is under attack. I am saying the current implementation is fit-for-purpose in AAVE because they have a single supplyIndex per chain/asset resulting in highly frequent updates as you described. As a result, the variability of supply index between pools is low because updates are regular. Therefore in AAVE a supplier would gain marginal advantage (if any) by making extra calls to forceUpdate().

However, in Zerolend we may have thousands of pools per chain as they're deployed permissionlessly, each pool supporting up to 128 assets. Do you believe each pool and every asset will experience the volume of users that the equivalent AAVE pool has? If not, we cannot rule out the possibility that there will be many pools with minimal users and therefore updates to the indexes will occur infrequently.

As a result, in these Zerolend 'low activity' pools:

Suppliers will by default earn less interest than in a 'high activity' pool which has the same IR.
Some users will calculate the profitability (after the cost of gas) to update the index at some frequency above the natural update-rate in the 'low activity' pools. By doing so they increase their (and any other supplier's) claim on the pool's assets as shown above and in the POC.

Over time as users realize these impacts, they may tend towards more concentrated liquidity in more 'active pools' which is contrary to the design of ZerolendOne which is trying to avoid monolithic pools:

"Monolithic pools have two major bottlenecks:

Risk cannot be scaled: A single bad asset or oracle, if compromised, can risk the entire protocol, affecting the entire pool. This necessitates risk management that scales in parallel with the pool's growth, which can often be very expensive. "

This is an issue AAVE does not need to contend with, but Zerolend does. This is due to Zerolend forking AAVE and applying the codebase to a protocol with fragmented liquidity (as described above and in this comment)

@dmitriia

"Common fix to infrequent updates is running a keeper bot that ensures some minimal update period (not too small to control the costs, not too big to have enough impact)."

I agree this would fix the problem, assuming is ecnonomical to do so across the all the protocol's chain/pool/asset combinations. But according to the contest readme, these are not planned for Zerolend:

Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?

There are liquidation bots that run off-chain to execute liquidations similar to how liquidations work on Aave.

sherlock-audit / 2024-06-new-scope-judging

Nihavent - Pool supply interest compounds randomly based on the frequency of `ReserveLogic::updateState()` calls, resulting in inconsistant and unexpected returns for suppliers #428

Pool supply interest compounds randomly based on the frequency of `ReserveLogic::updateState()` calls, resulting in inconsistant and unexpected returns for suppliers

Summary

Root Cause

Internal pre-conditions

External pre-conditions

Attack Path

Impact

PoC

Mitigation

Your scenario

Compound interest

AAVE vs. Zerolend

sherlock-audit / 2024-06-new-scope-judging

Nihavent - Pool supply interest compounds randomly based on the frequency of `ReserveLogic::updateState()` calls, resulting in inconsistant and unexpected returns for suppliers #428

Pool supply interest compounds randomly based on the frequency of ReserveLogic::updateState() calls, resulting in inconsistant and unexpected returns for suppliers

Summary

Root Cause

Internal pre-conditions

External pre-conditions

Attack Path

Impact

PoC

Mitigation

Your scenario

Compound interest

AAVE vs. Zerolend

Pool supply interest compounds randomly based on the frequency of `ReserveLogic::updateState()` calls, resulting in inconsistant and unexpected returns for suppliers