Revert Lend - Bigsam's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L954-L1014

Vulnerability details

Impact

Function _repay is an internal logic executor to repay borrowed assets. To repay we have to consider 3 possible conditions A. shares/asset inputted to clear the debt > debt B. shares/asset inputted to clear the debt < debt C. shares/asset inputted to clear the debt == debt.

Based on the logic of the code A will revert while only B and C are executed

   if (shares > current shares) {
            revert RepayExceedsDebt();
        }.

But there is an issue for C to Execute the user must input the correct present "debtamount" (debt+interest) incurred at the second of execution if they are repaying with amount, the debtshares value remains the same making it easy to repay with shares as long as you have enough assets to conver for the repayment, but based on this assertion there will be a big issue using amount as debt is always updated each second and always higher in asset value than the last second, this is further compounded and significant if the debtshare amount is higher.

Proof of Concept

The DEBT profile of Users are updated every second thus it increases per seconds. If a user assert his loan (by checking his loan info) to be 1001 shares,

at a value of 1102 based on an increase in interest by 10%

assuming q96 to the interest rate is 1/1 before,

if it takes the user 10 seconds after assertion of the value to execute the debt repayment value would have increased based on the the interest generated within that 10 second. Even if a user is so precise and misses by just a second he still has some debt asset/shares to pay. 1.

(uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96) = _updateGlobalInterest(); calls 2

function _updateGlobalInterest()
        internal
        returns (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96)
    {
        // only needs to be updated once per block (when needed)
        if (block. timestamp > lastExchangeRateUpdate) {
            (newDebtExchangeRateX96, newLendExchangeRateX96) = _calculateGlobalInterest();
            lastDebtExchangeRateX96 = newDebtExchangeRateX96;
            lastLendExchangeRateX96 = newLendExchangeRateX96;
            lastExchangeRateUpdate = block.timestamp;
            emit ExchangeRateUpdate(newDebtExchangeRateX96, newLendExchangeRateX96);
        } else {
            newDebtExchangeRateX96 = lastDebtExchangeRateX96;
            newLendExchangeRateX96 = lastLendExchangeRateX96;
        }  calls 3

function _calculateGlobalInterest()
        internal
        View
        returns (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96)
    {
        uint256 oldDebtExchangeRateX96 = lastDebtExchangeRateX96;
        uint256 oldLendExchangeRateX96 = lastLendExchangeRateX96;

        (, uint256 available,) = _getAvailableBalance(oldDebtExchangeRateX96, oldLendExchangeRateX96);

        uint256 debt = _convertToAssets(debtSharesTotal, oldDebtExchangeRateX96, Math.Rounding.Up);

        (uint256 borrowRateX96, uint256 supplyRateX96) = interestRateModel.getRatesPerSecondX96(available, debt);

        supplyRateX96 = supplyRateX96.mulDiv(Q32 - reserveFactorX32, Q32);

        // always growing or equal
        uint256 lastRateUpdate = lastExchangeRateUpdate;

        if (lastRateUpdate > 0) {
            newDebtExchangeRateX96 = oldDebtExchangeRateX96
                + oldDebtExchangeRateX96 * (block.timestamp - lastRateUpdate) * borrowRateX96 / Q96;
            newLendExchangeRateX96 = oldLendExchangeRateX96
                + oldLendExchangeRateX96 * (block.timestamp - lastRateUpdate) * supplyRateX96 / Q96;
        } else {
            newDebtExchangeRateX96 = oldDebtExchangeRateX96;
            newLendExchangeRateX96 = oldLendExchangeRateX96;
        }
   
 } 

The above updates the "debtamount" of the user before executing other logics. Thus it will be very rare and extremely difficult for a user using asset for payment to get the exact shares/asset value to be paid at a particular second of execution and if they are one second late they will still have a minimal debt available which is the interest for that one second missed, a second might be insignificant for small amounts but it becomes significant as the debtshares increases hence they will most likely (with a high probability) not be able to clear this debt completely. This renders the debt-clearing assertion unrealistic while using asset to repay, DEBT can be reduced easily but it's extremely difficult to clear all at once.

Tools Used

Manual code analysis.

Recommended Mitigation Steps

To address this issue and facilitate more effective debt repayment, consider implementing the following changes: Allow for a more flexible repayment mechanism where shares/asset inputted can exceed the debt incurred. This would enable users to repay their debts more efficiently, even if they are unable to precisely match the debt amount. To enhance the complete payment of the debt incurred in a single transaction more effectively and efficiently consider allowing Shares inputted > debt incurred. using the logic below

from line 972

// Initialize RemainingShares variable
uint256 RemainingShares = 0;

// Handle excess repayments
if (shares > currentShares) {
    // Calculate remaining shares after repaying current debt
    RemainingShares = shares - currentShares;
    // Update Shares variable to currentShares value
    Shares = currentShares;
    // Convert remaining shares to assets
    Remainingassets = _convertToAssets(RemainingShares, newDebtExchangeRateX96, Math.Rounding.Up);
}

// Transfer assets from the user
if (assets > 0) {
    if (permitData.length > 0) {
        // Decode permitData for signature validation
        (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
            abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
        // Perform permit transfer
        permit2.permitTransferFrom(
            permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets - Remainingassets), msg.sender, signature
        );
    } else {
        // Transfer assets if no permit required
        // Revert if insufficient token approval
        SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), assets - Remainingassets);
    }
}

// Update debtSharesTotal and dailyDebtIncreaseLimitLeft
uint256 loanDebtShares = loan.debtShares - shares;
loan.debtShares = loanDebtShares;
debtSharesTotal -= shares;
dailyDebtIncreaseLimitLeft += assets - Remainingassets;

// Update collateral and perform cleanup if fully repaid
_updateAndCheckCollateral(
    tokenId, newDebtExchangeRateX96, newLendExchangeRateX96, loanDebtShares + shares, loanDebtShares
);

// Get loan owner address
address owner = tokenOwner[tokenId];

// Check if fully repaid and perform cleanup if needed
if (currentShares == shares) {
    _cleanupLoan(tokenId, newDebtExchangeRateX96, newLendExchangeRateX96, owner);
} else {
    // Revert if resulting loan is too small
    if (_convertToAssets(loanDebtShares, newDebtExchangeRateX96, Math.Rounding.Up) < minLoanSize) {
        revert MinLoanSize();
    }
}

// Emit Repay event
emit Repay(tokenId, msg.sender, owner, assets, shares);

assets - Remainingassets can also be refactored to save gas. uint256 newasset = assets - Remainingassets.

Assessed type

Math