mergeTranches() If the lender is a LoanManager it will break the Pool accounting

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/loans/MultiSourceLoan.sol#L1133

Vulnerability details

Vulnerability details

Anyone can run mergeTranches() to merge similar tranches with the same lender and startTime into a new tranche.

    function mergeTranches(uint256 _loanId, Loan memory _loan, uint256 _minTranche, uint256 _maxTranche)
        external
        returns (uint256, Loan memory)
    {
        _baseLoanChecks(_loanId, _loan);
        uint256 loanId = _getAndSetNewLoanId();
        Loan memory loanMergedTranches = _mergeTranches(loanId, _loan, _minTranche, _maxTranche);
        _loans[loanId] = loanMergedTranches.hash();
        delete _loans[_loanId];

        emit TranchesMerged(loanMergedTranches, _minTranche, _maxTranche);

        return (loanId, loanMergedTranches);
    }

    function _mergeTranches(
        uint256 _newLoanId,
        IMultiSourceLoan.Loan memory _loan,
        uint256 _minTranche,
        uint256 _maxTranche
    ) private pure returns (IMultiSourceLoan.Loan memory) {
        /// @dev if the diff is also just 1, then we wouldn't be merging anything
        if (_minTranche >= _maxTranche - 1) {
            revert InvalidParametersError();
        }

        IMultiSourceLoan.Tranche[] memory tranche =
            new IMultiSourceLoan.Tranche[](_loan.tranche.length - (_maxTranche - _minTranche) + 1);

        uint256 originalIndex = 0;
        /// @dev Copy tranches before
        for (; originalIndex < _minTranche;) {
            tranche[originalIndex] = _loan.tranche[originalIndex];
            unchecked {
                ++originalIndex;
            }
        }

        /// @dev Merge tranches. Just picking one, they must all match.
        address lender = _loan.tranche[_minTranche].lender;
        uint256 startTime = _loan.tranche[_minTranche].startTime;

        uint256 principalAmount;
        uint256 cumAprBps;
        uint256 accruedInterest;
        /// @dev Use to validate _totalTranches
        for (; originalIndex < _maxTranche;) {
            IMultiSourceLoan.Tranche memory thisTranche = _loan.tranche[originalIndex];
            if (lender != thisTranche.lender || startTime != thisTranche.startTime) {
                revert MismatchError();
            }
            principalAmount += thisTranche.principalAmount;
            cumAprBps += thisTranche.aprBps * thisTranche.principalAmount;
            accruedInterest += thisTranche.accruedInterest;
            unchecked {
                ++originalIndex;
            }
        }
        /// @dev Output of merged tranches.
        tranche[_minTranche] = IMultiSourceLoan.Tranche(
@>          _newLoanId,
            _loan.tranche[_minTranche].floor,
            principalAmount,
            lender,
            accruedInterest,
            startTime,
            cumAprBps / principalAmount
        );

        /// @dev Copy remaining ones
        uint256 remainingIndex = _minTranche + 1;
        for (; originalIndex < _loan.tranche.length;) {
            tranche[remainingIndex] = _loan.tranche[originalIndex];
            unchecked {
                ++originalIndex;
                ++remainingIndex;
            }
        }
        _loan.tranche = tranche;

        return _loan;
    }

The newly merged tranche will have a new loanId.

But there is a problem if the lender is a LoanManager, the new loanId breaks the Pool's accounting system.

Because when Loan repays, Pool will locate the corresponding _queueAccounting[], _queueOutstandingValues, getTotalReceived[] based on the tranche's loanId.

Pool.sol

    function _loanTermination(
        address _loanContract,
        uint256 _loanId,
        uint256 _principalAmount,
        uint256 _apr,
        uint256 _interestEarned,
        uint256 _received
    ) private {
        uint256 pendingIndex = _pendingQueueIndex;
        uint256 totalQueues = getMaxTotalWithdrawalQueues + 1;
        uint256 idx;
        /// @dev oldest queue is the one after pendingIndex
        uint256 i;
        for (i = 1; i < totalQueues;) {
            idx = (pendingIndex + i) % totalQueues;
@>          if (getLastLoanId[idx][_loanContract] >= _loanId) {
                break;
            }
            unchecked {
                ++i;
            }
        }
        /// @dev We iterated through all queues and never broke, meaning it was issued after the newest one.
        if (i == totalQueues) {
            _outstandingValues =
                _updateOutstandingValuesOnTermination(_outstandingValues, _principalAmount, _apr, _interestEarned);
            return;
        } else {
            uint256 pendingToQueue =
@>              _received.mulDivDown(PRINCIPAL_PRECISION - _queueAccounting[idx].netPoolFraction, PRINCIPAL_PRECISION);
@>          getTotalReceived[idx] += _received;
            getAvailableToWithdraw += pendingToQueue;
@>          _queueOutstandingValues[idx] = _updateOutstandingValuesOnTermination(
                _queueOutstandingValues[idx], _principalAmount, _apr, _interestEarned
            );
        }
    }

The new loanId will result in incorrect localization, assigning the wrong asset to the wrong queue

Impact.

mergeTranches() merged loanId will break Pool's accounting system, leading to incorrect asset assignment

Recommended Mitigation

Recommendation: mergeTranches() when lender can't be a LoanManager

    function _mergeTranches(
        uint256 _newLoanId,
        IMultiSourceLoan.Loan memory _loan,
        uint256 _minTranche,
        uint256 _maxTranche
    ) private pure returns (IMultiSourceLoan.Loan memory) {
...
        for (; originalIndex < _maxTranche;) {
            IMultiSourceLoan.Tranche memory thisTranche = _loan.tranche[originalIndex];
-           if (lender != thisTranche.lender || startTime != thisTranche.startTime) {
+           if (lender != thisTranche.lender || getLoanManagerRegistry.isLoanManager(thisTranche.lender) || startTime != thisTranche.startTime) {
                revert MismatchError();
            }
            principalAmount += thisTranche.principalAmount;
            cumAprBps += thisTranche.aprBps * thisTranche.principalAmount;
            accruedInterest += thisTranche.accruedInterest;
            unchecked {
                ++originalIndex;
            }
        }
        /// @dev Output of merged tranches.
        tranche[_minTranche] = IMultiSourceLoan.Tranche(
            _newLoanId,
            _loan.tranche[_minTranche].floor,
            principalAmount,
            lender,
            accruedInterest,
            startTime,
            cumAprBps / principalAmount
        );

Assessed type

Context

[c4-judge]

0xA5DF marked the issue as primary issue

[0xA5DF]

Similar to #69, but here the warden claims that the issue occurs only under the condition that lender is LoanManager Tbh, I didn't fully understand why is that, maybe the sponsor or other wardens can help me out with this one

[minhquanym]

@0xA5DF Yes, it should be dup of #69. LoanManager is an abstract contract that Pool implemented, they are the same thing

[c4-judge]

0xA5DF marked the issue as duplicate of #69

[0xA5DF]

Got it, thanks

[c4-judge]

0xA5DF marked the issue as satisfactory