code-423n4 / 2024-04-gondi-findings

0 stars 0 forks source link

Possible overflow when borrower accepts renegotiation offer in `refinanceFull()` #56

Closed c4-bot-8 closed 7 months ago

c4-bot-8 commented 7 months ago

Lines of code

https://github.com/code-423n4/2024-04-gondi/blob/b9863d73c08fcdd2337dc80a8b5e0917e18b036c/src/lib/loans/MultiSourceLoan.sol#L195

Vulnerability details

Impact

In the refinanceFull() function, if the borrower is the caller, it signifies a renegotiation offer. The borrower will clear the accrued interest, permitting them to repay the accrued interest without waiting for the full loan repayment.

} else {
    /// @notice Borrowers clears interest
    _checkSignature(_renegotiationOffer.lender, _renegotiationOffer.hash(), _renegotiationOfferSignature);
    netNewLender -= totalAccruedInterest; // @audit possible overflow when netNewLender < totalAccruedInterest
    totalAccruedInterest = 0;
}

However, if netNewLender < totalAccruedInterest, there's a risk of overflow. Consequently, refinanceFull() will revert, preventing the borrower from accepting the renegotiation offer.

Proof of Concept

Consider the following scenario:

  1. Alice (the borrower) takes a 10 ETH loan, and the accrued interest is already 2.1 ETH.
  2. Alice wants to reduce the principal amount and is interested in a renegotiation offer from Bob for a 2 ETH principal amount loan.
  3. When Alice calls refinanceFull() and passes in Bob's renegotiation offer, it reverts because:
    netNewLender = principalAmount - fee = 2 eth (assume fee = 0)
    totalAccruedInterest = 2.1 eth
    netNewLender -= totalAccruedInterest (revert)

Tools Used

Manual Review

Recommended Mitigation Steps

Consider deducting only min(netNewLender, totalAccruedInterest) from netNewLender.

Assessed type

Under/Overflow

c4-judge commented 7 months ago

0xA5DF marked the issue as primary issue

0xend commented 7 months ago

Given the nature of laons, renegotiating a loan to a principal lower than interest accrues seems highly unlikely but would consider this.

0xend commented 7 months ago

I think this is a low btw

C4-Staff commented 7 months ago

@0xend Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged.

c4-judge commented 7 months ago

0xA5DF changed the severity to QA (Quality Assurance)

0xA5DF commented 7 months ago

Given the nature of laons, renegotiating a loan to a principal lower than interest accrues seems highly unlikely but would consider this.

Marking as low due to this. If the warden can prove that this scenario is at least somewhat likely to happen I'd consider reinstating med severity.

c4-judge commented 7 months ago

0xA5DF marked the issue as grade-c