QA Report - Githubissues

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    NFTLoanFacilitator.sol, updateOriginationFeeRate
    NFTLoanFacilitator.sol, updateRequiredImprovementRate

Title: Does not validate the input fee parameter Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    NFTLoanFacilitator.updateOriginationFeeRate (_originationFeeRate)

Title: Add a timelock Severity: Low Risk

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/NFTLoanFacilitator.sol#L279
    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/NFTLoanFacilitator.sol#L289

Title: Mult instead div in compares Severity: Low Risk

To improve algorithm precision instead using division in comparison use multiplication in the following scenario:

        Instead a < b / c use a * c < b. 

In all of the big and trusted contracts this rule is maintained.

    NFTLoanFacilitator.sol, 177, && previousInterestRate - (previousInterestRate * requiredImprovementRate / SCALAR) >= interestRate), "NFTLoanFacilitator: proposed terms must be better than existing terms");
    NFTLoanFacilitator.sol, 175, || previousDurationSeconds + (previousDurationSeconds * requiredImprovementRate / SCALAR) <= durationSeconds
    NFTLoanFacilitator.sol, 174, require((previousLoanAmount * requiredImprovementRate / SCALAR) <= amountIncrease

Title: Check transfer receiver is not 0 to avoid burned money Severity: Low Risk

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/NFTLoanFacilitator.sol#L155
    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/LendTicket.sol#L48
    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/LendTicket.sol#L21
    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/NFTLoanFacilitator.sol#L262
    https://github.com/code-423n4/2022-04-backed/tree/main/contracts/NFTLoanFacilitator.sol#L88

Title: Override function but with different argument location Severity: Low/Med Risk

    BorrowTicket.sol.constructor inherent NFTLoanTicket.sol.constructor but the parameters does not match
    LendTicket.sol.constructor inherent NFTLoanTicket.sol.constructor but the parameters does not match

Title: transfer return value of a general ERC20 is ignored Severity: Low: Can be even High Risk

Need to use safeTransfer instead of transfer. As there are popular tokens, such as USDT that transfer/trasnferFrom method doesn’t return anything. The transfer return value has to be checked (as there are some other tokens that returns false instead revert), that means you must

Check the transfer return value Another popular possibility is to add a whiteList. Those are the appearances (solidity file, line number, actual line):
```
NFTLoanFacilitator.sol, 88 (createLoan), IERC721(collateralContractAddress).transferFrom(msg.sender, address(this), collateralTokenId);
```

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.

    NFTLoanFacilitator.sol.withdrawOriginationFees asset
    NFTLoanFacilitator.sol.createLoan loanAssetContractAddress
    NFTLoanFacilitator.sol.lend sendLendTicketTo
    LendTicket.sol.loanFacilitatorTransfer to
    NFTLoanFacilitator.sol.setLendTicketContract _contract

code-423n4 / 2022-04-backed-findings

QA Report #31