0xadrii - `rolloverLoanWithFlash` does not allow adding collateral when accepting commitments

[sherlock-admin4]

0xadrii

medium

rolloverLoanWithFlash does not allow adding collateral when accepting commitments

Summary

The rolloverLoanWithFlash never transfers collateral from the borrower in order to initiate a new loan. This makes it impossible to use rolloverLoanWithFlash to accept commitments where collateral must be supplied.

Vulnerability Detail

The rolloverLoanWithFlash performs two main important steps:

1. Repaying an existing loan
2. Accepting a commitment, which will submit a new bid request and open a loan with the commitment contract.

The second step will execute the following actions (for the sake of the example, let’s focus on the specific situation where _commitmentArgs.smartCommitmentAddress != address(0)):

1. The _acceptCommitment internal function will be called. This call will forward the call to the commitment forwarder contract, which is the contract that will actually interact with the smart commitment to open a new loan:

   // FlashRolloverLoan_G5.sol
   function _acceptCommitment(
           address lenderCommitmentForwarder,
           address borrower,
           address principalToken,
           AcceptCommitmentArgs memory _commitmentArgs
       )
           internal
           virtual
           returns (uint256 bidId_, uint256 acceptCommitmentAmount_)
       {
           uint256 fundsBeforeAcceptCommitment = IERC20Upgradeable(principalToken)
               .balanceOf(address(this));
   
           if (_commitmentArgs.smartCommitmentAddress != address(0)) {
   
                bytes memory responseData = address(lenderCommitmentForwarder)
                       .functionCall(
                           abi.encodePacked(
                               abi.encodeWithSelector(
                                   ISmartCommitmentForwarder
                                       .acceptSmartCommitmentWithRecipient 
                                       .selector, 
                                   _commitmentArgs.smartCommitmentAddress,
                                   _commitmentArgs.principalAmount,
                                   _commitmentArgs.collateralAmount,
                                   _commitmentArgs.collateralTokenId,
                                   _commitmentArgs.collateralTokenAddress,
                                   address(this),
                                   _commitmentArgs.interestRate,
                                   _commitmentArgs.loanDuration
                               ),
                               borrower //cant be msg.sender because of the flash flow
                           )
                       );
   
                   (bidId_) = abi.decode(responseData, (uint256));

2. The smart commitment forwarder’s acceptCommitmentWithRecipient function will be triggered. This function will call its internal _acceptCommitment function:

   // SmartCommitmentForwarder.sol
   
   function _acceptCommitment(
           address _smartCommitmentAddress,
           uint256 _principalAmount,
           uint256 _collateralAmount,
           uint256 _collateralTokenId,
           address _collateralTokenAddress,
           address _recipient,
           uint16 _interestRate,
           uint32 _loanDuration
       ) internal returns (uint256 bidId) {
           ...
   
           bidId = _submitBidWithCollateral(createLoanArgs, _msgSender()); 
   
           _commitment.acceptFundsForAcceptBid(
               _msgSender(), //borrower
               bidId,
               _principalAmount,
               _collateralAmount,
               _collateralTokenAddress,
               _collateralTokenId,
               _loanDuration,
               _interestRate
           );
   
           emit ExercisedSmartCommitment(
               _smartCommitmentAddress,
               _msgSender(),
               _principalAmount,
               bidId
           );
       }

   It is important to note how _msgSender() is passed as the second parameter when calling the _submitBidWithCollateral function. This is done because Teller V2 implements ERC2771, which allows to have a specific market forwarder as a caller of functions. It is then necessary to append the actual msg.sender at the last 20 bytes of the call, so that TellerV2 can fetch the real sender. However, the _msgSender() in this context is the Flash rollover contract, not the user. This is what will cause the bug.

3. The _submitBidWithCollateral will forward the call to TellerV2, and append the _borrower parameter as the last 20 bytes of the calldata so that TellerV2 can extract the borrower when fetching the msg.sender. Because _borrower has been set to the Flash rollover contract, the bid submitted will have the borrower set to the flash rollover contract.

   // SmartCommitmentForwarder.sol
   
   function _submitBidWithCollateral(
           CreateLoanArgs memory _createLoanArgs,
           address _borrower
       ) internal virtual returns (uint256 bidId) {
           bytes memory responseData;
   
           responseData = _forwardCall(
               abi.encodeWithSignature(
                   "submitBid(address,uint256,uint256,uint32,uint16,string,address,(uint8,uint256,uint256,address)[])",
                   _createLoanArgs.lendingToken,
                   _createLoanArgs.marketId,
                   _createLoanArgs.principal,
                   _createLoanArgs.duration,
                   _createLoanArgs.interestRate,
                   _createLoanArgs.metadataURI,
                   _createLoanArgs.recipient,
                   _createLoanArgs.collateral
               ),
               _borrower
           );
   
           return abi.decode(responseData, (uint256));
       }
   

Because of this, the call to _commitment.acceptFundsForAcceptBid performed inside the _acceptCommitment's function inside SmartCommitmentForwarder.sol will always fail because acceptFundsForAcceptBid will try to open a loan where collateral must be transferred from the borrower. Because the borrower has been set to be FlashRolloverLoan_G5, the collateral can’t be transferred and the call will always fail.

Impact

Medium. It is not possible to execute the rolloverLoanWithFlash to open loans where collateral is required, given that the collateral will always be wrongly fetched from the FlashRolloverLoan_G5 contract.

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#L186

Tool used

Manual Review

Recommendation

When executing the rolloverLoanWithFlashfunction in FlashRolloverLoan_G5, the collateral should be transferred from the user and TellerV2 should be approved to operate the collateral. This will make the call work, because FlashRolloverLoan_G5 will hold the collateral assets and they will be correctly pulled from it by TellerV2.

Duplicate of #138