samuraii77 - Borrower can give more collateral than he should and get it locked as well as possible lose it

[sherlock-admin3]

samuraii77

high

Borrower can give more collateral than he should and get it locked as well as possible lose it

Summary

Borrower can give more collateral than he should and get it locked as well as possible lose it due to the protocol taking all of the provided collateral instead of just the required.

Vulnerability Detail

A borrower can immediately take out a loan under pre-set conditions using the acceptCommitmentWithRecipient() function. He has to input the commitment he is targeting that has those pre-set conditions as well as some other values such as _principalAmount and _collateralAmount. Let's assume he is trying to borrow an ERC20 token and is giving such in return. There is this if check in the internal _acceptCommitment() function:

if (commitmentCollateralTokenType != CommitmentCollateralType.NONE) {
            createLoanArgs.collateral = new Collateral[](1);
            createLoanArgs.collateral[0] = Collateral({
                _collateralType: _getEscrowCollateralType(commitmentCollateralTokenType), // Some kind of standardization
                _tokenId: _collateralTokenId,
                _amount: _collateralAmount,
                _collateralAddress: _collateralTokenAddress // commitment.collateralTokenAddress
            });
        }

If the commitmentCollateralTokenType is not NONE (it's ERC20 in our case), then we add a Collateral struct to the createLoanArgs.

Then, we call _submitBidWithCollateral() which calls the submitBid() function on the TellerV2 contract (the one with the Collateral function argument).

bidId = _submitBidWithCollateral(createLoanArgs, _msgSender());

That function commits the input collateral (committing here means that the collateral is in a way scheduled for taking but hasn't been taken yet):

bool validation = collateralManager.commitCollateral(
            bidId_,
            _collateralInfo
        );

If everything goes well (borrower has as much collateral as he has specified and some other conditions), the validation variable will be true which passes the require statement on the next line. Then, back to _acceptCommitment(), we call this function:

_commitment.acceptFundsForAcceptBid(
            _msgSender(), //borrower
            bidId,
            _principalAmount,
            _collateralAmount,
            _collateralTokenAddress,
            _collateralTokenId,
            _loanDuration,
            _interestRate
        );

This function checks whether the borrower passes the pre-set conditions (e.g., has provided enough collateral for the principal, has specified a high enough interest rate). One of the checks it does is whether the collateral is enough:

        uint256 requiredCollateral = getCollateralRequiredForPrincipalAmount(
            _principalAmount
        );

        require(
            (_collateralAmount * STANDARD_EXPANSION_FACTOR) >=
                requiredCollateral,
            "Insufficient Borrower Collateral"
        );

It then does a few more things and calls TellerV2::lenderAcceptBid() to accept the bid as all of the conditions have passed. There, one of the main things that happen are the collateral being sent to an escrow contract using the committed collateral and the principal being set to the borrower.

        // Tell the collateral manager to deploy the escrow and pull funds from the borrower if applicable
        collateralManager.deployAndDeposit(_bidId);

        //transfer funds to borrower
        if (amountToBorrower > 0) {
            bid.loanDetails.lendingToken.safeTransferFrom(
                sender,
                bid.receiver,
                amountToBorrower
            );
        }

The issue arises because the collateral the borrower specified might be more than required. It is very common and expected that if a borrower provides more collateral than required, he will get the extra amount back. A borrower might provide more collateral in a lot of different cases, he might have not checked the exact amount he is required to send or there might have been a quick change in the market making his specified collateral worth more than the specified principal.

The borrower will not be able to take out his extra collateral until he has paid off the loan leaving the funds locked for a while. Even worse, if he can not pay off the loan for some reason, his collateral will be taken and he will lose more than he should have.

Impact

Borrower can give more collateral than he should and get it locked as well as possible lose it

Code Snippet

https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L481-L576 https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/SmartCommitmentForwarder.sol#L68-L125 https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroup_Smart.sol#L336-L382 https://github.com/sherlock-audit/2024-04-teller-finance/blob/defe55469a2576735af67483acf31d623e13592d/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L314-L343

Tool used

Manual Review

Recommendation

Only take the amount that is required, not the whole amount he specified. Another option would be to calculate the principal amount based on the collateral he provided or vice versa.

[nevillehuang]

Invalid, user input error.