sherlock-admin commented 1 year ago

0xmuxyz

high

A borrower/lender or liquidator will fail to withdraw the collateral assets due to reaching a gas limit

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.

Vulnerability Detail

Within the ICollateralEscrowV1, the Collateral struct would be defined line this: https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/interfaces/escrow/ICollateralEscrowV1.sol#L10-L15

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

Within the CollateralManager, the CollateralInfo struct would be defined like this: https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L34-L37

    /**
     * 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: https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L27

    // 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/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/TellerV2.sol#L311 https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/TellerV2.sol#L325

    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 CollateralManager#commitCollateral(), each collateral asset (info) would be associated with a _bidId respectively by calling the CollateralManager#_commitCollateral() in the for-loop like this: https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L127

    /**
     * @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 returns (bool validation_) {
        address borrower = tellerV2.getLoanBorrower(_bidId);
        (validation_, ) = checkBalances(borrower, _collateralInfo);

        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/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L428 https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L430-L434

    /**
     * @notice Checks the validity of a borrower's collateral balance and commits it to a bid.
     * @param _bidId The id of the associated bid.
     * @param _collateralInfo Additional information about the collateral asset.
     */
    function _commitCollateral(
        uint256 _bidId,
        Collateral memory _collateralInfo
    ) internal virtual {
        CollateralInfo storage collateral = _bidCollaterals[_bidId];
        collateral.collateralAddresses.add(_collateralInfo._collateralAddress);
        collateral.collateralInfo[
            _collateralInfo._collateralAddress
        ] = _collateralInfo;  /// @audit
        ...

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/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L253 https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L255

    /**
     * @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 {
        BidState bidState = tellerV2.getBidState(_bidId);
        if (bidState == BidState.PAID) {
            _withdraw(_bidId, tellerV2.getLoanBorrower(_bidId)); /// @audit 
        } else if (tellerV2.isLoanDefaulted(_bidId)) {
            _withdraw(_bidId, tellerV2.getLoanLender(_bidId));  /// @audit 
           ...

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/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L278

    /**
     * @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
    {
        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/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/CollateralManager.sol#L394-L409

    /**
     * @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.

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.

Code Snippet

Tool used

Manual Review

Recommendation

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

ethereumdegen commented 1 year ago

Thank you for your feedback. This is very similar / essentially the same as the 'collateral poisoning' issue that had been identified in the README of this contest as a known-issue: it had been explained and known that collateral could be made impossible to withdraw which could impact the ability to do the last repayment of a loan. This is a slight variation in that it describes that the collateral could be so vast that withdrawing it would exceed the gas limit of a block. Thank you for this perspective. In any case we do plan to separate the repayment logic from the collateral withdraw logic to mitigate such an issue.

ctf-sec commented 1 year ago

Escalate for 10 USDC. this is low / medium issue.

As the sponsor say

his is very similar / essentially the same as the 'collateral poisoning' issue that had been identified in the README of this contest as a known-issue: it had been explained and known that collateral could be made impossible to withdraw which could impact the ability to do the last repayment of a loan.

according to the previous description, the issue should be marked as low and non-reward

but DOS and exceed the gas limit of block make this a medium, definitely not a high finding

According to https://docs.sherlock.xyz/audits/judging/judging

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

and

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? It would not count if the DOS, etc. lasts a known, finite amount of time <1 year. If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation. The greater the cost of the attack for an attacker, the less severe the issue becomes.

The cost of making the collateral loop for exceed block gas limit is clearly very high + it only impact single lendering / borrow loan, not all loan states.

So this should be a medium / low finiding, definitely not high severity issue.

sherlock-admin commented 1 year ago

Escalate for 10 USDC. this is low / medium issue.

As the sponsor say

his is very similar / essentially the same as the 'collateral poisoning' issue that had been identified in the README of this contest as a known-issue: it had been explained and known that collateral could be made impossible to withdraw which could impact the ability to do the last repayment of a loan.

according to the previous description, the issue should be marked as low and non-reward

but DOS and exceed the gas limit of block make this a medium, definitely not a high finding

According to https://docs.sherlock.xyz/audits/judging/judging

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

and

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? It would not count if the DOS, etc. lasts a known, finite amount of time <1 year. If it will result in funds being inaccessible for >=1 year, then it would count as a loss of funds and be eligible for a Medium or High designation. The greater the cost of the attack for an attacker, the less severe the issue becomes.

The cost of making the collateral loop for exceed block gas limit is clearly very high + it only impact single lendering / borrow loan, not all loan states.

So this should be a medium / low finiding, definitely not high severity issue.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Trumpero commented 1 year ago

This issue differs from the known issue as it highlights a scenario where the loan was successfully accepted but can't be withdrawn due to the gas limit. However, this situation is unlikely, so I agree that it should be a medium issue.

hrishibhat commented 1 year ago

Escalation accepted

Valid medium This can be considered as a valid medium.

sherlock-admin commented 1 year ago

Escalation accepted

Valid medium This can be considered as a valid medium.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

IAm0x52 commented 1 year ago

Fixed here by separating withdraw and repay logic

sherlock-audit / 2023-03-teller-judging

0xmuxyz - A borrower/lender or liquidator will fail to withdraw the collateral assets due to reaching a gas limit #357

A borrower/lender or liquidator will fail to withdraw the collateral assets due to reaching a gas limit

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation