search

code-423n4 / 2022-04-backed-findings

1 stars 1 forks source link

When an attacker lends to a loan, the attacker can trigger DoS that any lenders can not buyout it #89

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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).

wilsoncusack commented 2 years ago

I think this is just part of perils of working with certain assets and I am not sure we will mitigate

wilsoncusack commented 2 years ago

Sorry I lost track of this one/labeled incorrectly. This is indeed an issue we intend to address: we will block erc777 tokens.

The worse implication here is that a lender could prevent a borrower from repaying and could seize the NFT.

wilsoncusack commented 2 years ago

Still not sure if this should be high or medium. But there are legit ERC777 tokens that a borrower might selecting unknowingly, so probably is high?

gzeoneth commented 2 years ago

I suggest this as Med Risk as no fund is loss by preventing buyout.

wilsoncusack commented 2 years ago

I suggest this as Med Risk as no fund is loss by preventing buyout.

But as I said above the bigger issue is they could block repayment, guaranteeing default and seizure of collateral?

gzeoneth commented 2 years ago

I suggest this as Med Risk as no fund is loss by preventing buyout.

But as I said above the bigger issue is they could block repayment, guaranteeing default and seizure of collateral?

I think you are correct as there is a similar call in L241. However both warden failed to describe such attack path and I am inclined to keep this as Med Risk.

wilsoncusack commented 2 years ago

resolved with https://github.com/with-backed/backed-protocol/pull/69