Nihavent - Unclaimable reserve assets will accrue in a pool due to the difference between interest paid on borrows and interest earned on supplies

[sherlock-admin3]

Nihavent

Medium

Unclaimable reserve assets will accrue in a pool due to the difference between interest paid on borrows and interest earned on supplies

Summary

The interest paid on borrows is calculated in a compounding fashion, but the interest earned on supplying assets is calculated in a fixed way. As a result more interest will be repaid by borrowers than is claimable by suppliers. This buildup of balance never gets rebased into the liquidityIndex, nor is it claimable with some sort of 'skim' function.

Root Cause

Any time an action calls ReserveLogic::updateState(), ReserveLogic::_updateIndexes() is called.

In _updateIndexes(), the _cache.nextLiquidityIndex is a scaled up version of _cache.currLiquidityIndex based on the 'linear interest' calculated in MathUtils::calculateLinearInterest().

calculateLinearInterest scales a fixed interest annual interest rate by the amount of time elapsed since the last call:

  function calculateLinearInterest(uint256 rate, uint40 lastUpdateTimestamp) internal view returns (uint256) {
    //solium-disable-next-line
    uint256 result = rate * (block.timestamp - uint256(lastUpdateTimestamp));
    unchecked {
      result = result / SECONDS_PER_YEAR;
    }

    return WadRayMath.RAY + result;
  }

Similarly, the _cache.nextBorrowIndex is a scaled up version of _cache.currBorrowIndex based on the 'compound interest' calculated in MathUtils::calculateCompoundedInterest().

calculateCompoundedInterest compounds a rate based on the time elapsed since it was last called:

  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;
  }

As a result, more interest is payable on debt than is earned on supplied liquidity. This is a design choice by the protocol, however without a function to 'skim' this extra interest, these tokens will buildup and are locked in the protocol.

Internal pre-conditions

1. Pool operates normally with supplies/borrows/repays
2. updateState() must NOT be called every second, as this would create a compounding-effect on the 'linear rate' such that the difference in interest paid on debts is equal to the interest earned on supplies.

External pre-conditions

No response

Attack Path

1. Several users supply tokens to a pool as normal
2. Users borrow against the liquidity
3. Time passes, all borrows are repaid
4. All suppliers withdraw their funds (as part of this operation the treasury also withdraws their fee assets)
5. A pool remains with 0 supplyShares and 0 debtShares, but still has a token balance which is unclaimable by anyone

Impact

1. Token buildup in contract is unclaimable by anyone
   • The built up token balance can be borrowed and flash loaned, leading to compounding build up of unclaimable liquidity

PoC

Create a new file in /test/forge/core/pool and paste the below contents. The test shows a simple supply/borrow/warp/repay flow. After the actions are complete, the pool has more tokenB than is claimable by the supplier and the treasury. These tokens are now locked in the contract

// 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 AuditUnclaimableBalanceBuildupOnPool is PoolLiquidationTest {

  function test_POC_UnclaimableBalanceBuildupOnPool () public {
    uint256 aliceMintAmount = 10_000e18;
    uint256 bobMintAmount = 10_000e18;
    uint256 supplyAmount = 1000e18;
    uint256 borrowAmount = 1000e18;

    _mintAndApprove(alice, tokenA, aliceMintAmount, address(pool));         // alice collateral
    _mintAndApprove(bob, tokenB, bobMintAmount, address(pool));             // bob supply
    _mintAndApprove(alice, tokenB, aliceMintAmount, address(pool));         // alice needs some funds to pay interest

    vm.startPrank(bob);
    pool.supplySimple(address(tokenB), bob, supplyAmount, 0); 
    vm.stopPrank();

    vm.startPrank(alice);
    pool.supplySimple(address(tokenA), alice, aliceMintAmount, 0);  // alice collateral
    pool.borrowSimple(address(tokenB), alice, borrowAmount, 0);     // 100% utilization
    vm.stopPrank();

    vm.warp(block.timestamp + 365 days); // Time passes, interest accrues, treasury shares accrue
    pool.forceUpdateReserves();

    // Alice full repay
    vm.startPrank(alice);
    tokenB.approve(address(pool), type(uint256).max);
    pool.repaySimple(address(tokenB), type(uint256).max, 0);

    (,,, uint256 debtShares) = pool.marketBalances(address(tokenB));
    // All debt has been repaid
    assert(debtShares == 0); 

    // Bob's claim on pool's tokenB
    bytes32 bobPos = keccak256(abi.encodePacked(bob, 'index', uint256(0)));
    uint256 BobsMaxWithdrawAssets = pool.supplyShares(address(tokenB), bobPos) * pool.getReserveData(address(tokenB)).liquidityIndex / 1e27;

    // Treasury claim on pool's tokenB
    uint256 accruedTreasuryAssets = pool.getReserveData(address(tokenB)).accruedToTreasuryShares * pool.getReserveData(address(tokenB)).liquidityIndex / 1e27;

    // Total balance of pool's tokenB
    uint256 poolTokenBBalance = tokenB.balanceOf(address(pool));

    assert(poolTokenBBalance > BobsMaxWithdrawAssets + accruedTreasuryAssets); // There are more tokenB on the pool than all suppliers + treasury claim. 
  }

}

Mitigation

One option is to create a function which claims the latent funds to the treasury, callable by an owner

• Calls forceUpdateReserves()
• Calls executeMintToTreasury()
• Calculates the latent funds on a pool's reserve (something like tokenA.balanceOf(pool) - ( totalSupplyShares * liquidityIndex ))
• Sends these funds to the treasury

Another option would be to occasionally rebase liquidityIndex to increase the value of supplyShares so supplies have a claim on these extra funds.

In both cases it may be sensible to leave some dust as a buffer.

[0xspearmint1]

escalate

This issue is invalid for multiple reasons

1. The condition for this issue as stated by the watson is that updateState() must NOT be called regularly. This is totally unrealistic since any any action (supply, borrow, withdraw, repay, etc) will call updateState(). In the POC they provided, it involves not calling updateState() for 365 days after borrowing funds.

2. Sherlock's criteria for a medium issue requires the following:

   Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

No user experiences a loss in this issue



1. Lenders receive the correct interest rate


2. Borrowers pay the correct borrow rate

[sherlock-admin3]

escalate

This issue is invalid for multiple reasons

1. The condition for this issue as stated by the watson is that updateState() must NOT be called regularly. This is totally unrealistic since any any action (supply, borrow, withdraw, repay, etc) will call updateState(). In the POC they provided, it involves not calling updateState() for 365 days after borrowing funds.

2. Sherlock's criteria for a medium issue requires the following:

   Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

No user experiences a loss in this issue



1. Lenders receive the correct interest rate


2. Borrowers pay the correct borrow rate

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.

[Nihavent]

"1. The condition for this issue as stated by the watson is that updateState() must NOT be called regularly. This is totally unrealistic since any any action (supply, borrow, withdraw, repay, etc) will call updateState(). In the POC they provided, it involves not calling updateState() for 365 days after borrowing funds."

The escalation comment misquoted the report saying "updateState() must NOT be called regularly" when the report states "updateState() must NOT be called every second". These are meaningfully different statements because the impact is present when updateState() is called as frequently as 2 seconds, which is more frequently than what would be expected in most pool/asset combinations.

The elapsed time between updateState() calls is completely arbitrary and in the POC I used 365 days as a matter of habbit. See adjusted POC below where instead of warping 365 days, we warp just 2 seconds and the test still passes:

// 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 AuditUnclaimableBalanceBuildupOnPool is PoolLiquidationTest {

    ...

-   vm.warp(block.timestamp + 365 days); // Time passes, interest accrues, treasury shares accrue
+   vm.warp(block.timestamp + 2 seconds); // Time passes, interest accrues, treasury shares accrue
    pool.forceUpdateReserves();

    ...
  }

}

Ran 1 test for test/forge/core/pool/UnclaimableBalanceBuildupOnPool.t.sol:AuditUnclaimableBalanceBuildupOnPool
[PASS] test_POC_UnclaimableBalanceBuildupOnPool() (gas: 811472)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 11.18ms (1.08ms CPU time)

"2. Sherlock's criteria for a medium issue requires the following:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

No user experiences a loss in this issue

Lenders receive the correct interest rate Borrowers pay the correct borrow rate"

Funds locked in the contract must be considered lost because they are permanently unretrievable.

The amount locked (lets call it surpluss) increases every time debt is repaid. Over time it's reasonable to expect a signficiant portion of all a pool's assets will be surpluss and not claimable by anyone.

The value of locked funds will clearly exceed 10 USD as there will usually be several percentage points difference between the indexes. This of course will vary depending on the frequency of updateState() calls. If this needs to be quantified I would be happy to help, but it clearly exceeds dust values.

Finally, we can refer to the Sherlock standards to determine that permanent locked funds constitutes a valid issue:

"2. Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue:

1. The issue causes locking of funds for users for more than a week.
2. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly. \ Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue."

[cvetanovv]

For me, this issue is borderline Medium/Low. Because of this, we have to look at Sherlock's rules.

These are the requirements for Medium severity:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The loss of the affected party must exceed 0.01% and 10 USD.

The losses exceed 0.01% and 10 USD, and the issue meets the requirements for Medium severity.

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

[0xSpearmint]

@cvetanovv Who exactly is experiencing the loss of funds here?

The borrowers pay the expected borrow rate according to the interest rate contract.

The lenders receive the expected supply rate according to the interest rate contract.

The protocol receives the expected revenue from the reserve factor.

AAVE does implement a rescueTokens function but it allows the owner to arbitrarily remove any amount of tokens from the pool, this is fine because AAVE governance is trusted. However, in ZeroLend pool deployment is permission-less, implementing such a function for each pool would pose a huge security risk which is why the protocol chose to not implement it.

This looks like an obvious design choice to me.

[DemoreXTess]

@cvetanovv I agree with @0xSpearmint. This is actually not simply a design choice. Using compounding rate for borrowers and linear rate for suppliers are recommended way to build a lending protocol. In order to keep protocols health at higher point most of the lending protocol using this way. Those money is not lost, it's always in circulation for keeping liquidity in safe point. Repaying all the debt and getting all the pools' money is not rational movement in this PoC. There is no impact here as @0xSpearmint mentioned.

Secondly, I wonder @Nihavent did you solve all the problems in the protocl which is related with interest rate and accrued fund ? Because identifying this problem with this PoC is really hard in current circumstances. We have 33 findings right now. I don't know how many of them related with those.

[Nihavent]

We all seem to agree that there will be unclaimable assets building up in the pool. These are the funds that are locked in the contract, and these funds are clearly lost.

” This looks like an obvious design choice to me.”

Given that the devs implemented ‘sweep()’ in NFTPositionManager which claims tokens of much lower value, not implementing similar functionality in the Pool contract is an obvious oversight and cannot be considered design.

Another piece of evidence that this is not a design choice is the code comment

”The approximation slightly underpays liquidity providers and undercharges borrowers”

The comment indicates that due to using binomial approximation (3 terms) to calculate compounding interest, borrowers slightly underpay interest and suppliers receive slightly less interest. This gives us a glimpse into the dev’s intentions that the supply interest earned should be much closer (potentially even equal to) to the debt interest paid (otherwise the precision lost due to this implementation does not matter). The current implementation allows a buildup of unclaimable funds which far exceeds the precision lost in the code comment.

@0xSpearmint

Who lost the funds?

It would be up to the protocol to make a design decision as to who claims these funds. If we take the code comment above it would appear that the suppliers are entitled to these funds (the suppliers must earn close to the interest repaid by borrowers for suppliers to feel the precision lost described in the code comment). I did not take a definitive stand on this, I don’t believe it’s require for valid medium severity.

@DemoreXTess

” Using compounding rate for borrowers and linear rate for suppliers are recommended way to build a lending protocol.”

This is completely fine as long as there is a way to claim the delta between repaid debt interest and earned supply interest. If not this delta is locked in the contract.

” Those money is not lost, it's always in circulation for keeping liquidity in safe point. Repaying all the debt and getting all the pools' money is not rational movement in this PoC.”

Yes the unclaimable assets continue to be borrowed and repaid, which further increases the amount of unclaimable assets. Don’t be fooled by the fact that money is moving, a significant portion of it is unclaimable.

Repaying all debt and showing that the total claimable assets is less than the total pool’s assets was the simplest way to show this issue. This is a rational situation in Zerolend with permissionless pools, many pools will become inactive and liquidity may concentrate towards ‘high performing’ pools. In the current implementation all inactive pools will have latent funds locked permanently.

Even in active pools the unclaimable reserve is building up, which is why Aave fixed this problem with ‘rescueTokens’