Attacker can enable redemptions during Off-Boarding allowing users to redeem peg tokens

[c4-bot-6]

Lines of code

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

Vulnerability details

According to Docs: -

"Since liquidation occurs by auction, it will still set a market price instead of allowing some users to redeem above peg after a loss has occurred. During a LendingTerm offboarding (while auctions of the collateral of a term are running), redemptions in the PSM are paused."

The LendingTermOffboarding contract allows any GUILD holder to poll for the removal of a lending term, and if enough GUILD holders vote for a removal poll, the term can be offboarded without delay.
After the proposal to off board a lending term reaches the desired quorum, the offboard() is called to remove that lending term & pause redemptions in the SimplePSM contract. After all the loans for that lending term have been settled, the cleanup() is called to revoke the minter & pnl notifier role.
If all the lending terms have been off boarded from the protocol, the redemption are started/unpaused in the SimplePSM contract.
Otherwise the only other way to enable/disable redemptions is through the Governor role.

However it is easy for an attacker to enable the redeem functionality allowing users to transfer the peg tokens.

Proof of Concept

nOffboardingsInProgress keeps track of the number of off boardings currently happening.

File: LendingTermOffboarding.sol

    /// @notice number of offboardings in progress.
    uint256 public nOffboardingsInProgress;

After the quorum has been reached, offboard() is called which removes the lending term & pauses the redemptions.

File: LendingTermOffboarding.sol

     function offboard(address term) external whenNotPaused {
       ...

        // pause psm redemptions
        if (
            nOffboardingsInProgress++ == 0 &&                      @note
            !SimplePSM(psm).redemptionsPaused()
        ) {
            SimplePSM(psm).setRedemptionsPaused(true);
        }
        ...
    }    

If there are no off-boardings left in the protocol, the redemptions are unpaused as can be seen inside the if statement & canOffboard is set to false for that lending term.

File: LendingTermOffboarding.sol

     function cleanup(address term) external whenNotPaused {
     ...

        // unpause psm redemptions
        if (
            --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
        ) {
            SimplePSM(psm).setRedemptionsPaused(false);        @note - unpaused if no off boardings in progress
        }
        canOffboard[term] = false;
    }    

If redemptions are paused, then users cannot redeem their peg tokens.

File: SimplePSM.sol

    /// @notice set `redemptionsPaused`
    /// governor-only, to allow full governance to update the psm mechanisms,
    /// or automated processes to pause redemptions under certain conditions.
    function setRedemptionsPaused(
        bool paused
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        redemptionsPaused = paused;
        emit RedemptionsPaused(block.timestamp, paused);
    }

Attack Scenario:

• Currently 2 off boardings are in progress (say term X & term Y).
• Both have reached quorum & offboard() has been called for both X & Y pausing redemptions in psm. This sets nOffboardingsInProgress = 2
• All loans & issuance have been cleared for X within the 7 day time period (snapshotBlock + POLL_DURATION_BLOCKS) & cleanup() is called setting nOffboardingsInProgress = 1. Redemptions are still paused.
• At this time the Attacker (who has some guild tokens) calls supportOffboard() with lending term X as parameter which passes because the duration to veto for X hasn't ended yet.
• All the checks pass & canOffboard[term] is again set to true because the quorum already been reached previously.
• Attacker now calls cleanup() which passes all the checks & .revokeRole() would just return false in this case.
• The if condition is executed because now --nOffboardingsInProgress == 0 & which ultimately sets redemptions to false (unpause).
• The off boarding of lending term Y is still in progress at this time whose loans haven't been cleared yet.
• Attacker & all other users can withdraw the peg tokens from SimplePSM contract.

Another situation would be if 10 off boarding are in progress & any 1 of them successfully calls cleanup() within the POLL_DURATION_BLOCKS (7 day) period, other who haven't voted for that term can call supportOffboard() followed by cleanup() to unpause redemptions.

Impact

According to docs:

"The SimplePSM targets a value equal to ProfitManager.creditMultiplier(), so when bad debt is created and all loans are marked up, they stay the same in terms of peg token, because new CREDIT can be minted with fewer peg tokens from the PSM."Conversely, when new loans are issued, if there are funds available in the SimplePSM, borrowers know the amount of peg tokens they'll be able to redeem their borrowed CREDIT for.

The above attack breaks this core functionality. Users are able to redeem their peg tokens phase thereby decreasing the supply.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in supportOffboard() to ensure the lending term being voted on exists in the gauge. This way if the attacker called to veto the already removed term, the function would revert.

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1141

[c4-judge]

Trumpero changed the severity to 2 (Med Risk)

[c4-judge]

Trumpero marked the issue as satisfactory

[Trumpero]

dup of #1141 due to the same root cause: supportOffboard can still be called after offboarding.

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero marked the issue as duplicate of #1141