Backed Protocol contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

In case of collateral price fluctuation, lender can create a situation where borrower would be unable to repay the loan and lender could get hold of collateral

Proof of Concept

1. User creates a loan request for duration 1 day and minimum amount as 0.1 million usdt and puts his collateral NFT
2. Lender lends the amount with the same conditions to this user
3. Due to favorable news, the collateral NFT price hikes too much say now becomes 0.5 million USDT
4. Lender wants borrower to be unable to repay loan so that he could get hold of borrower collateral
5. Lender immediately funds additional 0.2 million usdt to borrower under same loan id.
6. The facilitatorTake fees is deducted on additional amount 0.2 million. Assuming the fees comes to be X so the amount transferred to borrower would be 0.2-X million
7. Borrower panics as he never needed this much amount and want to simply close the loan but he cannot close as he is short of amount X and duration of 1 day is not enough to get X amount
8. As a result, borrower is unable to repay on time and lender uses the opportunity to grab borrower collateral giving him a plain benefit of 0.5-0.1-0.2 million = 0.2 million

Recommended Mitigation Steps

Always have a max lending amount. Also it would be great to have a feature where borrower can accept/reject lending offers

Lines of code

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

Vulnerability details

Impact

Since some tokens take transfer fees on performing transfer operations and current contract implementation is not considering same, lender funds could be lost

Proof of Concept

1. Attacker creates a loan request for token XYZ taking 10% transfer fees
2. Lender lends amount X of token XYZ to Attacker
3. After some duration Attacker repay and close the loan but Lender will only receive X-10%+interest which is lesser

Recommended Mitigation Steps

Obtain the actual amount which is transferred to/from the contract

1. NFTLoanFacilitator.sol#L116 -> closeLoan function -> Attacker can create 10000+ loans with fake colleteral which will spam the system. Loan closing and opening from same wallet address should be allowed after certain duration of to prevent spamming. Even Admin cannot remove these fake loans from system, new function should be introduced to allow same

2. NFTLoanFacilitator.sol#L296 -> withdrawOriginationFees function -> Attacker creates loan using malicious loanAssetContractAddress token, Attacker himself lend the token which deducts the OriginationFees. Admin later tries withdrawing fees using withdrawOriginationFees which cause malicious code to execute

3. NFTLoanFacilitator.sol#L129 -> lend function -> Lender could set a very high duration (say 100 years) by mistake. His lended funds and interest will get stuck till 100 years are passed. Recommendation is to set a max duration for loan tenure

4. No functionality for loanAssetContractAddress migration. In case any security issue is found in loanAssetContractAddress and token is moved to new address, there is no way for admin to update the same in NFTLoanFacilitator contract. This will block borrower from closing the loan

LendTicket.sol#L27 -> _transfer function -> Add check from!=to