c4-bot-7 commented 5 months ago

Lines of code

Vulnerability details

C4 Issue

H-10: The attackers front-running repayloans so that the debt cannot be repaid

Comments

Original Vulnerabilities/impacts: The report mainly identifies several attack flows that might allow a malicious lender to DOS a user repayLoan for the lender to forcefully gain the collateral NFT from the borrower.

The attack is dependent on (1) a new loanId created for the refinance or addTranche or MergeTranches flow; (2) the front-run attack has to be performed multiple times to cause borrower’s liquidation; Or the attack is performed when the borrower repayLoan at the last minute; (3) The collateral NFT is valuable for the attack to be profitable for the lender.

Mitigation

Fix: https://github.com/pixeldaogg/florida-contracts/pull/379/files Combined with fix for H-12

    function addNewTranche(
        RenegotiationOffer calldata _renegotiationOffer,
        Loan memory _loan,
        bytes calldata _renegotiationOfferSignature
    ) external nonReentrant returns (uint256, Loan memory) {
        if (msg.sender != _loan.borrower) {
            revert InvalidCallerError();
        }
...

(1) addNewTranche() flow:

The mitigation is combined with the fix for addNewTranche() access control (H-12) which only allows the borrower to call. This eliminates the attack vector from the attacker calling addNewTranche().

Note: similarly refinanceFromExecutionData() also requires the borrower’s authorization and is thus not vulnerable.

(2) refinanceFull and refinancePartial() flow:

In addition, the sponsor mentioned in comments that the lock feature(_isLoanLocked()) limits the lenders ability to front-run multiple times. _isLoanLocked() is used in refinanceFull() and refinancePartial() ‘s lender initiated flow. This feature will prevent the malicious lender from front-running and calling refinanceFull() and refinanceParital() frequently. This will most likely allow the borrower to repayLoan() successfully when locked.

(3) mergeTranches() flow:

This flow can only be called by the lender if the lender has tranches to merge and can only be called once or limited times, which most likely allows the borrower to repayLoan() at a second try.

Based on the above, I consider the impact is mitigated. However, it should be noted that an edge case when the borrower repays at the last minute might still allow the attack through refinance or mergeTranches() flow to succeed.

But in this case, it can also be assumed that the borrower inherently accepts the risk of delayed repayLoan settlements when repayLoan only at the very last minute (_loan.startTime + _loan.duration).

Conclusion

LGTM

c4-judge commented 5 months ago

alex-ppg marked the issue as satisfactory