denzi_ - Repaying function will revert in most cases

[sherlock-admin4]

denzi_

High

Repaying function will revert in most cases

Summary

User's trying to repay or partial repay will have their transactions reverted due to a check which compares previous debt - current debt to make sure it is equal to the amount repaid.

Vulnerability Detail

When a user tries to repay their current debt, they will call NFTPositionManager::repay() by passing in params which include the amount of asset they want to repay. This repay() function calls NFTPositionManagerSetters::_repay() function which contains the issue.

The _repay() function contains the following code after some checks at the start

uint256 previousDebtBalance = pool.getDebt(params.asset, address(this), params.tokenId);
DataTypes.SharesType memory repaid = pool.repay(params.asset, params.amount, params.tokenId, params.data);
uint256 currentDebtBalance = pool.getDebt(params.asset, address(this), params.tokenId);

The getDebt() function is supposed to convert the debt shares into amount and return them (there is also a bug in the return value so lets assume that this function is working as intended and has been fixed, the issue described in the report occurs after the fix and before the fix).

  function getDebt(address asset, address who, uint256 index) external view returns (uint256 debt) {
    bytes32 positionId = who.getPositionId(index);
    return _balances[asset][positionId].getDebtBalance(_reserves[asset].borrowIndex);
  }

The getDebt() function calls getDebtBalance() which takes in the current borrowIndex of the protocol and calculates the debt shares with the increase due to interest on the debt. Once this call is done, previousDebtBalance will now have amount of assets in debt.

The next call is to pool.repay() this function which leads to BorrowLogic::executeRepay() in here the following lines are executed at the start

DataTypes.ReserveCache memory cache = reserve.cache(totalSupplies);
reserve.updateState(params.reserveFactor, cache);
payback.assets = balances.getDebtBalance(cache.nextBorrowIndex);

The updateState() function updates the following variables For Liquidity : _cache.nextLiquidityIndex and _reserve.liquidityIndex For Borrowing : _cache.nextBorrowIndex and _reserve.borrowIndex

and then proceeds to getDebtBalance similar to how we did it in the previous function but this time we send in the nextBorrowIndex. Now lets assume the rest of the function proceeds and this function has been completely executed, the control moves back towards NFTPositionManagerSetters::_repay().

The next line to be executed is

uint256 currentDebtBalance = pool.getDebt(params.asset, address(this), params.tokenId);

Now when we getDebt this time, which again forwards call to getDebtBalance()

  function getDebt(address asset, address who, uint256 index) external view returns (uint256 debt) {
    bytes32 positionId = who.getPositionId(index);
    return _balances[asset][positionId].getDebtBalance(_reserves[asset].borrowIndex);
  }

we pass in the current borrowIndex, now this is where the issue occurs. Inside BorrowLogic::executeRepay() the code called updateState() which set a new borrowIndex for the reserve.

This causes the currentDebtBalance to be the debt balance of the user with the updated borrowIndex while the previousDebtBalance to be the debt balance of th user with the previous borrowIndex.

After this the function confirms if the difference between the two is exactly the same as repaid.assets which is the amount of assets paid back by the user in BorrowLogic::executeRepay() function, inside that function there can be two values

If the user is partial repaying, meaning that their debt still exists and the amount they provided to repay is less than their current debt then payback.assets = params.amount or if the user is fully repaying then their payback.assets will be their debt balance, this value is returned in the SharesType repaid.

The following line can cause reverts for the function disallowing users to repay their assets

if (previousDebtBalance - currentDebtBalance != repaid.assets) {
      revert NFTErrorsLib.BalanceMisMatch();
}

POC

Consider Bob to have a have debt of 20$ when the borrowIndex was 2. Bob's debtShares will be equal to 20/2 = 10

After sometime, the protcol's current registered borrowIndex is 2.1, working with the current code in place, the increase will be 10(2.1) - 10(2) = 1$ which needs to be converted into shares so 1$ / 2.1 = 0.47619047619.

Total debtShares at the moment for bob = 10 + 0.47619047619 => 10.4761904762 This value in shares will be 10.4761904762 * 2.1 = 22$ at the current index. This means bob has to be repay 22$ to fully cover their debt.

previousDebtBalance = 22$

Suppose bob only wants to repay 12$ at the moment and lower his debtShares, he will send the transaction with params.amount = 12$

Inside the function previousDebtBalance = 22$ is set and the code moves into executeRepay() where the updateState() function is called, inside the executeRepay() function, for the report consider the following new values after updateState() has been called

_cache.nextBorrowIndex = 2.2
_reserve.borrowIndex = 2.2

Then we get the debtBalance of the user using nextBorrowIndex

payback.assets = balances.getDebtBalance(cache.nextBorrowIndex);

Continuing the function, we get payback.assets = 10(2.2) - 10(2) = 22-20 = 2$ which in shares are 2 / 2.2 = 0.90909090909 so now bob's debtShares are 10.90909090909 with 2.2 index and the amount bob has to repay is 24$. Since bob only provided 12$, the following code will set payback.assets = params.amount

if (params.amount < payback.assets) payback.assets = params.amount;

The executeRepay() function then moves into balances.repayDebt() through the following code and payback struct is returned, Also 12$ of shares have been deducted from Bob's debtShares which would be 12 / 2.2 = 5.45454545455, remaining shares are now 10.90909090909 - 5.45454545455 = 5.45454545455

payback.shares = balances.repayDebt(totalSupplies, payback.assets, cache.nextBorrowIndex);

Back in the function after repay() call is finished we get the currentDebtBalance of Bob through

DataTypes.SharesType memory repaid = pool.repay(params.asset, params.amount, params.tokenId, params.data);
uint256 currentDebtBalance = pool.getDebt(params.asset, address(this), params.tokenId);

This will calculate as following 5.45454545455(2.2) - 5.45454545455(2.2) = 0. No increase since when burning Bob's shares we set the current index as lastIndex for Bob, so it will just convert the current shares into $ amount.

currentDebtBalance = 12$

Now we have the check

 if (previousDebtBalance - currentDebtBalance != repaid.assets) {
      revert NFTErrorsLib.BalanceMisMatch(); // and this reverts
    }

This will calculate as 22$ - 12$ = 10$ where as repaid.assets is equal to 12$ and the function will revert.

Impact

In most cases partial repaying will revert causing users to not repay their debt.

Code Snippet

NFTPositionManagerSetters::_repay()

Tool used

Manual Review

Recommendation

Move the check to inside BorrowLogic::executeRepay() to keep it consistent.

Duplicate of #467