A2-security - Inconsistent Application of Reserve Factor Changes Leads to Protocol Insolvency Risk

[sherlock-admin3]

A2-security

Medium

Inconsistent Application of Reserve Factor Changes Leads to Protocol Insolvency Risk

Summary

The ZeroLend protocol's PoolFactory allows for global changes to the reserveFactor, which affects all pools simultaneously. However, the ReserveLogic contract applies this change inconsistently between interest accrual and treasury minting processes. This inconsistency leads to a mismatch between accrued interest for users and the amount minted to the treasury, causing protocol insolvency or locked funds.

Vulnerability Detail

The reserveFactor is a crucial parameter in the protocol that determines the portion of interest accrued from borrowers that goes to the protocol's treasury. It's defined in the PoolFactory contract:

contract PoolFactory is IPoolFactory, Ownable2Step {
// ...
uint256 public reserveFactor;
// ...
}

This reserveFactor is used across all pools created by the factory. It's retrieved in various operations, such as in the supply function for example :

function _supply(address asset, uint256 amount, bytes32 pos, DataTypes.ExtraData memory data) internal nonReentrant(RentrancyKind.LENDING) returns (DataTypes.SharesType memory res) {
// ...
res = SupplyLogic.executeSupply(
_reserves[asset],
_usersConfig[pos],
_balances[asset][pos],
_totalSupplies[asset],
DataTypes.ExecuteSupplyParams({reserveFactor: _factory.reserveFactor(), /_ ... _/})
);
// ...
}

The reserveFactor plays a critical role in calculating interest rates and determining how much of the accrued interest goes to the liquidity providers and how much goes to the protocol's treasury . The issue arises from the fact that this reserveFactor can be changed globally for all pools:

function setReserveFactor(uint256 updated) external onlyOwner {
uint256 old = reserveFactor;
reserveFactor = updated;
emit ReserveFactorUpdated(old, updated, msg.sender);
}

let's examine how this change affects the core logic in the ReserveLogic contract:

function updateState(DataTypes.ReserveData storage self, uint256 _reserveFactor, DataTypes.ReserveCache memory _cache) internal {
if (self.lastUpdateTimestamp == uint40(block.timestamp)) return;

    _updateIndexes(self, _cache);
    _accrueToTreasury(_reserveFactor, self, _cache);

    self.lastUpdateTimestamp = uint40(block.timestamp);

}

The vulnerability lies in the fact that _updateIndexes and _accrueToTreasury will use different reserveFactor values when a change occurs:

if the reserveFactors is changed _updateIndexes will uses the old reserveFactor implicitly through cached liquidityRate:

function _updateIndexes(DataTypes.ReserveData storage _reserve, DataTypes.ReserveCache memory _cache) internal {
if (_cache.currLiquidityRate != 0) {
uint256 cumulatedLiquidityInterest = MathUtils.calculateLinearInterest(_cache.currLiquidityRate, _cache.reserveLastUpdateTimestamp);
_cache.nextLiquidityIndex = cumulatedLiquidityInterest.rayMul(_cache.currLiquidityIndex).toUint128();
_reserve.liquidityIndex = _cache.nextLiquidityIndex;
}
// ...
}

_accrueToTreasury will use the new reserveFactor:

function _accrueToTreasury(uint256 reserveFactor, DataTypes.ReserveData storage _reserve, DataTypes.ReserveCache memory _cache) internal {
// ...
vars.amountToMint = vars.totalDebtAccrued.percentMul(reserveFactor);
if (vars.amountToMint != 0) _reserve.accruedToTreasuryShares += vars.amountToMint.rayDiv(_cache.nextLiquidityIndex).toUint128();
}

This discrepancy results in the protocol minting more/less treasury shares than it should based on the actual accrued interest cause it uses the new reserveFactor. Over time, this can lead to a substantial overallocation/underallocation of funds to the treasury, depleting the reserves available for users or leaving funds locked in the pool contract forever.

example scenario :

• to simplify this issue consider the following example :
• Deposited: 10,000 USD
• Borrowed: 10,000 USD
• Initial reserveFactor: 10%
• Borrow rate: 12%
• Utilization ratio: 100%
• Liquidity rate: 12% * (100% - 10%) = 10.8%

After 2 months:

• Accrued interest: 200 USD
• reserveFactor changed to 30%
• updateState is called:
  • _updateIndexes: Liquidity index = (0.018 + 1) * 1 = 1.018 (based on old 10.8% rate)
  • _accrueToTreasury: Amount to mint = 200 * 0.3 = 60 USD (using new 30% reserveFactor)

When a user attempts to withdraw:

• User's assets: 10,000 * 1.018 = 10,180 USD
• Treasury owns: 60 USD
• Total required: 10,240 USD

However, the borrower only repaid 10,200 USD (10,000 principal + 200 interest), resulting in a 40 USD shortfall. This discrepancy can lead to failed withdrawals and insolvency of the protocol.

Impact

the Chage of reserveFactor leads to protocol insolvency risk or locked funds. Increased reserveFactor causes over-minting to treasury, leaving insufficient funds for user withdrawals. Decreased reserveFactor results in under-minting, locking tokens in the contract permanently. Both scenarios compromise the protocol's financial integrity

Code Snippet

• https://github.com/sherlock-audit/2024-06-new-scope/blob/c8300e73f4d751796daad3dadbae4d11072b3d79/zerolend-one/contracts/core/pool/PoolFactory.sol#L112-L116
• https://github.com/sherlock-audit/2024-06-new-scope/blob/c8300e73f4d751796daad3dadbae4d11072b3d79/zerolend-one/contracts/core/pool/logic/ReserveLogic.sol#L210

Tool used

Manual Review

Recommendation

• Given ZeroLend's permissionless design where anyone can create a pool, updating all pools simultaneously before updating the reserveFactor is impractical. we recommend storing the lastReserveFactor used for each pool. This approach is similar to other protocols and ensures consistency between interest accrual and treasury minting.

Add a new state variable in the ReserveData struct:

struct ReserveData {
    // ... existing fields
+    uint256 lastReserveFactor;
}

Modify the updateState function to use and update this lastReserveFactor:

function updateState(DataTypes.ReserveData storage self, uint256 _reserveFactor, DataTypes.ReserveCache memory _cache) internal {
    if (self.lastUpdateTimestamp == uint40(block.timestamp)) return;

    _updateIndexes(self, _cache);
-   _accrueToTreasury(_reserveFactor, self, _cache);
+   _accrueToTreasury(self.lastReserveFactor, self, _cache);

    self.lastUpdateTimestamp = uint40(block.timestamp);
+   self.lastReserveFactor = _reserveFactor;
}

This solution ensures that the same reserveFactor is used for both interest accrual and treasury minting within each update cycle, preventing inconsistencies while allowing for global reserveFactor changes to take effect gradually across all pools.

[sherlock-admin2]

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

Honour commented:

invalid: the cached reserveFactor is also the same used to accrue to treasury.

[nevillehuang]

request poc

Seems related to #199

[sherlock-admin3]

PoC requested from @A2-Security

Requests remaining: 19

[aliX40]

hey @nevillehuang ,this is not a dup of #199 , we have #316 which is duplicate of #199 . this one is different

• the comment :

invalid: the cached reserveFactor is also the same used to accrue to treasury.
is incorrect

• here a poc shows how change the factor will lead to insolvency and cause the last withdrawal not able to
  first we need to correct the balance calculation in PositionBalanceConfiguration:

  function getSupplyBalance(DataTypes.PositionBalance storage self, uint256 index) public view returns (uint256 supply) {
-     uint256 increase = self.supplyShares.rayMul(index) - self.supplyShares.rayMul(self.lastSupplyLiquidtyIndex);
-     return self.supplyShares + increase;
+    return self.supplyShares.rayMul(index);
  }

  function getDebtBalance(DataTypes.PositionBalance storage self, uint256 index) internal view returns (uint256 debt) {
-     uint256 increase = self.debtShares.rayMul(index) - self.debtShares.rayMul(self.lastDebtLiquidtyIndex);
-     return self.debtShares + increase;
+    return self.debtShares.rayMul(index);
  }

• add this test to PoolRepayTests

  function test_auditPoc_reserve() external {
    console.log('balance pool before : ', tokenA.balanceOf(address(pool)));
    _mintAndApprove(alice, tokenA, 2 * amount, address(pool));
    vm.startPrank(alice);

    pool.supplySimple(address(tokenA), alice, amount, 0); // deposit : 2000e18
    pool.borrowSimple(address(tokenA), alice, borrowAmount, 0); // borrow : 800e18

    vm.stopPrank();
    // wrap sometime so the intrest accrue :
    vm.warp(block.timestamp + 30 days);
    // change reserve factor to 0.2e4 (20%):
    poolFactory.setReserveFactor(0.2e4);

    vm.startPrank(alice);
    tokenA.approve(address(pool), UINT256_MAX);
    pool.repaySimple(address(tokenA), UINT256_MAX, 0);
    // withdraw all will revert cause there is not enough funds for treasury due to updating the factor :
    vm.expectRevert();
    pool.withdrawSimple(address(tokenA), alice, UINT256_MAX, 0);
    vm.stopPrank();

  }

• the transaction will revert , because the amount accrued to treasury , doesn't exist , and please notice that this will effect all the pool in the protocol , and this amount will keep growing , since it's accumulate yield as well , which is insolvency

The issue described in the report, is similar to a bug found in the aave v3 codebase when updating the reserveFactor. This bug have been disclosed and fixed with the v3.1 release
https://github.com/aave-dao/aave-v3-origin/blob/3aad8ca184159732e4b3d8c82cd56a8707a106a2/src/core/contracts/protocol/pool/PoolConfigurator.sol#L300C1-L315C4

  function setReserveFactor(
    address asset,
    uint256 newReserveFactor
  ) external override onlyRiskOrPoolAdmins {
    require(newReserveFactor <= PercentageMath.PERCENTAGE_FACTOR, Errors.INVALID_RESERVE_FACTOR);

  @>>   _pool.syncIndexesState(asset);

    DataTypes.ReserveConfigurationMap memory currentConfig = _pool.getConfiguration(asset);
    uint256 oldReserveFactor = currentConfig.getReserveFactor();
    currentConfig.setReserveFactor(newReserveFactor);
    _pool.setConfiguration(asset, currentConfig);
    emit ReserveFactorChanged(asset, oldReserveFactor, newReserveFactor);

    _pool.syncRatesState(asset);
  }

Also the fix we recomended is inspired by how the eulerv2 handled this, in their vault. (cache reserve factor when calling updateInterestrate, and use the cached factor when updating the index!)

[0xspearmint1]

escalate

setReserveFactor() is a protocol admin function

Sherlock rules state

Admin could have an incorrect call order. Example: If an Admin forgets to setWithdrawAddress() before calling withdrawAll() This is not a valid issue.

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

1. If the admin calls forceUpdateReserve() on the pools before calling setReserveFactor() this issue will not exist

2. Since setReserveFactor() is only called by the protocol admin, according to the sherlock rules admin actions that lead to issues are not valid

[sherlock-admin3]

escalate

setReserveFactor() is a protocol admin function

Sherlock rules state

Admin could have an incorrect call order. Example: If an Admin forgets to setWithdrawAddress() before calling withdrawAll() This is not a valid issue.

An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

1. If the admin calls forceUpdateReserve() on the pools before calling setReserveFactor() this issue will not exist

2. Since setReserveFactor() is only called by the protocol admin, according to the sherlock rules admin actions that lead to issues are not valid

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.

[aliX40]

First point:

• If the admin calls forceUpdateReserve() on the pools before calling setReserveFactor() this issue will not exist

This is not true and presents a Dos attack vector. Creating pools on zerolend is permissionless, u can't simply expect the admin to call forceUpdateReserve() on 10 of thousands of pools before changing the resrve factor. This is simply unrealistic, costly and opens an attack vector for people to dos the treasury
Second point:

• this issue doesn't expect and admin to make a mistake. Any call or changes to the reserve factor will provide damages to the deployed pools in the system. (Meaning if reserveFactor is increased or decreased there will be a significant Impact on the system (Please read our report for full details))

[0xDenzi]

I would also like to clarify further for the escalator that the 2nd point does not apply to functions which itself are broken/incomplete. The issue is not about admin missing or not executing or delaying a call or providing a wrong input. The issue is that the function is missing line/s of code to properly adjust the reserve factor.

[cvetanovv]

This issue falls right between the "Admin Input/call validation" rules and broken functionality:

Admin could have an incorrect call order. An admin action can break certain assumptions about the functioning of the code.

Breaks core contract functionality, rendering the contract useless or leading to loss of funds of the affected party larger than 0.01% and 10 USD.

But I think we have broken functionality here, not an admin error.

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

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xspearmint1: rejected