Merging tranches could make _loanTermination() accounting incorrect

[c4-bot-3]

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.

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:

1. 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.
2. In the Pool, assume getLastLoanId[1][loan] = 100, indicating that queue index 1 points to the latest loanId in the loan contract.
3. 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.
4. 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

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

Tools Used

Manual Review

Recommended Mitigation Steps

Limit the ability to call mergeTranches() directly to lenders only.

Assessed type

Other

[c4-judge]

0xA5DF marked the issue as primary issue

[c4-judge]

0xA5DF marked the issue as selected for report

[c4-judge]

0xA5DF marked the issue as satisfactory

[0xend]

https://github.com/pixeldaogg/florida-contracts/pull/360