Backed Protocol contest - FSchmoede's results

QA report

Non-critical

Use safeTransferFrom for ERC721 consistency

In the contract NFTLoanFacilitator.sol there is a mix of using IERC721.safeTransferFrom() and IERC721.transferFrom(). The methods repayAndCloseLoan() and seizeCollateral() uses the safeTransferFrom(), where the methods createLoan() and closeLoan() uses the transferFrom().

For the method closeLoan() it should just be changed, since it transfers the collateral from the contract to another address just like the methods repayAndCloseLoan() and seizeCollateral(), which both uses the safeTransferFrom().

However, in order to change the method createLoan() to use the safeTransferFrom() it will require the contract to implement the IERC721Receive.onERC721Received() method.

Notice! Currently the method closeLoan() does not make use of the safeTransferFrom() and this means that if a user provided a contract address as the parameter sendCollateralTo and this contract does not implement proper handling of ERC721, then the token would be lost. But since I see this as very unlikely, I choose to consider this as a non-critical instead of a low.

Gas optimization report

Keep revert strings below 32 bytes

Strings are stored in slots of 32 bytes, and hence the length of the revert string should be at max 32 bytes to fit inside 1 slot. If the string is just 33 bytes it will occupy 2 slots (64 bytes). Keeping the string size at 32 bytes or below will save gas on both deployment and when the revert condition is met.

Since the used version of Solidity is >=0.8.4 it would also be worth considering using Custom Errors which is both more gas efficient and allows thorough error descriptions using NatSpec.

The relevant code:

NFTLoanFacilitator.sol line 84:   "NFTLoanFacilitator: cannot use tickets as collateral"
NFTLoanFacilitator.sol line 86:   "NFTLoanFacilitator: cannot use tickets as collateral"
NFTLoanFacilitator.sol line 118:  "NFTLoanFacilitator: borrow ticket holder only"
NFTLoanFacilitator.sol line 121:  "NFTLoanFacilitator: has lender, use repayAndCloseLoan"
NFTLoanFacilitator.sol line 146:  "NFTLoanFacilitator: rate too high"
NFTLoanFacilitator.sol line 147:  "NFTLoanFacilitator: duration too low"
NFTLoanFacilitator.sol line 148:  "NFTLoanFacilitator: amount too low"
NFTLoanFacilitator.sol line 171:  "NFTLoanFacilitator: rate too high"
NFTLoanFacilitator.sol line 172:  "NFTLoanFacilitator: duration too low"
NFTLoanFacilitator.sol line 178:  "NFTLoanFacilitator: proposed terms must be better than existing terms"
NFTLoanFacilitator.sol line 189:  "NFTLoanFacilitator: accumulated interest exceeds uint128"
NFTLoanFacilitator.sol line 255:  "NFTLoanFacilitator: lend ticket holder only"
NFTLoanFacilitator.sol line 259:  "NFTLoanFacilitator: payment is not late"
NFTLoanFacilitator.sol line 321:  "NFTLoanFacilitator: 0 improvement rate"
NFTLoanFacilitator.sol line 86:

NFTLoanTicket.sol line 15:        "NFTLoanTicket: only loan facilitator"

Cache loanAmount

In the following code the variable loanInfo[loanId].loanAmount is read from storage 3 times (see audit-info comments), and hence should be cached so it is only read from storage once.

function repayAndCloseLoan(uint256 loanId) external override notClosed(loanId) {
        Loan storage loan = loanInfo[loanId];

        uint256 interest = _interestOwed(
            loan.loanAmount, @audit-info SLOAD1
            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); @audit-info SLOAD2
        IERC721(loan.collateralContractAddress).safeTransferFrom(
            address(this),
            IERC721(borrowTicketContract).ownerOf(loanId),
            loan.collateralTokenId
        );

        emit Repay(loanId, msg.sender, lender, interest, loan.loanAmount); @audit-info SLOAD3
        emit Close(loanId);
    }

Change this into

function repayAndCloseLoan(uint256 loanId) external override notClosed(loanId) {
        Loan storage loan = loanInfo[loanId];
        uint128 loanAmount = loan.loanAmount;

        uint256 interest = _interestOwed(
            loanAmount,
            loan.lastAccumulatedTimestamp,
            loan.perAnumInterestRate,
            loan.accumulatedInterest
        );
        address lender = IERC721(lendTicketContract).ownerOf(loanId);
        loan.closed = true;
        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, loanAmount);
        emit Close(loanId);
    }

The change is passing the provided test suite, and the .gas-snapshot reflected the change by reducing the gas costs from:

NFTLoanFacilitatorGasBenchMarkTest:testRepayAndClose() (gas: 81320 -> 81064)
NFTLoanFacilitatorTest:testRepayAndCloseSuccessful() (gas: 447725 -> 447469)
NFTLoanFacilitatorTest:testRepayInterestOwedExceedingUint128() (gas: 465901 -> 465645)