Backed Protocol contest - Dravee's results

Lines of code

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

Vulnerability details

Impact

Wrong amount calculated for facilitatorTake

Proof of Concept

Arbitrary ERC20 tokens can be passed as loanAssetContractAddress. With a transfer, the received amount should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter in the ERC20(loanAssetContractAddress).safeTransferFrom() function.

Lines of code

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

Vulnerability details

In the function closeLoan() of contract NFTLoanFacilitator.sol, the transferFrom keyword is used instead of safeTransferFrom. If the arbitrary sendCollateralTo address is a contract and is not aware of the incoming ERC721 token, the sent token could be locked.

I suggest transitioning from transferFrom to safeTransferFrom at line L124.

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");

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • Findings
    • Storage
      • Caching storage values in memory
    • Comparisons
      • > 0 is less efficient than != 0 for unsigned integers (with proof)
    • Arithmetics
      • ++i costs less gas compared to i++ or i += 1
      • Unchecking arithmetics operations that can't underflow/overflow
    • Visibility
      • Consider making some constants as non-public to save gas
    • Errors
      • Reduce the size of error messages (Long revert Strings)
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Findings

Storage

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

contracts/NFTLoanFacilitator.sol:
  174:                 require((previousLoanAmount * requiredImprovementRate / SCALAR) <= amountIncrease //@audit gas: should cache requiredImprovementRate
  175:                 || previousDurationSeconds + (previousDurationSeconds * requiredImprovementRate / SCALAR) <= durationSeconds   //@audit gas: should use cached requiredImprovementRate
  176                  || (previousInterestRate != 0 // do not allow rate improvement if rate already 0
  177:                     && previousInterestRate - (previousInterestRate * requiredImprovementRate / SCALAR) >= interestRate), //@audit gas: should use cached requiredImprovementRate
  
  231          Loan storage loan = loanInfo[loanId];
  232  
  233          uint256 interest = _interestOwed(
  234:             loan.loanAmount, //@audit gas: should cache loan.loanAmount
  235              loan.lastAccumulatedTimestamp,
  236              loan.perAnumInterestRate,
  237              loan.accumulatedInterest
  238          );
  239          address lender = IERC721(lendTicketContract).ownerOf(loanId);
  240          loan.closed = true;
  241:         ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount); //@audit gas: should use cached loan.loanAmount
  242          IERC721(loan.collateralContractAddress).safeTransferFrom(
  243              address(this),
  244              IERC721(borrowTicketContract).ownerOf(loanId),
  245              loan.collateralTokenId
  246          );
  247  
  248:         emit Repay(loanId, msg.sender, lender, interest, loan.loanAmount); //@audit gas: should use cached loan.loanAmount
  249          emit Close(loanId);
  250      }

  338          Loan storage loan = loanInfo[loanId];
  339          if (loan.closed || loan.lastAccumulatedTimestamp == 0) return 0;
  340  
  341:         return loanInfo[loanId].loanAmount + _interestOwed( //@audit gas: should use loan.loanAmount instead of loanInfo[loanId].loanAmount
  342              loan.loanAmount,
  343              loan.lastAccumulatedTimestamp,
  344              loan.perAnumInterestRate,
  345              loan.accumulatedInterest
  346          );

Comparisons

> 0 is less efficient than != 0 for unsigned integers (with proof)

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)

Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706

I suggest changing > 0 with != 0 here:

NFTLoanFacilitator.sol:321:        require(_improvementRate > 0, 'NFTLoanFacilitator: 0 improvement rate');

Also, please enable the Optimizer.

Arithmetics

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

LendTicket.sol:39:            balanceOf[from]--;
LendTicket.sol:41:            balanceOf[to]++;

I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tags for more details):

contracts/NFTLoanFacilitator.sol:
  156              uint256 facilitatorTake = amount * originationFeeRate / SCALAR;
  157              ERC20(loanAssetContractAddress).safeTransfer(
  158                  IERC721(borrowTicketContract).ownerOf(loanId),
  159:                 amount - facilitatorTake //@audit gas: should be unchecked as facilitatorTake is < amount and can't underflow L156 (originationFeeRate is upper bounded and always < SCALAR)

  209                  uint256 facilitatorTake = (amountIncrease * originationFeeRate / SCALAR);
  210                  ERC20(loanAssetContractAddress).safeTransfer(
  211                      IERC721(borrowTicketContract).ownerOf(loanId),
  212:                     amountIncrease - facilitatorTake //@audit gas: should be unchecked as facilitatorTake is < amount and can't underflow L209 (originationFeeRate is upper bounded and always < SCALAR)

Visibility

Consider making some constants as non-public to save gas

Reducing from public to private or internal can save gas when a constant isn't used outside of its contract. I suggest changing the visibility from public to internal or private here:

NFTLoanFacilitator.sol:21:    uint8 public constant override INTEREST_RATE_DECIMALS = 3;
NFTLoanFacilitator.sol:24:    uint256 public constant override SCALAR = 10 ** INTEREST_RATE_DECIMALS;

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

NFTLoanFacilitator.sol:118:        "NFTLoanFacilitator: borrow ticket holder only");
NFTLoanFacilitator.sol:121:        require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
NFTLoanFacilitator.sol:178:                "NFTLoanFacilitator: proposed terms must be better than existing terms");
NFTLoanFacilitator.sol:189:            "NFTLoanFacilitator: accumulated interest exceeds uint128");
NFTLoanFacilitator.sol:255:        "NFTLoanFacilitator: lend ticket holder only");
NFTLoanFacilitator.sol:259:        "NFTLoanFacilitator: payment is not late");
NFTLoanTicket.sol:15:        require(msg.sender == address(nftLoanFacilitator), "NFTLoanTicket: only loan facilitator"); 

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

LendTicket.sol:32:        require(from == ownerOf[id], "WRONG_FROM");
LendTicket.sol:34:        require(to != address(0), "INVALID_RECIPIENT");
NFTLoanFacilitator.sol:53:        require(!loanInfo[loanId].closed, "NFTLoanFacilitator: loan closed");
NFTLoanFacilitator.sol:81:        require(minDurationSeconds != 0, 'NFTLoanFacilitator: 0 duration');
NFTLoanFacilitator.sol:82:        require(minLoanAmount != 0, 'NFTLoanFacilitator: 0 loan amount');
NFTLoanFacilitator.sol:83:        require(collateralContractAddress != lendTicketContract,
NFTLoanFacilitator.sol:85:        require(collateralContractAddress != borrowTicketContract, 
NFTLoanFacilitator.sol:117:        require(IERC721(borrowTicketContract).ownerOf(loanId) == msg.sender,
NFTLoanFacilitator.sol:121:        require(loan.lastAccumulatedTimestamp == 0, "NFTLoanFacilitator: has lender, use repayAndCloseLoan");
NFTLoanFacilitator.sol:144:            require(loanAssetContractAddress != address(0), "NFTLoanFacilitator: invalid loan");
NFTLoanFacilitator.sol:146:            require(interestRate <= loan.perAnumInterestRate, 'NFTLoanFacilitator: rate too high');
NFTLoanFacilitator.sol:147:            require(durationSeconds >= loan.durationSeconds, 'NFTLoanFacilitator: duration too low');
NFTLoanFacilitator.sol:148:            require(amount >= loan.loanAmount, 'NFTLoanFacilitator: amount too low');
NFTLoanFacilitator.sol:171:                require(interestRate <= previousInterestRate, 'NFTLoanFacilitator: rate too high');
NFTLoanFacilitator.sol:172:                require(durationSeconds >= previousDurationSeconds, 'NFTLoanFacilitator: duration too low');
NFTLoanFacilitator.sol:174:                require((previousLoanAmount * requiredImprovementRate / SCALAR) <= amountIncrease
NFTLoanFacilitator.sol:188:            require(accumulatedInterest <= type(uint128).max,
NFTLoanFacilitator.sol:254:        require(IERC721(lendTicketContract).ownerOf(loanId) == msg.sender, 
NFTLoanFacilitator.sol:258:        require(block.timestamp > loan.durationSeconds + loan.lastAccumulatedTimestamp,
NFTLoanFacilitator.sol:280:        require(lendTicketContract == address(0), 'NFTLoanFacilitator: already set');
NFTLoanFacilitator.sol:290:        require(borrowTicketContract == address(0), 'NFTLoanFacilitator: already set');
NFTLoanFacilitator.sol:307:        require(_originationFeeRate <= 5 * (10 ** (INTEREST_RATE_DECIMALS - 2)), "NFTLoanFacilitator: max fee 5%");
NFTLoanFacilitator.sol:321:        require(_improvementRate > 0, 'NFTLoanFacilitator: 0 improvement rate');
NFTLoanTicket.sol:15:        require(msg.sender == address(nftLoanFacilitator), "NFTLoanTicket: only loan facilitator");

I suggest replacing revert strings with custom errors.