denzi_ - Incorrect Calculation of Assets in `getSupplyBalance()` and `getDebtBalance()` inside `PositionBalanceConfiguration.sol`

[sherlock-admin3]

denzi_

High

Incorrect Calculation of Assets in getSupplyBalance() and getDebtBalance() inside PositionBalanceConfiguration.sol

Summary

The protocol uses liquidityIndex and borrowIndex to keep track of the current indexes for both categories.

For more information on how indexes work, give this rareskills.io post a read.

The issue occurs when the code tries to access the supplyShares or debtShares of a user. Both functions intend to convert the shares of the users into amount and add in the increase since lastSupplyIndex, but the return variable contains incorrect calculation and does not return the amount properly.

Vulnerability Detail

When depositing into the protocol, the user's amount is divided by the current index at the time of depositing which gives shares of the user. To redeem their deposit back, the user's shares are multiplied by the current index at the time of withdrawing. After the user has been minted shares, their shares accrue interest and the value of their shares increase the current index increases. So when the user is withdrawing their asset by burning their shares, they receive their original amount + any increase due to interest.

Similarly for borrowing, when the users borrow any asset, shares are minted using the current index at the time of borrowing to determine how much amount they have borrowed. These shares also gain interest. When borrowers repay at a later time when the borrowIndex has increased, they will have to pay a greater amount of asset in return to cover the interest on their shares gained.

POC

Consider the following scenerio where Alice deposits 10$ into the pool. The values for index used are for this POC only, in a practical scenerio these will be smaller and increment slowly.

Amount = 10$
index = 2
```solidity

The depositCollateral() Function is used in supply to calculate how many shares Alice will get for her deposit.

```solidity
function depositCollateral(
    DataTypes.PositionBalance storage self,
    DataTypes.ReserveSupplies storage totalSupply,
    uint256 amount,
    uint128 index
  ) internal returns (bool isFirst, uint256 sharesMinted) {
    sharesMinted = amount.rayDiv(index);
    require(sharesMinted != 0, PoolErrorsLib.INVALID_MINT_AMOUNT);
    isFirst = self.supplyShares == 0;
    self.lastSupplyLiquidtyIndex = index;
    self.supplyShares += sharesMinted;
    totalSupply.supplyShares += sharesMinted;
  }

sharesMinted / amount.rayDiv(index) calculates the amount of shares for Alice which will be 5 in this case.

The following values will be achieved

self.lastSupplyLiquidtyIndex = 2;
self.supplyShares += 5;
totalSupply.supplyShares += 5;

Now Alice has 5 shares to her name on a deposit of 10$ at index 2. Overtime the index will increase which means that her shares value have gone up and she is entitled to a greater amount than her deposit.

Alice tries to withdraw her amount at a later time. The withdraw() function in NFTPositionManager.sol leads to _withdraw() in NFTPositionManagerSetters.sol which leads to _withdraw() in PoolSetters.sol. This function calls SupplyLogic::executeWithdraw() which contains the following code at the start of the function

function executeWithdraw(
    mapping(address => DataTypes.ReserveData) storage reservesData,
    mapping(uint256 => address) storage reservesList,
    DataTypes.UserConfigurationMap storage userConfig,
    mapping(address => mapping(bytes32 => DataTypes.PositionBalance)) storage balances,
    DataTypes.ReserveSupplies storage totalSupplies,
    DataTypes.ExecuteWithdrawParams memory params
  ) external returns (DataTypes.SharesType memory burnt) 
  {
    DataTypes.ReserveData storage reserve = reservesData[params.asset]; // we fetch the reserveData for the asset
    DataTypes.ReserveCache memory cache = reserve.cache(totalSupplies); // we cache 
    reserve.updateState(params.reserveFactor, cache); // update the state
    // more code....
```solidity

Here the protocol caches the current reserveData and totalSupplies for the asset and calls `updateState()` which updates the Indexes if the `lastUpdateTimestamp` is not equal to the current block.timestamp. Without going into great detail which can be seen [here](https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/pool/logic/ReserveLogic.sol#L215-L239)

The protocol updates the _cache.nextLiquidityIndex and sets _reserve.liquidityIndex = _cache.nextLiquidityIndex as well as updates the _cache.nextBorrowIndex and sets _reserve.borrowIndex = _cache.nextBorrowIndex. Here the protocol has updated the indexes for both supply and debt. 

Continuing the case with Alice, She has called the withdraw function and the indexes have been updated. The cache.nextLiquidityIndex is found to be 2.2. Using this new index, the protocol will burn her shares and give her a greater amount through the following code present after `updateState()`

```solidity
uint256 balance = balances[params.asset][params.position].getSupplyBalance(cache.nextLiquidityIndex);

The getSupplyBalance() contains the following code

uint256 increase = self.supplyShares.rayMul(index) - self.supplyShares.rayMul(self.lastSupplyLiquidtyIndex);
return self.supplyShares + increase;

The increase variable calculates the increase in Alice deposits. Since shares are calculated by Deposit Amount / Index , To calculate the amount back from shares, we use Shares * Index.

The issues occurs because the protocol calculates her increase in amount $ and then adds it directly into the shares so lets calculate for better understanding

increase = 5(2.2) - 5(2) = 11 - 10 = 1$

Alice has gained 1$ increase on her initial deposit of 10$ so her total withdrawable amount is 11$. This part is correct but the return statement returns self.SupplyShares + increase. Here the code has added 5 shares + 1$ which is incorrect. This leads to Alice having 6 shares which at the time of index would result in 6 * 2.2 = 13.2$

The natspec of the function says that the function converts shares into assets (which in this case is $). So the proper way of doing this would be to just return the $ amount at current index i.e. 5(2.2) or return 5(2) + 1$ which would also result in 11$.

Furthermore moving ahead after getting balance of the user, we validate the withdraw call by checking if the user provided amount <= balance or not, if not then in this case the withdraw will revert since Alice wanted to redeem 11$ rightfully and the balance returned is 6 (5 shares + 1$).

ValidationLogic.validateWithdraw(params.amount, balance);

function validateWithdraw(uint256 amount, uint256 userBalance) internal pure {
    require(amount != 0, PoolErrorsLib.INVALID_AMOUNT);
    require(amount <= userBalance, PoolErrorsLib.NOT_ENOUGH_AVAILABLE_USER_BALANCE);
}

Even if this does not revert in the case of partial withdrawals, the protocol's math will be compromised because of the remaining functions present in executeWithdraw()

Now note that the withdraw function requires user to provide amount in $ because the withdrawCollateral() function which takes this amount and converts it into shares according to the current index which are then used to subtract the sharesBurnt from user's debtShares and totalSupplies debtShares so calculate how many shares the users have withdrawn.

Impact

A lot of functions use getSupplyBalance() or getDebtBalance() e.g when withdrawing or repaying. The faulty return statement will break the math of the protocol and cause unexpected amounts.

Code Snippet

getSupplyBalance and getDebtBalance

Tool used

Manual Review

Recommendation

Return the proper amount of balance in both functions by converting the increase into shares and adding both up and then converting it back to asset amount

Duplicate of #473