Backed Protocol contest - WatchPug's results

Lines of code

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

Vulnerability details

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

function closeLoan(uint256 loanId, address sendCollateralTo) external override notClosed(loanId) {
    require(IERC721(borrowTicketContract).ownerOf(loanId) == msg.sender,
    "NFTLoanFacilitator: borrow ticket holder only");

    Loan storage loan = loanInfo[loanId];
    require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
    
    loan.closed = true;
    IERC721(loan.collateralContractAddress).transferFrom(address(this), sendCollateralTo, loan.collateralTokenId);
    emit Close(loanId);
}

The sendCollateralTo will receive the collateral NFT when closeLoan() is called. However, if sendCollateralTo is a contract address that does not support ERC721, the collateral NFT can be frozen in the contract.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

Recommendation

Change to:

function closeLoan(uint256 loanId, address sendCollateralTo) external override notClosed(loanId) {
    require(IERC721(borrowTicketContract).ownerOf(loanId) == msg.sender,
    "NFTLoanFacilitator: borrow ticket holder only");

    Loan storage loan = loanInfo[loanId];
    require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
    
    loan.closed = true;
    IERC721(loan.collateralContractAddress).safeTransferFrom(address(this), sendCollateralTo, loan.collateralTokenId);
    emit Close(loanId);
}

Lines of code

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

Vulnerability details

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

{
    uint256 previousInterestRate = loan.perAnumInterestRate;
    uint256 previousDurationSeconds = loan.durationSeconds;

    require(interestRate <= previousInterestRate, 'NFTLoanFacilitator: rate too high');
    require(durationSeconds >= previousDurationSeconds, 'NFTLoanFacilitator: duration too low');

    require((previousLoanAmount * requiredImprovementRate / SCALAR) <= amountIncrease
    || previousDurationSeconds + (previousDurationSeconds * requiredImprovementRate / SCALAR) <= durationSeconds 
    || (previousInterestRate != 0 // do not allow rate improvement if rate already 0
        && previousInterestRate - (previousInterestRate * requiredImprovementRate / SCALAR) >= interestRate), 
    "NFTLoanFacilitator: proposed terms must be better than existing terms");
}

The requiredImprovementRate represents the percentage of improvement required of at least one of the terms when buying out from a previous lender.

However, when previousInterestRate is less than 10 and requiredImprovementRate is 100, due to precision loss, the new interestRate is allowed to be the same as the previous one.

Making such an expected constraint absent.

PoC

1. Alice createLoan() with maxPerAnumInterest = 10, received loanId = 1
2. Bob lend() with interestRate = 9 for loanId = 1
3. Charlie lend() with interestRate = 9 (and all the same other terms with Bob) and buys out loanId = 1

Charlie is expected to provide at least 10% better terms, but actually bought out Bob with the same terms.

Recommendation

Consider using: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.5.0/contracts/utils/math/Math.sol#L39-L42

And change the check to:

(previousInterestRate != 0 // do not allow rate improvement if rate already 0
        && previousInterestRate - Math.ceilDiv(previousInterestRate * requiredImprovementRate, SCALAR) >= interestRate)

Lines of code

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

Vulnerability details

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

IERC721Mintable(borrowTicketContract).mint(mintBorrowTicketTo, id);

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanTicket.sol#L33-L35

function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
    _mint(to, tokenId);
}

If mintBorrowTicketTo is a contract that does not implement the onERC721Received method, in the current implementation of createLoan(), the tx will still be successfully, and the loan will be created.

This can be a problem if mintBorrowTicketTo can not handle ERC721 properly, as the BorrowTicket NFT will be used later to get back the user's funds.

Recommendation

Consider using safeMint in NFTLoanTicket.sol#mint():

function mint(address to, uint256 tokenId) external override loanFacilitatorOnly {
    _safeMint(to, tokenId);
}

[N] Misleading comments

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

/**
    * @notice updates the percent improvement required of at least one loan term when buying out lender 
    * a loan that already has a lender. E.g. setting this value to 10 means duration or amount
    * must be 10% higher or interest rate must be 10% lower. 
    * @dev Cannot be 0.
    */
function updateRequiredImprovementRate(uint256 _improvementRate) external onlyOwner {
    require(_improvementRate > 0, 'NFTLoanFacilitator: 0 improvement rate');

    requiredImprovementRate = _improvementRate;

    emit UpdateRequiredImprovementRate(_improvementRate);
}

"10 means duration or amount must be 10% higher" should be "100 means duration or amount must be 10% higher".

[N] Implementations should comply with the interfaces

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

    /** 
     * See {INFTLoanFacilitator-INTEREST_RATE_DECIMALS}.     
     * @dev lowest non-zero APR possible = (1/10^3) = 0.001 = 0.1%
     */
    uint8 public constant override INTEREST_RATE_DECIMALS = 3;

The current interface is:

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/interfaces/INFTLoanFacilitator.sol#L18-L22

    /**
     * @notice The magnitude of SCALAR
     * @dev 10^INTEREST_RATE_DECIMALS = 1 = 100%
     */
    function INTEREST_RATE_DECIMALS() external returns (uint8);

Should be:

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/interfaces/INFTLoanFacilitator.sol#L18-L22

    /**
     * @notice The magnitude of SCALAR
     * @dev 10^INTEREST_RATE_DECIMALS = 1 = 100%
     */
    function INTEREST_RATE_DECIMALS() external view returns (uint8);

Otherwise, it can not be used in view context, eg:

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/descriptors/libraries/PopulateSVGParams.sol#L52-L62

    function interestRateString(NFTLoanFacilitator nftLoanFacilitator, uint256 perAnumInterestRate) 
        private 
        view 
        returns (string memory)
    {
        return UintStrings.decimalString(
            perAnumInterestRate,
            nftLoanFacilitator.INTEREST_RATE_DECIMALS() - 2,
            true
            );
    }