Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bad Incentives For Lenders Who Buy Out Other Lenders #23

Closed
code423n4 opened this issue Apr 6, 2022 · 3 comments
Closed

Bad Incentives For Lenders Who Buy Out Other Lenders #23

code423n4 opened this issue Apr 6, 2022 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

Lenders are allowed to "buy out" another lender on a position via the loan function.

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

This is supposed to be a purely positive sum action for the borrower, as the new lender must provide "better" terms than their predecessor, as checked here:

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

However, in practice, this mechanic appears fundamentally flawed for a number of reasons:

Extending a loan's duration is not necessarily positive sum

By increasing the duration of a loan, the lender is also increasing the amount of interest the borrower must pay to keep their NFT at the end of the loan. If a borrower created a loan only expecting to have to pay $100 in interest over 12 months, if another lend came and extended the loan by 100 years, they'd now have to pay $10100 to keep their NFT. Even if the borrower noticed this and went to close their loan, this is a potential griefing vector, as the lender can keep extending their loan and force the borrower to pay gas each time to close it. There is a low cost for the attacker if the loan amount is low, as with a long enough duration the interest paid can outpace the principal.

Increasing a loan's amount is not necessarily positive sum

For most of the same reasons above, the ability to increase the amount being loaned to a user at any time is dangerous. A malicious lender could 10x a user's loan amount, forcing them to pay more interest to keep their NFT or risk having it seized. Sure the borrower could just default on purpose and keep the tokens, but if the NFT is precious for non monetary reasons or they are simply unaware the borrower could unintentionally wind up paying far more interest then they expected when they created their loan. Once again, even if the borrower is aware of the increased amount and goes to close their loan, this opens up a griefing vector as described above.

Recommended Mitigation Steps

Either:

  • Remove the ability to extend a loan's duration or amount entirely (allowing a new lender to propose a lower borrowing rate appears safe)
  • Allow the borrower to set limits on the max duration and amount of the loan
  • Require the lender's approval before their loan can be bought out on new terms

Or a implement a combination of two or more of these options.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Apr 6, 2022
code423n4 added a commit that referenced this issue Apr 6, 2022
@wilsoncusack
Copy link
Collaborator

again, this is known/intended protocol design

@wilsoncusack
Copy link
Collaborator

is basically duplicate of the max loan amount ones but will leave

@wilsoncusack wilsoncusack added the sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue label Apr 6, 2022
@gzeoneth
Copy link
Member

The part regarding loan duration is invalid, the borrower can close the loan anytime by paying current accumulatedInterest. Duplicate of #24

@gzeoneth gzeoneth added the duplicate This issue or pull request already exists label Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue
Projects
None yet
Development

No branches or pull requests

3 participants