Backed Protocol contest - rayn's results

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L205-L208 https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L215-L218

Vulnerability details

Impact

If an attacker already calls lend() to lend to a loan, the attacker can manipulate loanInfo by reentrancy attack when any lenders try to buyout. The attacker can set bad values of lendInfo (e.g. very long duration, and 0 interest rate) that the lender who wants to buyout don't expect.

Proof of Concept

An attacker lends a loan, and loanAssetContractAddress in loanInfo is ERC777 which is suffering from reentrancy attack. When a lender (victim) try to buyout the loan of the attacker:

1. The victim called lend().
2. In lend(), it always call ERC20(loanAssetContractAddress).safeTransfer to send accumulatedInterest + previousLoanAmount to currentLoanOwner (attacker).
3. The transfer of loanAssetContractAddress ERC777 will call _callTokensReceived so that the attacker can call lend() again in reentrancy with parameters:
   • loanId: same loan Id
   • interestRate: set to bad value (e.g. 0)
   • amount: same amount
   • durationSeconds: set to bad value (e.g. a long durationSeconds)
   • sendLendTicketTo: same address of the attacker (currentLoanOwner)
4. Now the variables in loanInfo are changed to bad value, and the victim will get the lend ticket but the loan term is manipulated, and can not set it back (because it requires a better term).

Tools Used

vim

Recommended Mitigation Steps

Use nonReentrant modifier on lend() to prevent reentrancy attack:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol

Lines of code

https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L205-L208 https://github.com/code-423n4/2022-04-backed/blob/main/contracts/NFTLoanFacilitator.sol#L215-L218

Vulnerability details

Impact

If an attacker (lender) lends to a loan, the attacker can always revert transactions when any lenders try to buyout, making anyone can not buyout the loan of the attacker.

Proof of Concept

1. A victim calls lend(), trying to buyout the loan of the attacker.
2. In lend(), it always call ERC20(loanAssetContractAddress).safeTransfer to send accumulatedInterest + previousLoanAmount to currentLoanOwner (attacker).
3. If the transfer of loanAssetContractAddress is ERC777, it will call _callTokensReceived that the attacker can manipulate and always revert it.
4. Because NFTLoanFacilitator uses safeTransfer and safeTransferFrom to check return value, the transaction of the victim will also be reverted. It makes anyone can not buyout the loan of the attacker.

In _callTokensReceived, the attacker just wants to revert the buyout transaction, but keep repayAndCloseLoan successful. The attacker can call loanInfoStruct(uint256 loanId) in _callTokensReceived to check if the value of loanInfo is changed or not to decide to revert it.

Tools Used

vim

Recommended Mitigation Steps

Don't transfer ERC20(loanAssetContractAddress) to currentLoanOwner in lend(), use a global mapping to record redemption of lenders and add an external function redeem for lenders to transfer ERC20(loanAssetContractAddress).

Summary

We list 2 non-critical findings here:

• (Non) loanEndSeconds does not check valid loanId
• (Non) Should use safe version of ERC721 functions

In conclusion, it's better to check the validity of the mapping’s key. And using a safe version of ERC721 functions is usually a better choice.

(Non) loanEndSeconds does not check valid loanId

Impact

In NFTLoanFacilitator.sol/loanEndSeconds(), it doesn’t check whether loanId is valid

Proof of Concept

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

Recommended Mitigation Steps

Add check for loanId

(Non) Should use safe version of ERC721 functions

Impact

In NFTLoanFacilitator.sol, it uses IERC721Mintable(borrowTicketContract).mint which is not a good practice.

Proof of Concept

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

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

Recommended Mitigation Steps

Use safeMint instead of mint

Use private variables to save gas

Those can be declared as private for gas savings, only creating view functions for variables that should be read externally.

Proof of Concept

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

Recommendation

Declare these variables to private.

Fetch loan fields into memory to save gas

If storage is accessed more than once, it should be fetch into memory to save gas. loan.lastAccumulatedTimestamp is accessed twice in totalOwed and interestOwed

Proof of Concept

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

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

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

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

Recommendation

Fetch loan.lastAccumulatedTimestamp into memory

uint40 lastAccumulatedTimestamp = loan.lastAccumulatedTimestamp;