Joyful Olive Meerkat - A borrower, lender, or liquidator may be unable to withdraw collateral assets if the gas limit is exceeded.

[sherlock-admin3]

Joyful Olive Meerkat

Medium

A borrower, lender, or liquidator may be unable to withdraw collateral assets if the gas limit is exceeded.

Summary

Within the TellerV2#submitBid(), there is no limitation that how many collateral assets a borrower can assign into the _collateralInfo array parameter.

This lead to some bad scenarios like this due to reaching gas limit:

• A borrower or a lender fail to withdraw the collateral assets when the loan would not be liquidated.
• A liquidator will fail to withdraw the collateral assets when the loan would be liquidated.

Root Cause

Within the ICollateralEscrowV1, the Collateral struct would be defined line this:

struct Collateral {
    CollateralType _collateralType;
    uint256 _amount;
    uint256 _tokenId;
    address _collateralAddress;
}

Within the CollateralManager, the CollateralInfo struct would be defined like this:

 /**
     * Since collateralInfo is mapped (address assetAddress => Collateral) that means
     * that only a single tokenId per nft per loan can be collateralized.
     * Ex. Two bored apes cannot be used as collateral for a single loan.
     */
    struct CollateralInfo {
        EnumerableSetUpgradeable.AddressSet collateralAddresses;
        mapping(address => Collateral) collateralInfo;
    }

Within the CollateralManager, the _bidCollaterals storage would be defined like this:

// bidIds -> validated collateral info
    mapping(uint256 => CollateralInfo) internal _bidCollaterals;

When a borrower submits a bid, the TellerV2#submitBid() would be called.
Within the TellerV2#submitBid(), multiple collaterals, which are ERC20/ERC721/ERC1155, can be assigned into the _collateralInfo array parameter by a borrower.
And then, these collateral assets stored into the _collateralInfo array would be associated with bidId_ through internally calling the CollateralManager#commitCollateral() like this:

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L314

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/TellerV2.sol#L328

function submitBid(
        address _lendingToken,
        uint256 _marketplaceId,
        uint256 _principal,
        uint32 _duration,
        uint16 _APR,
        string calldata _metadataURI,
        address _receiver,
        Collateral[] calldata _collateralInfo   <@ audit
    ) public override whenNotPaused returns (uint256 bidId_) {
        ...
        bool validation = collateralManager.commitCollateral(
            bidId_,
            _collateralInfo /// @audit 
        );
        ...

Within the commitCollateral(), each collateral asset (info) would be associated with a _bidId respectively by calling the CollateralManager#_commitCollateral() in the for-loop like this:

 /**
     * @notice Checks the validity of a borrower's multiple collateral balances and commits it to a bid.
     * @param _bidId The id of the associated bid.
     * @param _collateralInfo Additional information about the collateral assets.
     * @return validation_ Boolean indicating if the collateral balances were validated.
     */
    function commitCollateral(
        uint256 _bidId,
        Collateral[] calldata _collateralInfo   <@ audit
    ) public onlyTellerV2 returns (bool validation_) {
        address borrower = tellerV2.getLoanBorrower(_bidId);
        require(borrower != address(0), "Loan has no borrower");
        (validation_, ) = checkBalances(borrower, _collateralInfo);

        //if the collateral info is valid, call commitCollateral for each one
        if (validation_) {
            for (uint256 i; i < _collateralInfo.length; i++) {
                Collateral memory info = _collateralInfo[i];
                _commitCollateral(_bidId, info);  <@ audit
            }
        }
    }

Within the CollateralManager#_commitCollateral(), the _collateralInfo would be stored into the _bidCollaterals storage like this:

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L506

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L522-L526

When the deposited-collateral would be withdrawn by a borrower or a lender, the CollateralManager#withdraw() would be called.
Within the CollateralManager#withdraw(), the CollateralManager#_withdraw() would be called like this:

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L283

/**
     * @notice Withdraws deposited collateral from the created escrow of a bid that has been successfully repaid.
     * @param _bidId The id of the bid to withdraw collateral for.
     */
    function withdraw(uint256 _bidId) external whenProtocolNotPaused {
        BidState bidState = tellerV2.getBidState(_bidId);

        require(bidState == BidState.PAID, "collateral cannot be withdrawn");

        _withdraw(_bidId, tellerV2.getLoanBorrower(_bidId));  <@ audit

        emit CollateralClaimed(_bidId);
    }

When the deposited-collateral would be liquidated by a liquidator, the CollateralManager#liquidateCollateral() would be called.
Within the CollateralManager#liquidateCollateral(), the CollateralManager#_withdraw() would be called like this:

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L355

 /**
     * @notice Sends the deposited collateral to a liquidator of a bid.
     * @notice Can only be called by the protocol.
     * @param _bidId The id of the liquidated bid.
     * @param _liquidatorAddress The address of the liquidator to send the collateral to.
     */
    function liquidateCollateral(uint256 _bidId, address _liquidatorAddress)
        external
        onlyTellerV2
        whenProtocolNotPaused
    {
        if (isBidCollateralBacked(_bidId)) {
            BidState bidState = tellerV2.getBidState(_bidId);
            require(
                bidState == BidState.LIQUIDATED,
                "Loan has not been liquidated"
            );
            _withdraw(_bidId, _liquidatorAddress);  <@audit
        }
    }

Within the CollateralManager#_withdraw(), each collateral asset (collateralInfo._collateralAddress) would be withdrawn by internally calling the ICollateralEscrowV1#withdraw() in a for-loop like this:

https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/main/teller-protocol-v2-audit-2024/packages/contracts/contracts/CollateralManager.sol#L472-L487

 /**
     * @notice Withdraws collateral to a given receiver's address.
     * @param _bidId The id of the bid to withdraw collateral for.
     * @param _receiver The address to withdraw the collateral to.
     */
    function _withdraw(uint256 _bidId, address _receiver) internal virtual {
        for (
            uint256 i;
            i < _bidCollaterals[_bidId].collateralAddresses.length();  <@audit
            i++
        ) {
            // Get collateral info
            Collateral storage collateralInfo = _bidCollaterals[_bidId]
                .collateralInfo[
                    _bidCollaterals[_bidId].collateralAddresses.at(i)
                ];
            // Withdraw collateral from escrow and send it to bid lender
            ICollateralEscrowV1(_escrows[_bidId]).withdraw(   <@ audit
                collateralInfo._collateralAddress,
                collateralInfo._amount,
                _receiver
            );
            ....
        }
    }

However, within the TellerV2#submitBid(), there is no limitation that how many collateral assets a borrower can assign into the _collateralInfo array parameter.

This lead to a bad scenario like below:

• ① A borrower assign too many number of the collateral assets (ERC20/ERC721/ERC1155) into the _collateralInfo array parameter when the borrower call the TellerV2#submitBid() to submit a bid.
• ② Then, a lender accepts the bid via calling the TellerV2#lenderAcceptBid()
• ③ Then, a borrower or a lender try to withdraw the collateral, which is not liquidated, by calling the CollateralManager#withdraw(). Or, a liquidator try to withdraw the collateral, which is liquidated, by calling the CollateralManager#liquidateCollateral()
• ④ But, the transaction of the CollateralManager#withdraw() or the CollateralManager#liquidateCollateral() will be reverted in the for-loop of the CollateralManager#_withdraw() because that transaction will reach a gas limit.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Due to reaching gas limit, some bad scenarios would occur like this:

• A borrower or a lender fail to withdraw the collateral assets when the loan would not be liquidated.
• A liquidator will fail to withdraw the collateral assets when the loan would be liquidated.

PoC

No response

Mitigation

Within the TellerV2#submitBid(), consider adding a limitation about how many collateral assets a borrower can assign into the _collateralInfo array parameter.