Backed Protocol contest - robee's results

Lines of code

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

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

Vault.sol#addValue() assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. But, the actual transferred amount can be lower for those tokens. Therefore it's recommended to use the balance change before and after the transfer instead of the amount. This way you also support the tokens with transfer fee - that are popular.

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

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

1. 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

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    NFTLoanTicket.constructor (name)
    NFTLoanTicket.constructor (symbol)

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: NFTLoanFacilitator.sol, In line 258, Require message length to shorten: 39, The message: NFTLoanFacilitator: payment is not late
    Solidity file: NFTLoanFacilitator.sol, In line 82, Require message length to shorten: 33, The message: NFTLoanFacilitator: 0 loan amount
    Solidity file: NFTLoanFacilitator.sol, In line 321, Require message length to shorten: 38, The message: NFTLoanFacilitator: 0 improvement rate

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    NFTLoanFacilitator.sol, 321: change '_improvementRate > 0' to '_improvementRate != 0'

Title: Storage double reading. Could save SLOAD Severity: GAS

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    NFTLoanFacilitator.sol: originationFeeRate is read twice in lend

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    LendTicket.sol, _transfer
    NFTLoanFacilitator.sol, _interestOwed

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    LendTicket.sol.constructor
    BorrowTicket.sol.constructor

Title: Use unchecked to save gas for certain additive calculations that cannot overflow Severity: GAS

You can use unchecked in the following calculations since there is no risk to overflow:

    NFTLoanFacilitator.sol (L#382) - return loanAmount * (block.timestamp - lastAccumulatedTimestamp) * (perAnumInterestRate * 1e18 / 365 days) / 1e21 + accumulatedInterest;
    NFTLoanFacilitator.sol (L#258) - require(block.timestamp > loan.durationSeconds + loan.lastAccumulatedTimestamp, "NFTLoanFacilitator: payment is not late"); 

Title: Rearrange state variables Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In NFTLoanFacilitator.sol,rearranging the storage fields can optimize to: 6 slots from: 7 slots. The new order of types (you choose the actual variables): 1. uint256 2. uint256 3. uint256 4. uint256 5. address 6. uint8 7. address

Title: Change transferFrom to transfer Severity: GAS

'transferFrom(address(this), , **)' could be replaced by the following more gas efficient 'transfer(, **)' This replacement is more gas efficient and improves the code quality.

    NFTLoanFacilitator.sol, 124 : IERC721(loan.collateralContractAddress).transferFrom(address(this), sendCollateralTo, loan.collateralTokenId);

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    LendTicket.sol, _transfer

Title: Consider inline the following functions to save gas Severity: GAS

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)


    NFTLoanFacilitator.sol, _interestOwed, { return loanAmount * (block.timestamp - lastAccumulatedTimestamp) * (perAnumInterestRate * 1e18 / 365 days) / 1e21 // SCALAR * 1e18 + accumulatedInterest; }

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    NFTLoanTicket.sol, tokenURI