Can force borrower to pay huge interest

[code423n4]

Lines of code

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

Vulnerability details

Impact

The loan amount is used as a min loan amount. It can be matched as high as possible (realistically up to the collateral NFT's worth to remain in profit) and the borrower has to pay interest on the entire amount instead of just on the desired loan amount when the loan was created.

POC

• User needs a 10k USDC loan, NFTs are illiquid and they only have a BAYC worth 350k$. So buying another NFT worth roughly the desired 10k$ is not feasible. They will put the entire 350k$ BAYC as collateral for the 10k USDC loan.
• A lender matches the loan calling lend with 350k USDC.
• The borrower now has to pay interest on the entire 350k USDC even though they only wanted a 10k loan. Otherwise, they risk losing their collateral. Their effective rate on their 10k loan is 35x higher.

Recommended Mitigation Steps

The loan amount should not have min amount semantics.
When someone wants to get a loan, they specify a certain amount they need, they don't want to receive and pay interest on more than that.

[wilsoncusack]

The ability to increase the loan amount is seen as a feature of the protocol, not a bug

[wilsoncusack]

#30

[gzeoneth]

While a larger loan size is strictly beneficial to the borrower, the higher interest payment it entitled is not. The warden suggested a valid situation that may cost the user more than intended. Considering the amount lost is bounded because the lender carry more risk for a larger loan, downgrading this to Mid Risk for the sponsor to consider a maxLoanAmount parameter.

[gzeoneth]

After considering #9 brining up the loan origination fee, I believe this is a high risk issue for the protocol to not have a maxLoanAmount parameter.

[wilsoncusack]

After considering #9 brining up the loan origination fee, I believe this is a high risk issue for the protocol to not have a maxLoanAmount parameter.

IMO it does not make sense to label this as high severity. This is not an exploit but is just the protocol working exactly as described in the README.

[gzeoneth]

From README

Perpetual lender buyout: a lender can be boughtout at any time by a new lender who meets the existing terms and beats at least one term by at least 10%, e.g. 10% longer duration, 10% higher loan amount, 10% lower interest. The new lender pays the previous lender their principal + any interest owed. The loan duration restarts on buyout.

I don't agree that allowing higher loan amount necessarily mean the loan amount need to be unbounded. Given the increased interest and origination fee, a higher loan amount is not necessarily "beating existing terms" as described in the README.

[wilsoncusack]

It certainly doesn't necessarily mean that but it is how we chose to implement it and I think the description is clear that there is no cap. We define "beating" as having one of those values changed by at least 10% and so I think it is beating as described by the readme.

Nonetheless, I appreciate your drawing focus again to this point (we discussed on twitter with our community during audit as this became a point of interest, and have of course considered this idea when designing the protocol at the outset). We will again consider adding a Boolean flag to each loan as to whether the borrower allows loan amount increases

[wilsoncusack]

Respect judge to have final say, but since this is going public want to make sure our take on this is clear.

We believe the protocol design was clearly communicated in the README, including origination fee and the possibility for perpetually increasing loan amount. We think there is no "exploit" here, just people pointing out potential downsides to how the protocol is designed (as one might point out problems of impermanent loss with an AMM.) We view these as QA reports. We are interested in this feedback and listening to it in that we want to listen to potential users and make sure our protocol appeals to as many people as possible.

[gzeoneth]

I consider this as an exploit because asset can be lost. Combining unbounded loan amount, interest rate and origination fee (max 5%) a malicious lender can grief borrower with limited risk and get a chance to seize the collateral as demonstrated in the POC.

The fact that the code is working as described in README is irrelevant if it is going to make user lose their asset. If this is going to stay as a protocol design, I recommend to clearly communicate the risk of unbounded loan amount which is lacking in the contest repo.

[wilsoncusack]

Was resolved here with-backed/backed-protocol#75