QA Report

[code423n4]

QA Report

1. Immutable addresses should be 0-checked

Consider adding an address(0) check here:

File: NFTLoanTicket.sol
20:     constructor(
21:         string memory name, 
22:         string memory symbol, 
23:         NFTLoanFacilitator _nftLoanFacilitator, 
24:         NFTLoansTicketDescriptor _descriptor
25:     ) 
26:         ERC721(name, symbol) 
27:     {
28:         nftLoanFacilitator = _nftLoanFacilitator;  //@audit low: should be 0 checked
29:         descriptor = _descriptor;  //@audit low: should be 0 checked
30:     }

2. NFTLoanFacilitator.sol uses Ownable's default transferOwnership() instead of implementing a 2-step ownership transfer pattern

3. renounceOwnership() can be called in NFTLoanFacilitator.sol. Consider overriding the method to always keep an owner.

4. Comment says "private" instead of "internal" :

File: NFTLoanFacilitator.sol
369:     // === private === //@audit should say internal
370: 
371:     /// @dev Returns the total interest owed on loan
372:     function _interestOwed(
373:         uint256 loanAmount,
374:         uint256 lastAccumulatedTimestamp,
375:         uint256 perAnumInterestRate,
376:         uint256 accumulatedInterest
377:     ) 
378:         internal 
379:         view 
380:         returns (uint256) 
381:     {

5. onERC721Received not implemented in borrowTicketContract (BorrowTicket.sol)

The IERC721.safeTransferFrom call will trigger onERC721Received here:

File: NFTLoanFacilitator.sol
242:         IERC721(loan.collateralContractAddress).safeTransferFrom(
243:             address(this),
244:             IERC721(borrowTicketContract).ownerOf(loanId),
245:             loan.collateralTokenId
246:         );

It must return its Solidity selector to confirm the token transfer. If any other value is returned or the interface is not implemented by the recipient, the transfer will be reverted.

The selector can be obtained in Solidity with IERC721.onERC721Received.selector.

6. Code style: some strings are declared with '', others with "". I suggest only using one style.

Strings with '':

contracts/NFTLoanFacilitator.sol:
   81:         require(minDurationSeconds != 0, 'NFTLoanFacilitator: 0 duration');
   82:         require(minLoanAmount != 0, 'NFTLoanFacilitator: 0 loan amount');
   84:         'NFTLoanFacilitator: cannot use tickets as collateral');
   86:         'NFTLoanFacilitator: cannot use tickets as collateral');
  146:             require(interestRate <= loan.perAnumInterestRate, 'NFTLoanFacilitator: rate too high');
  147:             require(durationSeconds >= loan.durationSeconds, 'NFTLoanFacilitator: duration too low');
  148:             require(amount >= loan.loanAmount, 'NFTLoanFacilitator: amount too low');
  171:                 require(interestRate <= previousInterestRate, 'NFTLoanFacilitator: rate too high');
  172:                 require(durationSeconds >= previousDurationSeconds, 'NFTLoanFacilitator: duration too low');
  280:         require(lendTicketContract == address(0), 'NFTLoanFacilitator: already set');
  290:         require(borrowTicketContract == address(0), 'NFTLoanFacilitator: already set');
  321:         require(_improvementRate > 0, 'NFTLoanFacilitator: 0 improvement rate');

Strings with "":

contracts/LendTicket.sol:
  32:         require(from == ownerOf[id], "WRONG_FROM");
  34:         require(to != address(0), "INVALID_RECIPIENT");

contracts/NFTLoanFacilitator.sol:
   53:         require(!loanInfo[loanId].closed, "NFTLoanFacilitator: loan closed");
  118:         "NFTLoanFacilitator: borrow ticket holder only");
  121:         require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
  144:             require(loanAssetContractAddress != address(0), "NFTLoanFacilitator: invalid loan");
  178:                 "NFTLoanFacilitator: proposed terms must be better than existing terms");
  189:             "NFTLoanFacilitator: accumulated interest exceeds uint128");
  255:         "NFTLoanFacilitator: lend ticket holder only");
  259:         "NFTLoanFacilitator: payment is not late");
  307:         require(_originationFeeRate <= 5 * (10 ** (INTEREST_RATE_DECIMALS - 2)), "NFTLoanFacilitator: max fee 5%"); 

contracts/NFTLoanTicket.sol:
  15:         require(msg.sender == address(nftLoanFacilitator), "NFTLoanTicket: only loan facilitator");

[wilsoncusack]

will do 4.