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

Can re-onboard a lending term which hasn't been properly cleaned up, allowing it to be immediately offboarded at any time #280

Closed
c4-bot-8 opened this issue Dec 21, 2023 · 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 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/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154

Vulnerability details

Impact

If a LendingTerm is re-onboarded after being offboarded but before being cleaned up with LendingTermOffboarding.cleanup, the canOffboard[term] mapping will be set to true, allowing the LendingTerm to be offboarded at any time by anyone. This can be done to grief borrowers and gauge voters.

Proof of Concept

When a LendingTerm is offboarded, it's important that it goes through the two step process of offboard and cleanup. This is necessary because cleanup marks canOffboard[term] as false, preventing offboarding of the term without a successful proposal.

// LendingTermOffboarding.cleanup()
canOffboard[term] = false;
// LendingTermOffboarding.offboard()
require(canOffboard[term], "LendingTermOffboarding: quorum not met");

Unfortunately, it's reasonably possible that a term gets re-onboarded without first being cleaned up because:

  1. cleanup can only be executed after all existing loans are called and auctioned
  2. More importantly, onboarding doesn't validate that the term has been cleaned up

As a result, an unclean term could reasonably end up getting re-onboarded, in which case anyone could immediately offboard it at any time, causing loss to term participants.

Tools Used

  • Manual review

Recommended Mitigation Steps

LendingTermOnboarding should validate that the proposed term returns false from canOffboard[term].

Assessed type

Invalid Validation

@c4-bot-8 c4-bot-8 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 Dec 21, 2023
c4-bot-5 added a commit that referenced this issue Dec 21, 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 4, 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 the satisfactory satisfies C4 submission criteria; eligible for awards label Jan 25, 2024
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 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