Backed Protocol contest - 0xDjango's results

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L214-L221 https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L230-L250

Vulnerability details

Impact

If borrower lends their own loan, they can repay and close the loan before ownership of the lend ticket is transferred to the new lender. The borrower will keep the NFT + loan amount + accrued interest.

Proof of Concept

This exploit requires that the loanAssetContractAddress token transfers control to the receiver.

Steps of exploit:

• Borrower creates loan with createLoan().
• The same Borrower calls lend(), funding their own loan. The Borrower receives the lend ticket, and funds are transferred to themself.
• A new lender attempts to buy out the loan. The original loan amount + accruedInterest are sent to the original lender (same person as borrower).
• Due to lack of checks-effects-interactions pattern, the borrower is able to immediately call repayAndCloseLoan() before the lend ticket is transferred to the new lender.

The following code illustrates that the new lender sends funds to the original lender prior to receiving the lend ticket in return.

            } else {
                ERC20(loan.loanAssetContractAddress).safeTransferFrom(
                    msg.sender,
                    currentLoanOwner,
                    accumulatedInterest + previousLoanAmount
                );
            }
            ILendTicket(lendTicketContract).loanFacilitatorTransfer(currentLoanOwner, sendLendTicketTo, loanId);

The original lender/borrower calls the following repayAndCloseLoan() function so that they receive their collateral NFT from the protocol.

    function repayAndCloseLoan(uint256 loanId) external override notClosed(loanId) {
        Loan storage loan = loanInfo[loanId];


        uint256 interest = _interestOwed(
            loan.loanAmount,
            loan.lastAccumulatedTimestamp,
            loan.perAnumInterestRate,
            loan.accumulatedInterest
        );
        address lender = IERC721(lendTicketContract).ownerOf(loanId);
        loan.closed = true;
        ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount);
        IERC721(loan.collateralContractAddress).safeTransferFrom(
            address(this),
            IERC721(borrowTicketContract).ownerOf(loanId),
            loan.collateralTokenId
        );


        emit Repay(loanId, msg.sender, lender, interest, loan.loanAmount);
        emit Close(loanId);
    }

Finally, the new lender receives the lend ticket that has no utility at this point. The borrower now possesses the NFT, original loan amount, and accrued interest.

Tools Used

Manual review.

Recommended Mitigation Steps

Move the line to transfer the lend ticket to the new lender above the line to transfer to funds to the original lender. Or, use reentrancyGuard from OpenZeppelin to remove the risk of reentrant calls completely.

If desired, also require that the lender cannot be the same account as the borrower of a loan.

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L155-L160

Vulnerability details

Impact

Since the borrower is able to specify any asset token, it is possible that loans will be created with tokens that support fee on transfer. If a fee on transfer asset token is chosen, the protocol will contain a point of failure on the original lend() call.

It is my belief that this is a medium severity vulnerability due to its ability to impact core protocol functionality.

Proof of Concept

For the first lender to call lend(), if the transfer fee % of the asset token is larger than the origination fee %, the second transfer will fail in the following code:

            ERC20(loanAssetContractAddress).safeTransferFrom(msg.sender, address(this), amount);
            uint256 facilitatorTake = amount * originationFeeRate / SCALAR;
            ERC20(loanAssetContractAddress).safeTransfer(
                IERC721(borrowTicketContract).ownerOf(loanId),
                amount - facilitatorTake
            );

Example:

• originationFee = 2% Max fee is 5% per comments

• feeOnTransfer = 3%

• amount = 100 tokens

• Lender transfers amount

• NFTLoanFacilitator receives 97.

• facilitatorTake = 2

• NFTLoanFacilitator attempts to send 100 - 2 to borrower, but only has 97.

• Execution reverts.

Other considerations:

If the originationFee is less than or equal to the transferFee, the transfers will succeed but will be received at a loss for the borrower and lender. Specifically for the lender, it might be unwanted functionality for a lender to lend 100 and receive 97 following a successful repayment (excluding interest for this example).

Tools Used

Manual review.

Recommended Mitigation Steps

Since the originationFee is calculated based on the amount sent by the lender, this calculation will always underflow given the example above. Instead, a potential solution would be to calculate the originationFee based on the requested loan amount, allowing the lender to send a greater value so that feeOnTransfer <= originationFee.

Oppositely, the protocol can instead calculate the amount received from the initial transfer and use this amount to calculate the originationFee. The issue with this option is that the borrower will receive less than the desired loan amount.

Issue 1 (Low) - Check that NFTLoanFacilitator address is owner of NFT to remove malicious NFT loans

Backed has indicated that understanding potentially malicious NFT contracts are the responsibility of the borrower and lender. This submission is aimed to reduce the effect of malicious NFTs on the health of the protocol.

The NFTLoanFacilitator contract can contain a single check that NFTLoanFacilitator is the owner of the NFT at the end of the createLoan() call. In the contract's current state, the borrower can create a loan with a malicious NFT that was never transferred to the NFTLoanFacilitator contract. A lender would then be responsible to understand that the NFT collateral asset is not able to be seized. Adding the check that NFTLoanFacilitator is the owner of the NFT will reduce the probability of malicious loans sitting on the protocol.

Issue 2 (Non-critical) - Consider moving zero address check to createLoan()

Loans can be created with a faulty loanAssetContractAddress. This address is correctly checked during the lend function, reverting if it equals the zero address. Might as well move this check to the createLoan() function to mitigate unwanted faulty loans sitting in the protocol.

Issue 3 (Non-critical) - Consider adding max duration

Anyone can request a loan with interestRate = 0 and duration as a boundlessly large number. Obviously it is the responsibility of the lender to understand the parameters, but Backed might not desire 0% interest rate loans that basically cannot be collected on due to lengthy terms.

Current max duration is limited by uint32.

maxDuration = 2^32-1 = 4294967295

4294967295 / 60 / 60 / 24 / 365 = 136 years.

Perhaps add maxDuration = 3 years or similar.