sherlock-audit / 2024-04-teller-finance-judging

10 stars 9 forks source link

EgisSecurity - TellerV2.sol#lenderAcceptBid() - Marketplace fee recipient can be address(0), resulting in a DoS of the function #185

Closed sherlock-admin2 closed 4 months ago

sherlock-admin2 commented 4 months ago

EgisSecurity

medium

TellerV2.sol#lenderAcceptBid() - Marketplace fee recipient can be address(0), resulting in a DoS of the function

Summary

TellerV2.sol#lenderAcceptBid() - Marketplace fee recipient can be address(0), resulting in a DoS of the function

Vulnerability Detail

When a lender wants to accept a bit, he calls lenderAcceptBid.

When a bid is accepted some parts of the principal go to the marketplace fee recipient.

 //transfer fee to marketplace
        if (amountToMarketplace > 0) {
            bid.loanDetails.lendingToken.safeTransferFrom(
                sender,
             marketRegistry.getMarketFeeRecipient(bid.marketplaceId),
                amountToMarketplace
            );
        }

If we take a look at getMarketFeeRecipient we can see that if feeRecipient == address(0), then the market owner will be returned as a fee recipient, but the market owner can also be address(0).

function getMarketFeeRecipient(uint256 _marketId)
        public
        view
        override
        returns (address)
    {
        address recipient = markets[_marketId].feeRecipient;

        if (recipient == address(0)) {
            return _getMarketOwner(_marketId);
        }

        return recipient;
    }

Note that ledenderAcceptBid uses isMarketClosed, which only checks if the market is closed, not that it doesn't have an owner.

/**
     * @notice Returns the status of a market being open or closed for new bids. Does not indicate whether or not a market exists.
     * @param _marketId The market ID for the market to check.
     */
    function isMarketClosed(uint256 _marketId)
        public
        view
        override
        returns (bool)
    {
        return marketIsClosed[_marketId];
    }

This implies that the function is fine with a lender accepting a bid whose market owner is address(0), which can be problematic as shown in this issue.

Impact

Complete DoS on accepting bids that use the marketplace.

Note that there are tokens that allow token transfers where the to address is address(0), in those cases the marketplace fee will be gone forever.

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

Tool used

Manual Review

Recommendation

Rework the marketplace fee sending logic to

if (amountToMarketplace > 0 ) {
            if (marketRegistry.getMarketFeeRecipient(bid.marketplaceId) != address(0))
            {
                bid.loanDetails.lendingToken.safeTransferFrom(
                    sender,
                    marketRegistry.getMarketFeeRecipient(bid.marketplaceId),
                    amountToMarketplace
                );
            }
            else {
                bid.loanDetails.lendingToken.safeTransferFrom(
                    sender,
                    owner()
                    amountToMarketplace
                );
            }
        }

This way the function can always execute and if the market fee recipient is address(0), the fee will just go to the protocol, which is a lot better than being lost.

nevillehuang commented 4 months ago

Invalid, I, don't think it is sensible for any market owner to transfer ownership of market to address zero to cause themselves to lose marketplace fee.