LendingTerm._partialRepay function can be called to frontrun LendingTerm._call function call after maximum delay between partial repayments is reached for corresponding loan

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L237-L253
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L634-L675
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L490-L559

Vulnerability details

Impact

For a loan that requires partial repayments, the LendingTerm.partialRepayDelayPassed function would return true if the maximum delay between partial repayments has been reached.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L237-L253

    function partialRepayDelayPassed(
        bytes32 loanId
    ) public view returns (bool) {
        // if no periodic partial repays are expected, always return false
        if (params.maxDelayBetweenPartialRepay == 0) return false;

        ...

        // return true if delay is passed
        return
            lastPartialRepay[loanId] <
            block.timestamp - params.maxDelayBetweenPartialRepay;
    }

When the LendingTerm.partialRepayDelayPassed function returns true for the corresponding loan, a user can call the LendingTerm._call function to call such loan.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L634-L675

    function _call(
        address caller,
        bytes32 loanId,
        address _auctionHouse
    ) internal {
        Loan storage loan = loans[loanId];

        ...
        
        // check that the loan can be called
        require(
            GuildToken(refs.guildToken).isDeprecatedGauge(address(this)) ||
                partialRepayDelayPassed(loanId),
            "LendingTerm: cannot call"
        );

        ...
    }

However, the borrower of such loan can frontrun the LendingTerm._call function call by calling the LendingTerm._partialRepay function to make a partial repayment for such loan even after the maximum delay between partial repayments is reached. This can be done because the LendingTerm._partialRepay function does not check if the maximum delay between partial repayments has been reached or not for the corresponding loan. After the LendingTerm._partialRepay function call, lastPartialRepay[loanId] is set to block.timestamp for such loan, which causes the LendingTerm._call function call to revert because partialRepayDelayPassed(loanId) returns false for such loan after the frontrunning. Users, who should be able to call such loan and auction such loan's collateral, become unable to do so.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/e3d3e581e0e51a9ecf3a5a0c4e4bd4af32552fc0/src/loan/LendingTerm.sol#L490-L559

    function _partialRepay(
        address repayer,
        bytes32 loanId,
        uint256 debtToRepay
    ) internal {
        Loan storage loan = loans[loanId];

        ...

        // update loan in state
        loans[loanId].borrowAmount -= issuanceDecrease;
        lastPartialRepay[loanId] = block.timestamp;
        issuance -= issuanceDecrease;

        ...
    }

Proof of Concept

The following steps can occur.

1. Alice's loan requires partial repayments, and the maximum delay between partial repayments has been reached for such loan.
2. Bob calls Alice's loan by calling the LendingTerm._call function.
3. Alice frontruns Bob's LendingTerm._call function by calling the LendingTerm._partialRepay function to make a partial repayment for her loan.
4. After the frontrunning, lastPartialRepay[loanId] is set to block.timestamp for Alice's loan, which causes partialRepayDelayPassed(loanId) to return false when executing Bob's LendingTerm._call function call.
5. As a result, Bob's LendingTerm._call function call reverts, and he is not able to call Alice's loan when he should be allowed to.

Tools Used

Manual Review

Recommended Mitigation Steps

The LendingTerm._partialRepay function can be updated to revert if the maximum delay between partial repayments has been reached for the corresponding loan.

Assessed type

DoS

[0xSorryNotSorry]

This could form even with the TX order, subject to MEV.

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[Trumpero]

Non-issue. It's correct behavior for borrower to use partialRepay before liquidation

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[rbserver]

Hi @Trumpero,

According to the Mechanism Detail section of https://code4rena.com/audits/2023-12-ethereum-credit-guild, maxDelayBetweenPartialRepay exists to support loans requiring periodic payments if desired and if a partial repayment is missed (delay has passed), the loan can be called. For a loan that requires periodic payments, when the maximum delay between partial repayments has been passed, the borrower has clearly violated such loan's periodic payment requirement, and such loan can be called based on the specification described in the Mechanism Detail section. However, due to the frontrunning issue described in this finding, such borrower can force such loan to be failed to be called even though she or he has violated the periodic payment requirement already.

For example, suppose 24 hours have been passed after a loan's maximum delay between partial repayments has been reached, the borrower has violated such loan's periodic payment requirement for 24 hours already, and such loan should be allowed to be called according to the Mechanism Detail section's specs; yet, such loan cannot be called if such frontrunning occurs even though the borrower has violated such loan's periodic payment requirement for 24 hours already. This scenario is unfair to the users, who should be allowed to call such loan and auction such loan's collateral but become unable to do so.

Because the frontrunning issue described in this finding goes against the protocol's specs and intention, is unfair to the users who should be allowed to call such loan, and unfairly favors the borrower who has violated such loan's periodic payment requirement, such issue would possess a medium risk. Hence, would this finding be reconsidered as a medium risk finding? Thanks for your work!

[Trumpero]

@rbserver "I still don't see any issue here. The delay of partial repayment counts from the last repayment to the time of liquidation, so it doesn't conflict with the protocol's specifications and intentions. Liquidation isn't forced but encouraged, and repayments with longer delays are allowed if liquidation isn't immediate. The sponsor also mentions that what matters for the protocol is that the loan is repaid/closed.