Incorrect value of `totalDebtRepaid` in the event `RebalanceUp()` when attempting to repay more debt than the remaining one (when `amountRepaid < flashLoanAmount`)

[hats-bug-reporter]

Github username: @JacoboLansac
Twitter username: jacolansac
Submission hash (on-chain): 0x0ba83f8a62d8fb7291e928869d929b071f0953af17642015d831839b80d5b933
Severity: low

Description:
The following event is emitted when the function OrigamiLovTokenFlashAndBorrowManager::_rebalanceUpFlashLoanCallback() is called:

    event RebalanceUp(
        uint256 debtTokenFlashAmount,
        uint256 collateralWithdrawn,
@>      uint256 debtTokenRepaid,
        uint256 alRatioBefore, // The asset/liability ratio before the rebalance
        uint256 alRatioAfter, // The asset/liability ratio after the rebalance
        bool forceRebalance
    );

When the borrowLend contract repays the debt, it returns amountRepaid. In most cases amountRepaid==flasLoanAmount, but when the flashLoanAmount is higher than the remaining debt, the following will be true: amountRepaid < flasLoanAmount.

However in that case, The value of totalDebtRepaid is not updated, and it is left as totalDebtRepaid=flashLoanAmount. It is only updated with the surpluses, but there is no updates related to amountRepaid.

Therefore, in the case that _rebalanceUpFlashLoanCallback() is called with a remaining debt lower than the flashLoanAmount, the event RebalanceUp() will have a misleading debtTokenRepaid amount as it will show the value of the full flashloan, instead of the actual amountRepaid (plus the surpluses),

    function _rebalanceUpFlashLoanCallback(
        uint256 flashLoanAmount, 
        uint256 fee, 
        RebalanceUpParams memory params, 
        bool force
    ) internal {
        if (flashLoanAmount != params.flashLoanAmount) revert CommonEventsAndErrors.InvalidParam();

        // Get the current A/L to check for oracle prices, and so we can compare that the new A/L is higher after the rebalance
        Cache memory cache = populateCache(IOrigamiOracle.PriceType.SPOT_PRICE);
        uint128 alRatioBefore = _assetToLiabilityRatio(cache);

@>      uint256 totalDebtRepaid = flashLoanAmount;
        uint256 flashRepayAmount = flashLoanAmount + fee;
        IOrigamiBorrowAndLend _borrowLend = borrowLend;

        // Repay the [debtToken]
        {
            _debtToken.safeTransfer(address(_borrowLend), flashLoanAmount);
            // No need to check the withdrawnAmount returned, the amount passed in can never be type(uint256).max, so this will
            // be the exact `amount`
@>          (uint256 amountRepaid, /*uint256 withdrawnAmount*/) = _borrowLend.repayAndWithdraw(flashLoanAmount, params.collateralToWithdraw, address(this));

            // Repaying less than what was asked is only allowed in force mode.
@>          // This will only happen when there is no more debt in the money market, ie we are fully delevered
            if (!force) {
                if (amountRepaid != flashLoanAmount) revert CommonEventsAndErrors.InvalidAmount(address(_debtToken), flashLoanAmount);
            }
        }
        
        // Swap from [reserveToken] to [debtToken]
        // The expected amount of [debtToken] received after swapping from [reserveToken]
        // needs to at least cover the total flash loan amount + fee
        {
            uint256 debtTokenReceived = swapper.execute(_reserveToken, params.collateralToWithdraw, _debtToken, params.swapData);
            if (debtTokenReceived < flashRepayAmount) {
                revert CommonEventsAndErrors.Slippage(flashRepayAmount, debtTokenReceived);
            }
        }

        // If over the threshold, return any surplus [debtToken] from the swap to the borrowLend
        // And pay down residual debt
        {
            uint256 surplusAfterSwap = _debtToken.balanceOf(address(this)) - flashRepayAmount;
            uint256 borrowLendSurplus = _debtToken.balanceOf(address(_borrowLend));
            uint256 totalSurplus = borrowLendSurplus + surplusAfterSwap;
            if (totalSurplus > params.repaySurplusThreshold) {
                if (surplusAfterSwap != 0) {
                    _debtToken.safeTransfer(address(_borrowLend), surplusAfterSwap);
                }
                totalDebtRepaid += _borrowLend.repay(totalSurplus);
            }
        }

        // Validate that the new A/L is still within the `rebalanceALRange` and expected slippage range
        uint128 alRatioAfter = _validateAfterRebalance(
            cache, 
            alRatioBefore, 
            params.minNewAL, 
            params.maxNewAL, 
            AlValidationMode.HIGHER_THAN_BEFORE, 
            force
        );

        emit RebalanceUp(
            flashLoanAmount,
            params.collateralToWithdraw,
@>          totalDebtRepaid,
            alRatioBefore,
            alRatioAfter,
            force
        );
    }

Impact

The impact on-chain is none, as the EVM has no read access to events. However, if the protocol is using such events to track things like "when to rebalance up or down", then the impact could be significant as it will trigger the wrong actions.

Without full knowledge of how the off-chain part is architectured, I will classify this as low, but I would like to hear what the team thinks.

Recommendation

    function _rebalanceUpFlashLoanCallback(
        uint256 flashLoanAmount, 
        uint256 fee, 
        RebalanceUpParams memory params, 
        bool force
    ) internal {

	    	// ...
	    {
            // Repaying less than what was asked is only allowed in force mode.
            // This will only happen when there is no more debt in the money market, ie we are fully delevered
-           if (!force) {
-               if (amountRepaid != flashLoanAmount) revert CommonEventsAndErrors.InvalidAmount(address(_debtToken), flashLoanAmount);
-           }

+ 			if (amountRepaid != flashLoanAmount) {
+				if (!force) revert CommonEventsAndErrors.InvalidAmount(address(_debtToken), flashLoanAmount);
+				totalDebtRepaid = amountRepaid;
+			}
		}

		// ...
	}

[frontier159]

Is there a super-low category @JacoboLansac ? :-)

Agree this is an extremely minor bug, only affecting an event which will only be populated when 100% deleveraging (shutdown of the vault).

However a bug nonetheless. Thanks for reporting

[JacoboLansac]

Hahaha I also thought the same. Thanks for accepting it though.