Backed Protocol contest - securerodd's results

Backed Contest

April 05, 2022

@securerodd

Low Risks

1. Loan Duration Cannot be Lowered

Contract: NFTLoanFacilitator.sol

The first time a loan is granted in the lend(uint256 loanId, uint16 interestRate, uint128 amount, uint32 durationSeconds, address sendLendTicketTo) function, the loan’s duration is set to the passed in argument of durationSeconds. In order for another lender to take over this loan, they must offer a better loan (i.e. longer duration, better interest rate, or greater amount). Due to this check: require(durationSeconds >= previousDurationSeconds, 'NFTLoanFacilitator: duration too low');, all loans must be offered for at least as long as the loan before it.

Scenario:

1. Alice creates a loan for 1000 UNI using her Milady as collateral
2. Bob lends to Alice by meeting her minimum duration, amount, and interest rate requirements
3. David comes by and replaces Bob's loan by matching the amount and interest rate, but sets the duration to 100 years
4. Anybody else who wants to lend to Alice would have to offer a duration of at least 100 years even if they have a much better rate and amount

Recommendation: Consider allowing a lender to meet the minimum duration if the interest rate and amount are more favorable.

2. Unsafe Mint ERC-20 Operation

There were locations in the NFTLoanFacilitator.sol contract where the mint call was used instead of safeMint. safeMint performs a mint with an additional check that the receiver implements the onERC721Received() function. In both of these cases, the receiving address was a passed in parameter and as a result, it would be possible for a user to accidentally mint to a contract address that would lock the borrow or lend ticket NFT.

Locations:

createLoan(uint256 collateralTokenId, address collateralContractAddress, uint16 maxPerAnumInterest, uint128 minLoanAmount, address loanAssetContractAddress, uint32 minDurationSeconds, address mintBorrowTicketTo) 
-> L102: IERC721Mintable(borrowTicketContract).mint(mintBorrowTicketTo, id);

lend(uint256 loanId, uint16 interestRate, uint128 amount, uint32 durationSeconds, address sendLendTicketTo) 
-> L161: IERC721Mintable(lendTicketContract).mint(sendLendTicketTo, loanId);

Recommendation: Use the safeMint call instead of mint.

3. Unsafe TransferFrom ERC-20 Operation

There was a location in the NFTLoanFacilitator.sol contract where the transferFrom call was used instead of safeTransferFrom. safeTransferFrom performs a transferFrom with an additional check that the receiver implements the onERC721Received() function. In this case, the receiving address was a passed in parameter and as a result, it would be possible for a user to accidentally transfer to a contract address that would lock the NFT that was being held as collateral.

Locations:

closeLoan(uint256 loanId, address sendCollateralTo)
-> L124: IERC721(loan.collateralContractAddress).transferFrom(address(this), sendCollateralTo, loan.collateralTokenId);

Recommendation: Use the safeTransferFrom call instead of transferFrom.

Backed Contest

April 06, 2022

@securerodd

Gas Optimizations

1. != 0 Instead of > 0 in Require Statement

Contract: NFTLoanFacilitator.sol

Function: updateRequiredImprovementRate(uint256 _improvementRate)

Code: require(_improvementRate > 0, 'NFTLoanFacilitator: 0 improvement rate'); Gas: testUpdateRequiredImprovementRateRevertsIf0() (gas: 13310)

Recommended Code: require(_improvementRate != 0, 'NFTLoanFacilitator: 0 improvement rate'); Gas: testUpdateRequiredImprovementRateRevertsIf0() (gas: 13304)