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

Potential issue: Overfunded Loan #52

Open
adibas03 opened this issue Jul 20, 2019 · 10 comments
Open

Potential issue: Overfunded Loan #52

adibas03 opened this issue Jul 20, 2019 · 10 comments
Labels
bug Something isn't working

Comments

@adibas03
Copy link
Collaborator

With the implementation of the ERC20 stable coin,
more than the principal can be sent to the smart contracts as described in #32 ,
This becomes a problem if a user natively sends tokens to the contract address during the fund process, which means the user can send any amount, and will not be recorded as a contributor to the crowdloan, and will get not repayment.
The issue to the borrower can be removed, by removing limitation of the amount of principalDisbursed been worked on by @onggunhao , so any amount can be disbured, and that would be used in calculating the repayment.
The problem to the sender of the said funds, is that they can not get the funds back, and it would be sent to the contributors as part of the repayment, which the sender will not partake of.

@adibas03 adibas03 added the bug Something isn't working label Jul 20, 2019
@onggunhao
Copy link
Contributor

True. Is it common practice to have people be responsible for their own mistakes, in EthDev?

@adibas03
Copy link
Collaborator Author

In the case of the ERC20 token, the user is responsible for their activities,
which is part of the reason the UI is provided, to aid the users of the smart contracts.
I think another thing we can do is to put out a lot of data/intructions

@adibas03
Copy link
Collaborator Author

@tspoff do you have any thoughts on this?

@onggunhao
Copy link
Contributor

There is a scenario where an overfunded loan would cause an issue:

  1. If there is more Dai in the Crowdloan that what was principalRequested (i.e. native transfers)

  2. The current termsContract will not allow the loan to startRepaymentCycle as the principalDisbursed is greater than principalRequested

  3. We have this check in place to make sure that the borrower is not saddled with more debt than what they originally requested

A few possible solutions:

  1. We allow the borrower to withdraw the extra amount, but loan terms are based only on principalRequested.

  2. We only allow the borrower to withdraw a maximum of principalRequested, and the extra amount is locked in the smart contract.

@onggunhao
Copy link
Contributor

I've added an implementation of approach (1) in this commit.

f1aba44

Borrower can use crowdloan:withdraw() to get the full amount, but termsContract:startRepaymentCycle is capped at principalRequested

@onggunhao onggunhao mentioned this issue Jul 22, 2019
4 tasks
@adibas03
Copy link
Collaborator Author

Freezing the tokens in the smart Contract is not my first choice,
I would think allow the lender withdraw the amount, and the major issue will be having to pay the interest on the extra funds.
Since calculations should be based off principalDisbursed, we can just update the cap for principalDisbursed

@adibas03
Copy link
Collaborator Author

adibas03 commented Jul 22, 2019

I like the idea on discord by @onggunhao ,
of treating over-funded or non-documented funds as donations,
but it'll require that we ensure that a donation is a donation, and does not need to be paid pack.
So, the donation should be deducted from the amount to be repaid.
Which means we need will need to track the exact amount funded, and use that, similar to @onggunhao previous implementation, and allow the borrower withdraw the total amount

@adibas03
Copy link
Collaborator Author

Further thinking:
We already have the total value raised in the repaymentManager< it is called totalShares,
so, that gives us the actual value that was funded using the loan fund process, it is presently pegged at one-to-one as the principal.
So, we allow unlimited withdraw, we use the totalShares from the repaymentManager for the repayment flow. we can still record the balanceOf(crowdloan to know total amount sent in: donation+loan
@onggunhao

@onggunhao
Copy link
Contributor

@adibas03 Do you mean allowing the borrower to withdraw the funds?

I think this shouldn't be an issue if the totalCrowdfunded is below the principalRequested. The borrower will be paying interest on funds they requested, which is fine IMHO.

To note: in that scenario (totalCrowdfunded + native ERC20 transfers < principalRequested), it is actually the proper lenders who benefit, as their loan shares are worth more than they actually invested.

For example:
totalCrowdfunded: 48,000 Dai
Native transfers (unauthorized): 2000 Dai

They basically will be getting interest on 50,000 Dai lent out at 6% interest, split up among 48,000 shareholders. Good deal!

@adibas03
Copy link
Collaborator Author

adibas03 commented Jul 22, 2019

Yes, the borrower should be able to withdraw all funds.
there is no limitation on withdrawal <= principalRequested.

So, in the case of:
principalRequested: 50,000 Dai
totalCrowdfunded: 48,000 Dai
Native transfers (unauthorized): 5000 Dai

The 5000 Dai is taken as donations, not to be refunded by the borrower.
the 48,000 will be refunded as agreed between the borrower and the lender.
totalCrowdfunded = repaymentManager.totalShares(),
so we don't need to do any additional calculation or tracking of amount funded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants