Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loan offer's required collateral tokenId is not validated in some conditions, borrower can use any NFT to initiate loans #119

Open
c4-bot-10 opened this issue May 24, 2024 · 5 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

https://github.com/pixeldaogg/florida-contracts/blob/7bacbe3f2b4c1bb6c87961e3553118a6e6c2dcee/src/lib/loans/MultiSourceLoan.sol#L850

Vulnerability details

Impacts

Loan offer's required collateral tokenId is not validated in some conditions, borrower can use any NFT to initiate loans.

Proof of concept

When a lender generate LoanOffer, they can either specify a specific NFT tokenId, or allow a collection offer (any tokenId within the NFT collection). LoanOffer and executionData's collateral Id match is checked in _checkValidators().

Based on code doc, a lender's LoanOffer struct -> the collateral tokenId required :
(1) Empty validator array input -> lender want the exact tokenId match;
(2) Single validator element input && _loanOffer.validators[0].validator == address(0) -> lender accepts any token in the collection;
(3) Non-empty validator array &&_loanOffer.validators[0].validator != address(0) -> lender wants offer validators to check the offer.

|>  /// @notice Check generic offer validators for a given offer or
    ///         an exact match if no validators are given. The validators
    ///         check is performed only if tokenId is set to 0.
    ///         Having one empty validator is used for collection offers (all IDs match).
...
    function _checkValidators(LoanOffer calldata _loanOffer, uint256 _tokenId) private view {
...

Edge case: A lender wants an exact tokenId match and the tokenId is 0.

Based on the code doc, an exact match check should be performed when no validators are given. So lender's LoanOffer will be: (1) _loanOffer.nftCollateralTokenId =0 ; (2) _loanOffer.validators.length == 0;

However, _checkValidators() will not check this loanOffer at all. Due to empty validator array is provided, exact tokenId match is not checked due to vulnerable check if (_loanOffer.nftCollateralTokenId != 0){.... In the else branch, for-loop will be directly skipped, and exit the function with no checks.

    function _checkValidators(LoanOffer calldata _loanOffer, uint256 _tokenId) private view {
       uint256 offerTokenId = _loanOffer.nftCollateralTokenId;
        //@audit This is vulnerable check condition, will cause tokenId=0 and empty validators to be skipped entirely
 |>     if (_loanOffer.nftCollateralTokenId != 0) {
            if (offerTokenId != _tokenId) {
                revert InvalidCollateralIdError();
            }
        } else {
            uint256 totalValidators = _loanOffer.validators.length;
            if (totalValidators == 0 && _tokenId != 0) {
                revert InvalidCollateralIdError();
            } else if ((totalValidators == 1) && _isZeroAddress(_loanOffer.validators[0].validator)) {
                return;
            }
            for (uint256 i = 0; i < totalValidators;) {
                IBaseLoan.OfferValidator memory thisValidator = _loanOffer.validators[i];
                IOfferValidator(thisValidator.validator).validateOffer(_loanOffer, _tokenId, thisValidator.arguments);
                unchecked {
                    ++i;
                }
            }
        }
    }

(https://github.com/pixeldaogg/florida-contracts/blob/7bacbe3f2b4c1bb6c87961e3553118a6e6c2dcee/src/lib/loans/MultiSourceLoan.sol#L850)

emitLoan() →_processOffersFromExecutionData() → validateOfferExecution()→ _checkValidators()

As a result, a borrower can use any collateral tokenId when the lender requires tokenId 0.

Tools

Manual

Recommendations

Refactor the if control flow in _checkValidators() to always check tokenId for exact match if empty validator array

Assessed type

Other

@c4-bot-10 c4-bot-10 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 24, 2024
c4-bot-7 added a commit that referenced this issue May 24, 2024

Verified

This commit was signed with the committer’s verified signature.
calcastor BT (calcastor/mame)
@0xend
Copy link

0xend commented May 25, 2024

Not sure I follow. if loanOffer.nftCollateralTokenId = 0 and there are not validators, then we check f (totalValidators == 0 && _tokenId != 0) which would revert if the tokenId of the actual loan is any other than 0?

@0xend 0xend added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label May 25, 2024
@alex-ppg
Copy link

alex-ppg commented Jun 1, 2024

The Warden has demonstrated a limitation of the system when dealing with token IDs of 0 which are valid per the EIP-721 standard. I believe this is a valid issue with the codebase, and I advise the Sponsor (@0xend) to revisit it as the condition they mention would not revert given that _tokenId would be deliberately 0 to match a token ID of 0.

I believe a medium risk assessment is fair, as there is no documented limitation of the system's ability to support Token IDs of 0 and any orders involving them would be incorrectly validated and effectively not be fulfilled properly.

@c4-judge
Copy link

c4-judge commented Jun 1, 2024

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jun 1, 2024
@c4-judge
Copy link

c4-judge commented Jun 1, 2024

alex-ppg marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Jun 1, 2024
@0xend
Copy link

0xend commented Jun 24, 2024

Can you please explain the exact scenario and how it's passing these 2 tests.

function testEmitZeroTokenId() public {
        IMultiSourceLoan.LoanOffer memory loanOffer =
            _getSampleOffer(address(collateralCollection), collateralTokenId, _INITIAL_PRINCIPAL);
        uint256 tokenId = 0;
        loanOffer.nftCollateralTokenId = tokenId;
        collateralCollection.mint(userA, tokenId);

        vm.startPrank(_borrower);
        collateralCollection.approve(address(_msLoan), tokenId);
        _msLoan.emitLoan(_sampleLoanExecutionData(loanOffer));
        vm.stopPrank();

        assertEq(collateralCollection.ownerOf(tokenId), address(_msLoan));
    }

    function testEmitZeroTokenIdInvalid() public {
        IMultiSourceLoan.LoanOffer memory loanOffer =
            _getSampleOffer(address(collateralCollection), collateralTokenId, _INITIAL_PRINCIPAL);
        uint256 tokenId = 0;
        loanOffer.nftCollateralTokenId = tokenId;
        collateralCollection.mint(userA, tokenId);

        vm.startPrank(_borrower);
        collateralCollection.approve(address(_msLoan), tokenId);

        IMultiSourceLoan.LoanExecutionData memory lde = _sampleLoanExecutionData(loanOffer);
        lde.executionData.tokenId = 1;
        vm.expectRevert(abi.encodeWithSignature("InvalidCollateralIdError()"));
        _msLoan.emitLoan(lde);
        vm.stopPrank();
    }```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

4 participants