Backed Protocol contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

Take other lender loanTicket without improving any of the loan conditions

Proof of Concept

The bug is in NftLoanFacilliator.sol in the function lend() (link1). && previousInterestRate - (previousInterestRate * requiredImprovementRate / SCALAR) >= interestRate), for previousInterestRate < 10 (which is basiclly a rate that is < 1%) ,existing: previousInterestRate * requiredImprovementRate < 10^3 and then previousInterestRate * requiredImprovementRate / SCALAR = 0 which means any user can take the loanTicket from a lender without really lowering the interest for the borrower. (Note it is only possible if previousInterestRate < 1% but this is a fair assumption).

Tools Used

Recommended Mitigation Steps

Low and Non-critical Bugs

• If a user will pass the address of the NFTLoanFacilitator contract as the sendCollateralTo argument of the closeLoan function, the NFT will be locked in the contract. There are 2 ways to deal with it - either by checking that the given address is not the address of the NFTLoanFacilitator contract, or by adding a withdrawERC721 function. The second way is better because there might be other ways to get an NFT locked in the contract. This is also correct for the seizeCollateral function.

Gas Optimizations

• can use unchecked in the lend function because we know that facilitatorTake < amountIncrease (because facilitatorTake = (amountIncrease * originationFeeRate / SCALAR) and originationFeeRate <= SCALAR)

  ERC20(loanAssetContractAddress).safeTransfer(
      IERC721(borrowTicketContract).ownerOf(loanId),
      amountIncrease - facilitatorTake
  );

• These 2 requires are redundant, because they are checked in the next require

  // These 2 are redundant
  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");

• use one require instead of two in the createLoan function to save gas

  require(collateralContractAddress != lendTicketContract && 
          collateralContractAddress != borrowTicketContract, 
          'NFTLoanFacilitator: cannot use tickets as collateral');

• Inline the call to _transfer in the loanFacilitatorTransfer function (simply replace the call to transfer with the function call, the _transfer function is an internal function which is not used in any other place except the loanFacilitatorTransfer function)

  function loanFacilitatorTransfer(address from, address to, uint256 loanId) external override loanFacilitatorOnly {
      _transfer(from, to, loanId);
  }

• Calculate values pre-deploy instead of calculating them in the deployment

  uint256 public constant override SCALAR = 10 ** INTEREST_RATE_DECIMALS;
  uint256 public override originationFeeRate = 10 ** (INTEREST_RATE_DECIMALS - 2);
  uint256 public override requiredImprovementRate = 10 ** (INTEREST_RATE_DECIMALS - 1);

  The initial value of INTEREST_RATE_DECIMALS is 3 so these initial values can be calculated pre-deploy (1000, 10 and 100)