Merging tranches could make `_loanTermination()` accounting incorrect

criticalCode4rena

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/pools/Pool.sol#L516

Vulnerability details

Impact

In the Pool contract, when a loan is repaid or liquidated, a call to the Pool is made for accounting. The _loanTermination() function is eventually invoked. This function uses the loanId to determine the withdrawal queue to which the loan belongs. If the loan was issued after the last queue, it belongs entirely to the pool, and _outstandingValues is updated. If not, it updates the queue accounting, queue outstanding values, getTotalReceived and getAvailableToWithdraw.

solidity
function _loanTermination(
    ...
) 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
        );
    }
}

However, the mergeTranches() function is permissionless and only requires the merged tranches to be contiguous. Once tranches are merged, the loanId of the new tranche changes, which can lead to incorrect accounting in the Pool.

Proof of Concept

Consider the following scenario:

A borrower opens a loan and takes liquidity from multiple offers of the same Pool. The loan has the parameters loanId = 100, with two tranches, both having lender = pool_address.
In the Pool, assume getLastLoanId[1][loan] = 100, indicating that queue index 1 points to the latest loanId in the loan contract.
An attacker calls mergeTranches() to merge the two tranches of loanId = 100 with the same lender, which is the pool address. The new newLoanId = 101 is used in the new tranche.
Now, when the loan is repaid, the _loanTermination() function is invoked with _loanId = 101. The loop returns i == totalQueues, making the loan belong entirely to the pool, while it should belong to withdrawal queue index 1.

MultiSourceLoan.sol#L1132-L1140

solidity
tranche[_minTranche] = IMultiSourceLoan.Tranche(
    _newLoanId, // @audit can be used to change loanId
    _loan.tranche[_minTranche].floor,
    principalAmount,
    lender,
    accruedInterest,
    startTime,
    cumAprBps / principalAmount
);