Ethereum Credit Guild - 0x70C9's results

Lines of code

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

Vulnerability details

Impact

The LendingTermOffboarding contract allows any GUILD holder to poll for the removal of a lending term. When a term is offboarded, all the loans have to be called, and then the term can eventually be cleaned up, which removes the previous privileged roles it had.

The function supportOffboard puts canOffboard[term] to true when quorum gets reached:

        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }

After this, we can call offboard, which increments nOffboardingsInProgress but doesn't change canOffboard because cleanup is still required. Finally, when cleanup gets called, canOffboard is set to false and nOffboardingsInProgress gets decremented.

There is a problem with the supportOffboard function, which connects to the nOffboardingsInProgress variable that accounts the terms needing to cleanup. When a term gets eventually "cleaned up", canOffboard is set to false, as previously mentioned. If a new voter comes in and calls supportOffboard on the same term, the function will not check whether this term has already gone through offboarding or not, and because the quorum has indeed been reached, canOffboard will be set to true once again.

Calling offboard on the same term will not work because this part of the function code will revert the transaction:

        // update protocol config
        // this will revert if the term has already been offboarded
        // through another mean.
        GuildToken(guildToken).removeGauge(term);

However, cleanup can still be called, provided nOffboardingsInProgress is not 0. And importantly, if such variable is not 0, then it means that some other lending term that was waiting for being cleaned up will not be able to do so.

Proof of Concept

Let's do an exploit scenario. Imagine quorum is 1000.

• Alice calls proposeOffboard(term1) at block 5000
  • polls[5000][term1] = 1
  • lastPollBlock[term1] = 5000
• Alice calls proposeOffboard(term2) at block 5000
  • polls[5000][term2] = 1
  • lastPollBlock[term2] = 5000
• Bob calls supportOffboard(5000, term1) with weight 999
  • userPollVotes[Bob][5000][term1] = 999
  • polls[5000][term1] = 1000
  • canOffboard[term1] = true
• Bob calls offboard(term1)
  • term1 gets removed from gauges
  • nOffboardingsInProgress = 1
  • setRedemptionsPaused(true)
• Bob calls cleanup(term1)
  • roles get revoked
  • nOffboardingsInProgress = 0
  • setRedemptionsPaused(false)
  • canOffboard[term1] = false
• Bob calls supportOffboard(5000, term2) with weight 999
  • userPollVotes[Bob][5000][term2] = 999
  • polls[5000][term2] = 1000
  • canOffboard[term2] = true
• Bob calls offboard(term2)
  • term2 gets removed from gauges
  • nOffboardingsInProgress = 1
  • setRedemptionsPaused(true)
• Charlie calls supportOffboard(5000, term1) with weight 1
  • userPollVotes[Charlie][5000][term1] = 1
  • polls[5000][term1] = 1001
  • canOffboard[term1] = true
• Charlie calls cleanup(term1)
  • roles get revoked (they were already but no problem)
  • nOffboardingsInProgress = 0
  • setRedemptionsPaused(false)
  • canOffboard[term1] = false
• Bob calls cleanup(term2)
  • roles get revoked
  • --nOffboardingsInProgress underflows and reverts the call execution.
• Final result: term2 is stuck, cannot be cleanup'd.

Noteworthy, this example is anecdotal. All an attacker actually needs to do to accomplish this is seeing that 2 lending terms are ready for cleanup, and call supportOffboard plus cleanup on one of the terms.

Tools Used

Manual review.

Recommended Mitigation Steps

There are many options to resolve this issue. Here are 2 different ones:

• add a mapping which gets set to true once a term gets cleanedup, and preventing it to be supported again.
• replace nOffboardingsInProgress with a mapping that sets a term to true or false. This will not prevent terms from being supported again and cleaned up, but it will prevent the DoS attack vector, rendering the attack void of impact.

Assessed type

DoS

Judge has assessed an item in Issue #332 as 2 risk. The relevant finding follows:

params.maxDelayBetweenPartialRepay should never be zero

In LendingTerm.partialRepayDelayPassed, the code makes it return false if maxDelayBetweenPartialRepay is set to 0, which stands to reason. However, due to this, that value will prevent anyone from calling an underwater loan, unless the entire gauge gets deprecated, which seems pretty drastic. We recommend never to allow this parameter to be zero, considering how easy it is to get bad debt and require the gauge to be deprecated.