089.md

chaduke

medium

acceptCommitment() will always revert for the case of commitment.collateralTokenType == CommitmentCollateralType.ERC721

Summary

acceptCommitment() will always revert for the case of commitment.collateralTokenType == CommitmentCollateralType.ERC721. The main problem is that function getRequiredCollateral() will work for ERC20 but not for ERC721.

Vulnerability Detail

The acceptCommitment() function allows a borrower to accept a lender's loan offer commitement using an ERC20/ERC721/ERC1155 collateral.

https://github.com/sherlock-audit/2023-03-teller/blob/main/teller-protocol-v2/packages/contracts/contracts/LenderCommitmentForwarder.sol#L300-L388

However, the following piece of code, which is supposed only for ERC20/ERC1155 collateral, will revert when executed for a ERC721 collateral.

uint256 requiredCollateral = getRequiredCollateral(
            _principalAmount,
            commitment.maxPrincipalPerCollateralAmount,
            commitment.collateralTokenType,
            commitment.collateralTokenAddress,
            commitment.principalTokenAddress
        );

        if (_collateralAmount < requiredCollateral) {
            revert InsufficientBorrowerCollateral({
                required: requiredCollateral,
                actual: _collateralAmount
            });
        }

The problem lies in the getRequiredCollateral() function, which will revert for ERC721 collateral.

function getRequiredCollateral(
        uint256 _principalAmount,
        uint256 _maxPrincipalPerCollateralAmount,
        CommitmentCollateralType _collateralTokenType,
        address _collateralTokenAddress,
        address _principalTokenAddress
    ) public view virtual returns (uint256) {
        if (_collateralTokenType == CommitmentCollateralType.NONE) {
            return 0;
        }

        uint8 collateralDecimals;
        uint8 principalDecimals = IERC20MetadataUpgradeable(
            _principalTokenAddress
        ).decimals();

        if (_collateralTokenType == CommitmentCollateralType.ERC20) {
            collateralDecimals = IERC20MetadataUpgradeable(
                _collateralTokenAddress
            ).decimals();
        }

        /*
         * The principalAmount is expanded by (collateralDecimals+principalDecimals) to increase precision
         * and then it is divided by _maxPrincipalPerCollateralAmount which should already been expanded by principalDecimals
         */
        return
            MathUpgradeable.mulDiv(
                _principalAmount,
                (10**(collateralDecimals + principalDecimals)),
                _maxPrincipalPerCollateralAmount,
                MathUpgradeable.Rounding.Up
            );
    }

When commitment.collateralTokenType == CommitmentCollateralType.ERC721, we only need one NFT as a collateral, and we have _maxPrincipalPerCollateralAmount = 0, as a result, the getRequiredCollateral() function will have a divide-by-zero error and revert.

Impact

acceptCommitment() will always revert for the case of commitment.collateralTokenType == CommitmentCollateralType.ERC721

Code Snippet

Tool used

VSCode

Manual Review

Recommendation

Change the piece of code in question only for ERC20/ERC1155/ERC1155_ANY_ID.

function acceptCommitment(
        uint256 _commitmentId,
        uint256 _principalAmount,
        uint256 _collateralAmount,
        uint256 _collateralTokenId,
        address _collateralTokenAddress,
        uint16 _interestRate,
        uint32 _loanDuration
    ) external returns (uint256 bidId) {
        address borrower = _msgSender();

        Commitment storage commitment = commitments[_commitmentId];

        validateCommitment(commitment);

        require(
            _collateralTokenAddress == commitment.collateralTokenAddress,
            "Mismatching collateral token"
        );
        require(
            _interestRate >= commitment.minInterestRate,
            "Invalid interest rate"
        );
        require(
            _loanDuration <= commitment.maxDuration,
            "Invalid loan max duration"
        );

        require(
            commitmentBorrowersList[_commitmentId].length() == 0 ||
                commitmentBorrowersList[_commitmentId].contains(borrower),
            "unauthorized commitment borrower"
        );

        if (_principalAmount > commitment.maxPrincipal) {
            revert InsufficientCommitmentAllocation({
                allocated: commitment.maxPrincipal,
                requested: _principalAmount
            });
        }

+       if (
+           commitment.collateralTokenType == CommitmentCollateralType.ERC20 ||
+          commitment.collateralTokenType == CommitmentCollateralType.ERC1155 || 
+          commitment.collateralTokenType == CommitmentCollateralType.ERC1155_ANY_ID || 
+      ){

                 uint256 requiredCollateral = getRequiredCollateral(
                _principalAmount,
                commitment.maxPrincipalPerCollateralAmount,
                commitment.collateralTokenType,
                commitment.collateralTokenAddress,
                commitment.principalTokenAddress
            );

           if (_collateralAmount < requiredCollateral) {
                revert InsufficientBorrowerCollateral({
                required: requiredCollateral,
                actual: _collateralAmount});
           }
+    }

        if (
            commitment.collateralTokenType == CommitmentCollateralType.ERC721 ||
            commitment.collateralTokenType ==
            CommitmentCollateralType.ERC721_ANY_ID
        ) {
            require(
                _collateralAmount == 1,
                "invalid commitment collateral amount for ERC721"
            );
        }

        if (
            commitment.collateralTokenType == CommitmentCollateralType.ERC721 ||
            commitment.collateralTokenType == CommitmentCollateralType.ERC1155
        ) {
            require(
                commitment.collateralTokenId == _collateralTokenId,
                "invalid commitment collateral tokenId"
            );
        }

        bidId = _submitBidFromCommitment(
            borrower,
            commitment.marketId,
            commitment.principalTokenAddress,
            _principalAmount,
            commitment.collateralTokenAddress,
            _collateralAmount,
            _collateralTokenId,
            commitment.collateralTokenType,
            _loanDuration,
            _interestRate
        );

        _acceptBid(bidId, commitment.lender);

        _decrementCommitment(_commitmentId, _principalAmount);

        emit ExercisedCommitment(
            _commitmentId,
            borrower,
            _principalAmount,
            bidId
        );
    }