Backed Protocol contest - joshie's results

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L148 https://github.com/code-423n4/2022-04-backed/blob/e8015d7c4b295af131f017e646ba1b99c8f608f0/contracts/NFTLoanFacilitator.sol#L174

Vulnerability details

Impact

Creating a loan with a minimum amount, instead of a fixed/maximum amount may lead to forcing the borrower to pay more interest than he was expecting or hoping for. The user never knows the potential maximum amount he will need to pay, until the loan is actually completed/aborted. Neither does he have the option to set it out.

Example:

• NFT_A has floor of 100 ETH.
• Bob creates a loan with a minimum amount of 50 ETH, interest rate of 10% and giving NFT_A as collateral. Projected interest to pay: 5 ETH

Scenario A

• Alice accepts the conditions and loans 85 ETH. Projected interest: 8.5 ETH.
• Bob is not confortable.
• Bob has to choose between repaying it, or wait for someone to take up the loan on a smaller interest rate.

Scenario B

• Alice accepts the conditions and loans 50 ETH. Projected interest: 5 ETH.
• Bob uses the lent amount in some farm with a withdraw penalty or lock-up period
• Alice sees it, and increases the loan amount in 10% chunks until it reaches 90 ETH. Projected interest ~9 ETH
• Bob is in trouble if no one takes up the loan on a smaller interest rate.

Things to consider:

• If the loan duration is small, then it becomes more problematic.
• Currently, the user should always be ready to be paying his chosen interest rate against nft_floor - lowest_profit_margin_on_seizure
• If the floor rises, then it becomes more problematic.

Code:

NFTLoanFacilitator:148

require(amount >= loan.loanAmount, 'NFTLoanFacilitator: amount too low');

NFTLoanFacilitator:174

require((previousLoanAmount * requiredImprovementRate / SCALAR) <= amountIncrease

Recommended Mitigation Steps

One of the following:

• Set amount as fixed or capped.
• Add the option to set a maximum interest to be payed. (could be shielded)
• Create different kinds of loans: minimum, fixed and/or capped amount
• Be explicit on the UI about the risks of it.

-- 1 All functions that receive uints which are not uint256 could be changed to uint256 and only casted to the intended size when storing it. More info at: https://github.com/ourzora/v3/pull/125#issuecomment-1034238815

Just swapped the lend() function and consequent related tests resulting in: (...) Overall gas change: -11839 (-0.041%) But there are many other functions (eg. createLoan) which could use such change, so the gas saving should be way more than that.

-- 2 cache borrowTicketContract at createLoan to save one SLOAD on a successful transaction.

-- 3 use cached previousInterestRate on calling _interestOwed

Overall gas change: -60 (-0.000%)

diff --git a/contracts/NFTLoanFacilitator.sol b/contracts/NFTLoanFacilitator.sol
index 46d6ef5..bb25958 100644
--- a/contracts/NFTLoanFacilitator.sol
+++ b/contracts/NFTLoanFacilitator.sol
@@ -165,6 +165,7 @@ contract NFTLoanFacilitator is Ownable, INFTLoanFacilitator {
             // will underflow if amount < previousAmount
             uint256 amountIncrease = amount - previousLoanAmount;
 
+            uint256 accumulatedInterest;
             {
                 uint256 previousInterestRate = loan.perAnumInterestRate;
                 uint256 previousDurationSeconds = loan.durationSeconds;
@@ -177,14 +178,14 @@ contract NFTLoanFacilitator is Ownable, INFTLoanFacilitator {
                 || (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");
-            }
 
-            uint256 accumulatedInterest = _interestOwed(
-                previousLoanAmount,
-                loan.lastAccumulatedTimestamp,
-                loan.perAnumInterestRate,
-                loan.accumulatedInterest
-            );
+                 = _interestOwed(
+                    previousLoanAmount,
+                    loan.lastAccumulatedTimestamp,
+                    previousInterestRate,
+                    loan.accumulatedInterest
+                );
+            }
             require(accumulatedInterest <= type(uint128).max,
             "NFTLoanFacilitator: accumulated interest exceeds uint128");

-- 4

cache loan.loanAmount on repayAndCloseLoan

testRepayInterestOwedExceedingUint128() (gas: -253 (-0.001%)) 
testRepayAndCloseSuccessful() (gas: -253 (-0.001%)) 
testRepayAndClose() (gas: -253 (-0.003%)) 
Overall gas change: -759 (-0.004%)

diff --git a/contracts/NFTLoanFacilitator.sol b/contracts/NFTLoanFacilitator.sol
index f1f570f..ff2e3f7 100644
--- a/contracts/NFTLoanFacilitator.sol
+++ b/contracts/NFTLoanFacilitator.sol
@@ -231,23 +231,24 @@ contract NFTLoanFacilitator is Ownable, INFTLoanFacilitator {
     /// See {INFTLoanFacilitator-repayAndCloseLoan}.
     function repayAndCloseLoan(uint256 loanId) external override notClosed(loanId) {
         Loan storage loan = loanInfo[loanId];
+        uint256 _loanAmount = loan.loanAmount;
 
         uint256 interest = _interestOwed(
-            loan.loanAmount,
+            _loanAmount,
             loan.lastAccumulatedTimestamp,
             loan.perAnumInterestRate,
             loan.accumulatedInterest
         );
         address lender = IERC721(lendTicketContract).ownerOf(loanId);
         loan.closed = true;
-        ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + loan.loanAmount);
+        ERC20(loan.loanAssetContractAddress).safeTransferFrom(msg.sender, lender, interest + _loanAmount);
         IERC721(loan.collateralContractAddress).safeTransferFrom(
             address(this),
             IERC721(borrowTicketContract).ownerOf(loanId),
             loan.collateralTokenId
         );
 
-        emit Repay(loanId, msg.sender, lender, interest, loan.loanAmount);
+        emit Repay(loanId, msg.sender, lender, interest, _loanAmount);
         emit Close(loanId);
     }