LoanProposalImpl Manual Review Findings

LPI-01M: Inefficient Collateralization of Loan

Type	Severity	Location
Logical Fault		LoanProposalImpl.sol:L222-L224

Description:

A loan defined in LoanProposalImpl will be collateralized by the full amount of collateral required per loan token unit as well as the collateral required for each conversion of each repayment step.

Impact:

While not a security issue, this finding highlights a potential capital inefficiency in the system that should be optimized.

Example:

Copy
219IERC20Metadata(collToken).safeTransferFrom(
220    msg.sender,
221    address(this),
222    _finalCollAmountReservedForDefault +
223        _finalCollAmountReservedForConversions +
224        expectedTransferFee
225);

Recommendation:

This behaviour appears to not be well defined as a user would essentially have to supply double what they are expected to pay in a normal repayment scenario. As repayment schedules progress, the cost of a "default" inherently decreases.

As such, the actual collateral required for a loan should be the maximum between the total amount of collateral required for each repayment period as well as the total amount of collateral required for a "default" (which presumably uses an unfavourable ratio in contrast to the normal repayment procedure).

This will ensure maximum collateral efficiency for borrowers while retaining the same level of security for the liquidity pool.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

After extensive discussions with the Myso Finance team, we have concluded that this is an intended design of the protocol and is meant to permit maximum flexibility and leave the negotiation of capital-efficient terms entirely up to the borrower-lender agreements.

As such, we consider this exhibit nullified given that the current implementation permits both capital efficient and lender-protective loans to be established.

LPI-02M: Discrepant Loan Subscription Validation

Type	Severity	Location
Logical Fault		LoanProposalImpl.sol:L106, L138, L165-L168

Description:

The LoanProposalImpl::proposeLoanTerms function will validate that the _finalLoanAmount is not greater than the maximum permitted by the terms, however, the LoanProposalImpl::acceptLoanTerms and LoanProposalImpl::finalizeLoanTermsAndTransferColl functions will validate the total subscription rather than the final loan amount.

Impact:

Presently, it is possible for a loan whose proposition would fail to have been successfully finalized, indicating a discrepancy in the system that should not be present.

Example:

Copy
98uint256 totalSubscribed = IFundingPool(fundingPool).totalSubscribed(
99    address(this)
100);
101(, , uint256 _finalLoanAmount, , ) = getAbsoluteLoanTerms(
102    newLoanTerms,
103    totalSubscribed,
104    IERC20Metadata(IFundingPool(fundingPool).depositToken()).decimals()
105);
106if (_finalLoanAmount > newLoanTerms.maxLoanAmount) {
107    revert Errors.NewMaxLoanAmountBelowCurrentSubscriptions();
108}

Recommendation:

We advise this discrepancy to be addressed by streamlining the maximum / minimum loan amount validations and either applying them to the final loan amount after LoanProposalImpl::getAbsoluteLoanTerms or to the total subscription of the loan, either of which we consider an adequate resolution to this exhibit.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The minLoanAmount and maxLoanAmount configurational parameters of a loan were replaced by minSubscriptionAmount and maxSubscriptionAmount, indicating that the totalSubscriptions of the loan proposal will be utilized for these calculations and thus eliminating the exhibit's highlighted discrepancy.

LPI-03M: Incorrect Overlap of States

Type	Severity	Location
Logical Fault		LoanProposalImpl.sol:L313-L315, L353

Description:

In the current contract implementation it is possible to exercise a conversion and repay a loan schedule in the same timestamp as there is an overlap of states in the equality case of the referenced conditionals.

Impact:

While this exhibit does not lead to any exploitable scenario, it pinpoints a discrepancy in the system that should be addressed.

Example:

Copy
292function exerciseConversion() external {
293    address fundingPool = staticData.fundingPool;
294    uint256 lenderContribution = IFundingPool(fundingPool)
295        .subscribedBalanceOf(address(this), msg.sender);
296    if (lenderContribution == 0) {
297        revert Errors.InvalidSender();
298    }
299    if (
300        dynamicData.status != DataTypesPeerToPool.LoanStatus.LOAN_DEPLOYED
301    ) {
302        revert Errors.InvalidActionForCurrentStatus();
303    }
304    uint256 repaymentIdx = dynamicData.currentRepaymentIdx;
305    checkCurrRepaymentIdx(repaymentIdx);
306    if (lenderExercisedConversion[msg.sender][repaymentIdx]) {
307        revert Errors.AlreadyConverted();
308    }
309    // must be after when the period of this loan is due, but before borrower can repay
310    if (
311        block.timestamp <
312        _loanTerms.repaymentSchedule[repaymentIdx].dueTimestamp ||
313        block.timestamp >
314        _loanTerms.repaymentSchedule[repaymentIdx].dueTimestamp +
315            staticData.conversionGracePeriod
316    ) {
317        revert Errors.OutsideConversionTimeWindow();
318    }
319    uint256 conversionAmount = (_loanTerms
320        .repaymentSchedule[repaymentIdx]
321        .collTokenDueIfConverted * lenderContribution) /
322        IFundingPool(fundingPool).totalSubscribed(address(this));
323    collTokenConverted[repaymentIdx] += conversionAmount;
324    totalConvertedSubscriptionsPerIdx[repaymentIdx] += lenderContribution;
325    lenderExercisedConversion[msg.sender][repaymentIdx] = true;
326    IERC20Metadata(staticData.collToken).safeTransfer(
327        msg.sender,
328        conversionAmount
329    );
330
331    emit ConversionExercised(msg.sender, repaymentIdx, conversionAmount);
332}
333
334function repay(uint256 expectedTransferFee) external {
335    if (msg.sender != _loanTerms.borrower) {
336        revert Errors.InvalidSender();
337    }
338    if (
339        dynamicData.status != DataTypesPeerToPool.LoanStatus.LOAN_DEPLOYED
340    ) {
341        revert Errors.InvalidActionForCurrentStatus();
342    }
343    uint256 repaymentIdx = dynamicData.currentRepaymentIdx++;
344    checkCurrRepaymentIdx(repaymentIdx);
345    // must be after when the period of this loan when lenders can convert,
346    // but before default period for this period
347    uint256 currConversionCutoffTime = _loanTerms
348        .repaymentSchedule[repaymentIdx]
349        .dueTimestamp + staticData.conversionGracePeriod;
350    uint256 currRepaymentCutoffTime = currConversionCutoffTime +
351        staticData.repaymentGracePeriod;
352    if (
353        (block.timestamp < currConversionCutoffTime) ||
354        (block.timestamp > currRepaymentCutoffTime)
355    ) {
356        revert Errors.OutsideRepaymentTimeWindow();
357    }

Recommendation:

We advise either of the two conditionals to become inclusive, ensuring a consistent loan state across time.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The discrepancy has been alleviated by converting the conditional of LoanProposalImpl::exerciseConversion to be inclusive (i.e. >=) and thus covering the equality case.

LPI-04M: Potentially Insecure Acceptance of Loan Terms

Type	Severity	Location
Logical Fault		LoanProposalImpl.sol:L71-L114, L116

Description:

A borrower will accept a loan proposal by issuing a LoanProposalImpl::acceptLoanTerms transaction without actually passing in an argument that validates they intended to accept the current terms of the loan. As such, it is possible for an on-chain race condition to manifest whereby an arranger will detect a LoanProposalImpl::acceptLoanTerms invocation and issue new, potentially unfavourable terms right before they are accepted.

Impact:

While a protection mechanism is in place in the form of a cooldown period of 1 hours, this period is insufficient in the case of multi-signature wallets w/ potential timelocks which are expected to be the majority of cases interacting with a loan proposal.

Example:

Copy
116function acceptLoanTerms() external {

Recommendation:

We advise a hash-based mechanism to be set in place whereby LoanProposalImpl::acceptLoanTerms accepts a new bytes32 argument representing the hash of the current _loanTerms, permitting the borrower to validate the exact terms they are willing to accept.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The Myso Finance team opted to instead utilize the last loan term update time that already exists in the contract as a control variable, introducing it as an argument to the LoanProposalImpl::acceptLoanTerms function and mandating that it is equal to the contract's stored timestamp.

As the loan terms being accepted to are validated in the LoanProposalImpl::acceptLoanTerms function, we consider this exhibit fully alleviated.

LPI-05M: Incorrect Rollback Procedure

Type	Severity	Location
Logical Fault		LoanProposalImpl.sol:L244-L249

Description:

The LoanProposalImpl::rollback function is meant to permit the pool to revert its decision, refunding the borrower's collateral and permitting the pool to withdraw their funds.

The function is incorrectly available solely in a BORROWER_ACCEPTED state rather than a READY_TO_EXECUTE state despite its code refunding existing collateral which will only happen in a READY_TO_EXECUTE state.

Impact:

As the implementation's conditionals evaluate a LOAN_EXECUTION_GRACE_PERIOD, it is evident that the function is expected to be invoke-able in a READY_TO_EXECUTE state which is not the case. As such, we consider this exhibit to be of "medium" severity.

Example:

Copy
242function rollback() external {
243    // cannot be called anymore once lockInFinalAmountsAndProvideCollateral() called
244    if (
245        dynamicData.status !=
246        DataTypesPeerToPool.LoanStatus.BORROWER_ACCEPTED
247    ) {
248        revert Errors.InvalidActionForCurrentStatus();
249    }
250    uint256 totalSubscribed = IFundingPool(staticData.fundingPool)
251        .totalSubscribed(address(this));
252    uint256 _lenderInOrOutCutoffTime = lenderInOrOutCutoffTime();
253    if (
254        (msg.sender == _loanTerms.borrower &&
255            block.timestamp < _lenderInOrOutCutoffTime) ||
256        (block.timestamp >= _lenderInOrOutCutoffTime &&
257            totalSubscribed < _loanTerms.minLoanAmount) ||
258        (block.timestamp >=
259            _lenderInOrOutCutoffTime +
260                Constants.LOAN_EXECUTION_GRACE_PERIOD)
261    ) {
262        dynamicData.status = DataTypesPeerToPool.LoanStatus.ROLLBACK;
263        address collToken = staticData.collToken;
264        // transfer any previously provided collToken back to borrower
265        IERC20Metadata(collToken).safeTransfer(
266            _loanTerms.borrower,
267            IERC20Metadata(collToken).balanceOf(address(this))
268        );
269    } else {
270        revert Errors.InvalidRollBackRequest();
271    }
272
273    emit Rolledback();
274}

Recommendation:

We advise the code to permit a rollback in either a BORROWER_ACCEPTED state or a READY_TO_EXECUTE state, ensuring that the liquidity pool can adequately revert its decision in a timely manner.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The Myso Finance team has stated that the LoanProposalImpl::rollback function is solely possible during a BORROWER_ACCEPTED state and that the transfer of collateral included within it was meant to accommodate for any accidentally-sent tokens.

After internal discussions, the Myso Finance team has decided to remove the collateral transfer altogether to prevent ambiguities in the function's intended purpose. In any case, we consider this exhibit nullified as the original behaviour of the code was intended by the Myso Finance team.

LPI-06M: Insufficient Validation of Repayment Schedule

Type	Severity	Location
Input Sanitization		LoanProposalImpl.sol:L587-L619

Description:

The LoanProposalImpl::repaymentScheduleCheck function will insufficiently sanitize the repayment schedule as the sum of the loanTokenDue values needs to be exactly equal to Constants.BASE.

Impact:

An incorrect repayment schedule can cause a repayment to be performed of an amount that exceeds the actual loan given that the contract retains a loan's repayment status as an index of the repayment period rather than as a sum of the total repaid amount.

Example:

Copy
587function repaymentScheduleCheck(
588    DataTypesPeerToPool.Repayment[] calldata repaymentSchedule
589) internal view {
590    if (repaymentSchedule.length == 0) {
591        revert Errors.EmptyRepaymentSchedule();
592    }
593    if (
594        repaymentSchedule[0].dueTimestamp <
595        block.timestamp + Constants.MIN_TIME_UNTIL_FIRST_DUE_DATE
596    ) {
597        revert Errors.FirstDueDateTooClose();
598    }
599    uint256 prevDueDate;
600    uint256 currDueDate;
601    for (uint i = 0; i < repaymentSchedule.length; ) {
602        if (
603            repaymentSchedule[i].loanTokenDue == 0 ||
604            repaymentSchedule[i].collTokenDueIfConverted == 0
605        ) {
606            revert Errors.RepaymentOrConversionAmountIsZero();
607        }
608        currDueDate = repaymentSchedule[i].dueTimestamp;
609        if (
610            currDueDate < prevDueDate + Constants.MIN_TIME_BETWEEN_DUE_DATES
611        ) {
612            revert Errors.InvalidDueDates();
613        }
614        prevDueDate = currDueDate;
615        unchecked {
616            i++;
617        }
618    }
619}

Recommendation:

We advise the loop to properly sum the loanTokenDue members of each repayment schedule and to validate that the sum is equal to Constants.BASE as the end using an if-revert pattern akin to other validations.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The Myso Finance team evaluated this exhibit and has stated that it is intended design for the sum of loanTokenDue values to be arbitrary as different loan arrangements can account for different use cases (i.e. interest on top of the loan or discount due to the prospect of conversions ATM / ITM).

As such, we consider this exhibit nullified given that it represents desirable behaviour by the Myso Finance team.