Closed sherlock-admin2 closed 2 months ago
i believe this is invalid because
1) _flashAmount is an ephemeral number, it is added and then it is taken back out so it is NOT part of funds remaining 2) the value _borrowerAmount is what makes up the difference -- it is the small amount the borrower has to put in to pay the flah fees and other fees
We know rollover definitely works . Wont fix .
here are successful fx of it on mainnet
https://etherscan.io/address/0xf236d5Cc4d45eA0eF223Bfdf9583e655f51C12fB
Hey, I believe this was wrongly closed.
This is the same issue as #45 . This comment here shows an example where this scenario could occur and details how the issue could arise.
here are successful fx of it on mainnet https://etherscan.io/address/0xf236d5Cc4d45eA0eF223Bfdf9583e655f51C12fB
As we can see, there are several transactions failing in that contract, which are probably failing due to this issue.
@0xadrii I recognize that, so I believe both this issue and #45 is invalid. The user not understanding how to use the roll-over functionality wouldn't constitute a valid issue to me. There is no difference in the V4 and V5 versions for this functionality. Also could you dm me for further enquiries since I didn't request for input here?
Edit:
Here is the V4 version of the contract (see FlashRolloverLoan_G4.sol
), where the computations are identical and consistent with sponsor comments:
function executeOperation(
address _flashToken,
uint256 _flashAmount,
uint256 _flashFees,
address _initiator,
bytes calldata _data
) external virtual onlyFlashLoanPool returns (bool) {
require(
_initiator == address(this),
"This contract must be the initiator"
);
RolloverCallbackArgs memory _rolloverArgs = abi.decode(
_data,
(RolloverCallbackArgs)
);
uint256 repaymentAmount = _repayLoanFull(
_rolloverArgs.loanId,
_flashToken,
_flashAmount
);
AcceptCommitmentArgs memory acceptCommitmentArgs = abi.decode(
_rolloverArgs.acceptCommitmentArgs,
(AcceptCommitmentArgs)
);
// Accept commitment and receive funds to this contract
(uint256 newLoanId, uint256 acceptCommitmentAmount) = _acceptCommitment(
_rolloverArgs.lenderCommitmentForwarder,
_rolloverArgs.borrower,
_flashToken,
acceptCommitmentArgs
);
//approve the repayment for the flash loan
IERC20Upgradeable(_flashToken).approve(
address(POOL()),
_flashAmount + _flashFees
);
uint256 fundsRemaining = acceptCommitmentAmount +
_rolloverArgs.borrowerAmount -
repaymentAmount -
_flashFees;
if (fundsRemaining > 0) {
IERC20Upgradeable(_flashToken).transfer(
_rolloverArgs.borrower,
fundsRemaining
);
}
emit RolloverLoanComplete(
_rolloverArgs.borrower,
_rolloverArgs.loanId,
newLoanId,
fundsRemaining
);
return true;
}
0xadrii
medium
Substracting repaymentAmount instead of _flashAmount when computing the funds remaining in executeOperation will make the call fail for payments where the flash borrowed amount was higher than the loan repayment amount
Summary
Substracting repaymentAmount instead of _flashAmount when computing the fundsRemaining in the executeOperation function from FlashRolloverLoan_G5 will make the function always fail because more assets than the intended will be transferred to the borrower.
Vulnerability Detail
The
FlashRolloverLoan_G5
contract allows users to rollover their existing loans using a flash loan from AAVE. In order to do so, therolloverLoanWithFlash
function is called. This will trigger a flasloan that will execute theexecuteOperation
callback and transfer the flash loan funds toFlashRolloverLoan_G5
:The
executeOperation
will perform the following steps:_repayLoanFull
function will be called. This will repay the specified loan in full using the_flashAmount
assets, which are the assets requested to AAVE’s flash loan. It is important to note that the requested_flashAmount
can differ from the actual amount repaid in the loan, which is returned and stored in therepaymentAmount
variable, meaning that therepaymentAmount
could be smaller than_flashAmount
._acceptCommitment
function will be called. This function will create a new loan, andacceptCommitmentAmount
of collateral will be transferred to theFlashRolloverLoan_G5
contract.fundsRemaining
in the contract.fundsRemaining
is the amount that can be transferred to the borrower after repaying the flash loan.The problem is that the
fundsRemaining
are computed by substracting therepaymentAmount
, instead of the_flashAmount
.As mentioned in step 1, the
_flashAmount
requested by the user when performing the flash loan can be greater than the actual amount repaid for the loan. However, thefundsRemaining
computation does not consider this situation, and wrongly substractsrepaymentAmount
instead of_flashAmount
, assuming that the difference between_flashAmount
andrepaymentAmount
are assets that belong to the borrower, when in reality belong to the repayment of the flash loan (note how the prior approval to the pool approves_flashAmount + _flashFees
instead ofrepaymentAmount + _flashFees
).This will make the
executeOperation
to always fail when the difference between_flashAmount
andrepaymentAmount
is greater than zero, because AAVE’s pool will try to pull_flashAmount + _flashFees
fromFlashRolloverLoan_G5
, but only_flashAmount - repaymentAmount + _flashFees
will remain in the contract.Impact
Medium. This is a likely scenario where the
executeOperation
will always fail if the amount requested by the user in the flash loan is greater than the amount needed to fully repay the loan.Code Snippet
https://github.com/sherlock-audit/2024-04-teller-finance/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/FlashRolloverLoan_G5.sol#L201
Tool used
Manual Review
Recommendation
Substract
_flashAmount
instead ofrepaymentAmount
when computing thefundsRemaining
: