Repayers using EOA accounts can be affected if baddebt is generated when they are repaying loans

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/LendingTerm.sol#L580-L594

Vulnerability details

Impact

• Repayers using EOA accounts will need to spend more PeggedTokens than what they should to get the needed CreditTokens to repay a loan if the market accrues bad debt while the repayer is doing the repayment.

Proof of Concept

• When repaying loans, the LendingTerm::_repay() function computes the total loanDebt to be repaid (includes interests), it pulls the CreditTokens from the repayer the computed value of the loanDebt, then distributes interests, burns the principal, updates the issuance and finally transfers back the borrower's collateral to the borrower.
  • The problem is that this existing implementation forces the repayer to mint in advance the amount of loanDebt to be repaid, the thing is that EOA accounts can't do the minting of the required CreditTokens and also repay the loan in the same transaction. EOA accounts will first mint the CreditTokens and then will send a separate tx to repay the loan. The problem is that if bad debt is generated in the system and the creditMultiplier is decremented between the time when tx of the repayer to mint the CreditTokens and when the tx to repay the loan is actually executed, the repayer will be impacted by the bad debt, because the amount of minted CreditTokens won't be enough to cover the new value that will be computed for the loanDebt (since the creditMultiplier shrank, more CreditTokens will be required to cover the same debt). This will cause the tx to repay the loan to revert, since the repayer won't have enough CreditTokens in its balance, but more importantly, now, the repayer will be forced to mint more CreditTokens, which means, the repayer will need to spend more PeggedTokens to mint the extra CreditTokens that are required to cover the new value of the loanDebt, that means, the repayer was impacted by the bad debt that was generated in the system. The design of the protocol is such that bad debt is covered by stakers, surpluss buffer, and CreditToken holders, but, it is important to make a distinction between a holder who is holding the CreditTokens expecting a return on his investments, and a repayer who was forced to mint CreditTokens before repaying his loan, repayers are already paying interests and fees, it is not fair to impact them if bad debt is generated in the system while they are repaying their loans.

Let's run some numbers to demonstrate the impact of this current implementation.

• Assume creditMultiplier is 1e18 (has not been updated), UserA borrowed 100 CreditTokens and the current value of the loanDebt to repay the loan is 120 CreditTokens to cover the interests and fees. UserA goes to the PSM module and deposits 120 PeggedTokens in exchange for 120 CreditTokens. The user now goes ahead to call the repay() to repay his loan. Before the UserA tx to repay the loan is executed, some loans accrue bad debt in the system and cause the creditMultiplier to shrink to 0.9e18. Now, when the user tx is finally executed, the new value of the loanDebt will be ~133 CreditTokens, this will cause the tx to revert because UserA only minted the required amount of loanDebt which was 120 CreditTokens. Now, the user will need to go again to the PSM module to mint the extra CreditTokens and will need to spend more collateral to mint those extra CreditTokens (~14 more PeggedToken).
  • As a result of the current implementation, UserA was forced to spend more PeggedTokens to repay his debt, if we assume CreditTokens are gUSDC and PeggedTokens are USDC, now, to repay the loan, the UserA was forced to spend ~134 USDC instead of 120USDC that is the real value of the debt to be repaid.

LendingTerm.sol

//@audit-issue => A repayer could compute how much CreditTokens are required to repay a loan by calling this function, the computed value will be based on the current value of the creditMultiplier
//@audit-issue => The repayer would then go and mint the amount returned by this function before calling the `repay()` to finally repay his loan

/// @notice outstanding borrowed amount of a loan, including interests
function getLoanDebt(bytes32 loanId) public view returns (uint256) {
    ...

    // compute interest owed
    uint256 borrowAmount = loan.borrowAmount;
    uint256 interest = (borrowAmount *
        params.interestRate *
        (block.timestamp - borrowTime)) /
        YEAR /
        1e18;
    uint256 loanDebt = borrowAmount + interest;
    uint256 _openingFee = params.openingFee;
    if (_openingFee != 0) {
        loanDebt += (borrowAmount * _openingFee) / 1e18;
    }
    uint256 creditMultiplier = ProfitManager(refs.profitManager)
        .creditMultiplier();
    
    //@audit-info => The loanDebt is normalized using the current value of the `creditMultiplier`. loanDebt includes interests and fees accrued by the original borrowAmount
    loanDebt = (loanDebt * loan.borrowCreditMultiplier) / creditMultiplier;

    return loanDebt;
}


//@audit-issue => The problem when repaying the loan is if bad debt was generated in the system, now, the value of the `creditMultiplier` will be slightly lower than when the user queried the total amount of CreditTokens to be repaid by calling the `getLoanDebt()`

function _repay(address repayer, bytes32 loanId) internal {
    ...
    ...
    ...

    // compute interest owed
    //@audit-issue => Now, when repaying the loan, the creditMultiplier will be different, thus, the computed value of the loanDebt will be greater than before, thus, more CreditTokens will be required to repay the same loan
    uint256 loanDebt = getLoanDebt(loanId);
    uint256 borrowAmount = loan.borrowAmount;
    uint256 creditMultiplier = ProfitManager(refs.profitManager)
        .creditMultiplier();
    uint256 principal = (borrowAmount * loan.borrowCreditMultiplier) /
        creditMultiplier;
    uint256 interest = loanDebt - principal;

    //@audit-issue => The amount of `loanDebt` CreditTokens are pulled from the repayer, this means, the repayer must have already minted the CreditTokens and it also granted enough allowance to the LendingTerm contract to spend on his behalf!
    /// pull debt from the borrower and replenish the buffer of available debt that can be minted.
    CreditToken(refs.creditToken).transferFrom(
        repayer,
        address(this),
        loanDebt
    );

    ...
    ...
    ...
}

Tools Used

Manual Audit

Recommended Mitigation Steps

• The most straightforward solution to mitigate this problem is to allow the repayers to mint the exact required amount of CreditTokens to repay their loans within the same tx when the loan is actually being repaid. It can be added as an option for any repayer who wants to go that route and protect themselves against bad debt generated in the system while they are repaying their loans, those who don't want to mint the exact amount of CreditTokens that are required to repay the loan can follow the current implementation where the CreditTokens will be pulled from their balance. If enabled, the LendingTerm based on the computed loanDebt will pull the exact amount of PeggedTokens that are required to mint the exact amount of CreditTokens to repay the loan, and by using the PSM module, the LendingTerm will mint the exact required amount of CreditTokens and will transfer the PeggedTokens that were pulled from the repayer.

LendingTerm.sol

+ function _repay(address repayer, bytes32 loanId, bool mintCreditTokens) internal {
    ...

    // compute interest owed
    uint256 loanDebt = getLoanDebt(loanId);
    ...    

-   /// pull debt from the borrower and replenish the buffer of available debt that can be minted.
-   CreditToken(refs.creditToken).transferFrom(
-       repayer,
-       address(this),
-       loanDebt
-   );

+   if(mintCreditTokens) {
+     //@audit-info => Computes the exact amount of peggedTokens that are required to repay the debt
+     uint256 peggedTokensToRepay = SimplePSM(refs.psmModule).getRedeemAmountOut(loanDebt);
+     address pegToken = SimplePSM(refs.psmModule).pegToken();

+     //@audit-info => Pull the peggedTokens to repay the debt from the repayer into the LendingTerm
+     ERC20(pegToken).safeTransferFrom(repayer, address(this), peggedTokensToRepay);

+     //@audit-info => Mint the exact CreditTokens to repay the debt
+     //@audit-info => The PSM module will pull the PeggedTokens from the LendingTerm and will mint the CreditTokens to the LendingTerm contract
+     uint256 repaidCreditTokens = SimplePSM(refs.psmModule).mint(peggedTokensToRepay);

+     assert(repaidCreditTokens == loanDebt)
+   } else {
+      /// Pull debt from the borrower and replenish the buffer of available debt that can be minted.
+      CreditToken(refs.creditToken).transferFrom(
+          repayer,
+          address(this),
+          loanDebt
+      );
+   }

    if (interest != 0) {
        // forward profit portion to the ProfitManager
        CreditToken(refs.creditToken).transfer(
            refs.profitManager,
            interest
        );

        // report profit
        ProfitManager(refs.profitManager).notifyPnL(
            address(this),
            int256(interest)
        );
    }

    // burn loan principal
    CreditToken(refs.creditToken).burn(principal);
    RateLimitedMinter(refs.creditMinter).replenishBuffer(principal);

    // close the loan
    loan.closeTime = block.timestamp;
    issuance -= borrowAmount;

    // return the collateral to the borrower
    IERC20(params.collateralToken).safeTransfer(
        loan.borrower,
        loan.collateralAmount
    );

    // emit event
    emit LoanClose(block.timestamp, loanId, LoanCloseType.Repay, loanDebt);
}

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

This is interesting. I'm not sure whether to describe this as a vulnerability, since it is already in our plan to have a "smart account" or "router" tool for borrowers to atomically unwind their position/repay the exact correct amount according to the credit multiplier, that allows to multicall on term.borrow+psm.redeem and psm.mint+term.repay for instance. I can see how this feature could be made part of the base lending term, rather than something on top. Seems like a thoughtful consideration so I don't mind confirming

[c4-sponsor]

eswak (sponsor) confirmed

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as selected for report

[0xbtk]

Hey @Trumpero, i don't see how this is an issue. It's akin to saying that if token prices drop, repayments might increase slightly. The impact is limited and falls within the realm of crypto. Consider a lending protocol where users deposit ETH as collateral for a volatile asset loan. If, during partial repayment, the token's price drops between the minting and the actual loan repayment transaction, it's a crypto fluctuation, not a bug.

Thanks for your time!

[stalinMacias]

Hey @0xbtk The thing is that borrowers borrowed CreditTokens which their value is tight to the creditMultiplier, it is not tied to a specific market price. If a borrower borrowed a specific amount of CreditTokens it should repay the same amount of borrowed tokens + interests. In this case, if the creditMultiplier drops, this will force the borrower to buy more creditTokens than what they should really bought, thus, the total amount of repaid debt will be bigger than what it really should. As I explained in the report, the existing implementation can cause users to end up paying more value for what they borrowed.

Important to emphazise that the drop of the creditMultiplier has nothing to do with the collateral that was used to back up the loan, that is a different thing. creditMultiplier only matters for the CreditToken and the PeggedToken.