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 inconsistent state in LendingTermOffboarding can lead to redemptions remaining paused forever #1255

Closed
c4-bot-8 opened this issue Dec 28, 2023 · 5 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 downgraded by judge Judge downgraded the risk level of this issue duplicate-1147 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-bot-8
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L154
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L191-L195

Vulnerability details

Impact

The LendingTermOffboarding contract has a potential issue that could lead to an inconsistent state in the system. This inconsistency could brick redemptions and disrupt the normal functioning of the lending term offboarding process.

The issue arises when a lending term is offboarded and then immediately re-onboarded without the cleanup() function being called. This would allow anyone to immediately offboard the term again, leading to an incorrect value in nOffboardingsInProgress. This would in turn block the unpausing of redemptions in the PSM as the nOffboardingsInProgress variable could not be decreased down to 0 again.

Proof of Concept

Consider the following sequence of events:

  1. A LendingTerm is offboarded with the intention of calling all loans and immediately re-onboarding it (due to e.g. some loans being so old that the interest accrued brings them close to being underwater)
  2. The LendingTerm is immediately re-onboarded.
  3. No one calls the cleanup() function during the time it is being offboarded.
  4. Since canOffboard[term] is still true, anyone can call the offboard() function again.
  5. This adds the term to the _deprecatedGauges set again and increases nOffboardingsInProgress to 2.
  6. Now, cleanup() can only be called once as canOffboard[term] will be false on subsequent calls, which makes it impossible to unpause redemptions in the PSM.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L154
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L191-L195

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, consider adding a check in the proposeOnboard() function to ensure that a term cannot be re-onboarded if it hasn't been cleaned up. This could be done by checking if LendingTermOffboarding.canOffboard[term] is false before allowing the term to be onboarded.

Assessed type

Other

@c4-bot-8 c4-bot-8 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 28, 2023
c4-bot-1 added a commit that referenced this issue Dec 28, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Jan 1, 2024
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #1147

@c4-judge
Copy link
Contributor

Trumpero marked the issue as duplicate of #1141

@c4-judge
Copy link
Contributor

Trumpero marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Jan 25, 2024
@c4-judge
Copy link
Contributor

Trumpero changed the severity to 2 (Med Risk)

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 downgraded by judge Judge downgraded the risk level of this issue duplicate-1147 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants