0x3b - FlashRolloverLoan_G5 can repay more than it's needed

[sherlock-admin3]

0x3b

high

FlashRolloverLoan_G5 can repay more than it's needed

Summary

executeOperation refunds the borrower all the unspent funds. However, if the flash loan (FL) was larger than the needed funds, the excess amount is transferred to the borrower. This can lead to either the transaction reverting if we are unable to repay the FL, or the borrower receiving extra funds and the FL being repaid from the contract's balances.

Vulnerability Detail

Users can choose their desired _flashLoanAmount when using rolloverLoanWithFlash. This, in turn, causes the loan provider to send us the funds and execute executeOperation. After we have repaid the FL, the amount plus fees are approved for the FL provider to take, and the remaining funds are sent to the borrower:

uint256 fundsRemaining = acceptCommitmentAmount +
    _rolloverArgs.borrowerAmount -
    repaymentAmount -
    _flashFees;

However, the current calculation does not account for the possibility that we can borrow far more than needed to repay. _repayLoan will pass without issue, as it has a check to see if we are sending more funds than necessary:

if (paymentAmount >= _owedAmount) {
    paymentAmount = _owedAmount;

The extra funds will remain inside the FlashRolloverLoan_G5 contract, where will be accounted for when calculating fundsRemaining and transferred to the user. This will either:

• drain the contract - if there are funds inside they will go towards paying the flash loan.
• revert the TX - if the contract has 0 balance, then we need FL amount == repay amount or else the TX will always revert.

Achieving an FL amount == repaymentAmount is challenging as the interest changes every block. Moreover, we cannot do FL amount < repaymentAmount because it will revert, even if we supply enough borrowerAmount, which is another issue.

Impact

Users can extract funds from the FlashRolloverLoan_G5 or cause any transaction with an FL amount greater than the repaymentAmount to revert.

Code Snippet

uint256 fundsRemaining = acceptCommitmentAmount +
    _rolloverArgs.borrowerAmount -
    repaymentAmount -
    _flashFees;

Tool used

Manual Review

Recommendation

Ensure the calculations take into account how large the FL is. A simpler option would be to limit the flash loan amount to be equal to the repayment amount.

Duplicate of #257

[ethereumdegen]

I think i remember talking about this when engineering this dust-repayment and recalling that we agreed that it is OK for it to be able to 'drain funds' from this contract as there are not supposed to ever be any funds in the contract anyways.

Also as far as the revert, that is a non issue as we expect the user to be smart enough to put in a high enough flash loan amount or it will revert. That is OK.

However it could be 'better' slightly to limit the flash loan amount to be equal to the repayment amt so i will investigate this but this is probably a minor rather than a medium if anything at all . thank you for the input !!

[0x3b33]

Hello @ethereumdegen,

Thank you for your response. However, I believe there has been a slight misunderstanding regarding the revert.

You mentioned:

As far as the revert is concerned, that is a non-issue as we expect the user to input a sufficiently high flash loan amount; otherwise, it will revert. That is acceptable.

However, the main issue is about the fact that we need the FL amount to be exact == or else the TX will revert. This will happen because fundsRemaining does not take into account the size of the FL, but just how much we have repaid.

        uint256 fundsRemaining = acceptCommitmentAmount +_rolloverArgs.borrowerAmount - repaymentAmount - _flashFees;

This portion of the code transfers any excess funds to the borrower. Therefore, if the FL amount exceeds the necessary amount, the excess will be transferred to the user, causing the transaction to revert because the contract will not have enough funds to repay the FL to AAVE.

Example:

Prerequisites	Values
Repayment amount	1000 USDC
Accept commitment amount	2000 USDC
FL Amount	1200 USDC
FL Fees	1 USDC
Borrower amount	0 USDC

In this scenario, the fundsRemaining calculation would be:
acceptCommitmentAmount + borrowerAmount - repaymentAmount - flashFees = 2000 + 0 - 1000 - 1 = 999 USDC

However, incorporating the correct FL amount into this calculation changes it to:
acceptCommitmentAmount + borrowerAmount - flashAmount - flashFees = 2000 + 0 - 1200 - 1 = 799 USDC

We would then transfer to the borrower 999 USDC and leave 1001USDC inside the contract, but our FL requires 1201 USDC.

The discrepancy of 200 USDC, if not correctly accounted for, would be transferred to the user, leaving the FlashRolloverLoan_G5 contract with only 1001 USDC — insufficient to cover the repayment of the 1201 USDC flash loan.

This should be considered at least a medium, as FlashRolloverLoan_G5 will revert most of the time.