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

Mutual consent cannot be revoked and stays valid forever #33

Open
code423n4 opened this issue Nov 4, 2022 · 4 comments
Open

Mutual consent cannot be revoked and stays valid forever #33

code423n4 opened this issue Nov 4, 2022 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-02 primary issue Highest quality submission among a set of duplicates satisfactory Finding meets requirement selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/utils/MutualConsent.sol#L11-L68
https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L247-L262

Vulnerability details

Impact

Contracts that inherit from the MutualConsent contract, have access to a mutualConsent modifier.
Functions that use this modifier need consent from two parties to be called successfully.

Once one party has given consent for a function call, it cannot revoke consent.
This means that the other party can call this function at any time now.

This opens the door for several exploitation paths.
Most notably though the functions LineOfCredit.setRates(), LineOfCredit.addCredit() and LineOfCredit.increaseCredit() can cause problems.

One party can use Social Engineering to make the other party consent to multiple function calls and exploit the multiple consents.

Proof of Concept

  1. A borrower and lender want to change the rates for a credit.
    The borrower wants to create the possibility for himself to change the rates in the future without the lender's consent.
  2. The borrower and lender agree to set dRate and fRate to 5%.
  3. The lender calls the LineOfCredit.setRates() function to give his consent.
  4. The borrower might now say to the lender "Let's put the rate to 5.1% instead, I will give an extra 0.1%"
  5. The borrower and lender now both call the LineOfCredit.setRates() function to set the rates to 5.1%.
  6. The borrower can now set the rates to 5% at any time. E.g. they might increase the rates further in the future (the borrower playing by the rules)
    and at some point the borrower can decide to set the rates to 5%.

Links:
MutualConsent contract: https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/MutualConsent.sol

LineOfCredit.setRates() function: https://github.com/debtdao/Line-of-Credit/blob/e8aa08b44f6132a5ed901f8daa231700c5afeb3a/contracts/modules/credit/LineOfCredit.sol#L247-L262

Tools Used

VSCode

Recommended Mitigation Steps

There are several options to fix this issue:

  1. Add a function to the MutualConsent contract to revoke consent for a function call.
  2. Make consent valid only for a certain amount of time.
  3. Invalidate existing consents for a function when function is called with different arguments.

Option 3 requires a lot of additional bookkeeping but is probably the cleanest solution.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 4, 2022
code423n4 added a commit that referenced this issue Nov 4, 2022
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Nov 15, 2022
@c4-judge
Copy link
Contributor

dmvt marked the issue as primary issue

@c4-judge
Copy link
Contributor

dmvt marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Nov 17, 2022
@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Nov 30, 2022
@c4-sponsor
Copy link

kibagateaux marked the issue as sponsor confirmed

@c4-judge
Copy link
Contributor

c4-judge commented Dec 6, 2022

dmvt marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory Finding meets requirement label Dec 6, 2022
@C4-Staff C4-Staff added the M-02 label Dec 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-02 primary issue Highest quality submission among a set of duplicates satisfactory Finding meets requirement selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants