PSM redemptions for the market can get completely bricked`

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L153-L170
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L175-L199
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L239-L261

Vulnerability details

Impact

PSM redemptions for the market can get completely bricked if the term is onboarded and offboarded twice without calling the cleanup, until the governance resumes the redemptions on the PSM. This will make the credit tokens unredeemable until the governance calls the setRedemptionsPaused(false) on the PSM. Also, every offboarding of a term will now require additional attention from governance to unpause the redemptions every time the term is offboarded.

Proof of Concept

• A given term can be onboarded as part of governance.
• Due to a loss or certain market conditions the term can be offboarded and re-onboarded later.
• When the governance offboards a term the nOffboardingsInProgress is increased. The first offboard call in the market will also pause the redemptions for the whole market as seen in the function below.

function offboard(address term) external whenNotPaused {
    require(canOffboard[term], "LendingTermOffboarding: quorum not met");

    // update protocol config
    // this will revert if the term has already been offboarded
    // through another mean.
    GuildToken(guildToken).removeGauge(term);
    // pause psm redemptions
    if (
        nOffboardingsInProgress++ == 0 &&
        !SimplePSM(psm).redemptionsPaused()
    ) {
        SimplePSM(psm).setRedemptionsPaused(true);
    }

    emit Offboard(block.timestamp, term);
}

• After all loans in the term are called and issuance reduced to 0 the cleanup can be called

function cleanup(address term) external whenNotPaused {
    require(canOffboard[term], "LendingTermOffboarding: quorum not met");

    require(
        LendingTerm(term).issuance() == 0,
        "LendingTermOffboarding: not all loans closed"
    );
    require(
        GuildToken(guildToken).isDeprecatedGauge(term),
        "LendingTermOffboarding: re-onboarded"
    );

    // update protocol config
    core().revokeRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, term);
    core().revokeRole(CoreRoles.GAUGE_PNL_NOTIFIER, term);

    // unpause psm redemptions
    if (
        --nOffboardingsInProgress == 0 && SimplePSM(psm).redemptionsPaused()
    ) {
        SimplePSM(psm).setRedemptionsPaused(false);
    }

    canOffboard[term] = false;
    emit Cleanup(block.timestamp, term);
}

• This will reduce the nOffboardingsInProgress, set canOffboard[term] = false and resume redemptions if nOffboardingsInProgress is reduced to 0.

Example

• Now let's look at the following scenario
• Governance onboards term A
• After some time they decide to offboard it and call offboard(term A)
• This sets nOffboardingsInProgress = 1 and canOffboard[term] = true
• The cleanup is not called and the term is re-onboarded again.
• This will pass. Adding a role to an account that already has the roles that want to be assigned is not a problem as it only returns false.
• Now since canOffboard[term] = true anyone can immediately call the offboard(term A) followed by cleanup(term A) and brick the whole PSM until governance intervenes. Why? Let's take a look
• As we offboarded the term twice without cleaning up the nOffboardingsInProgress = 2.
• So when cleanup is called the nOffboardingsInProgress is reduced to 1 and canOffboard[term] set to false
• Since canOffboard[term] = false the cleanup cannot be called again and the nOffboardingsInProgress cannot be reduced from 1 to 0 and can never unpause the redemptions on the PSM unless the governance calls the setRedemptionsPaused(false) on the PSM.
• This creates a DOS for some time as governance actions are under timelock and the credit cannot be redeemed until the redemptions are resumed.
• I think this is a possible scenario that the cleanup forgets to be called as it is not enforced to be. Everybody makes mistakes and in time i think it is bound to happen if the protocol is widely used.

Coded POC

Add this test to LendingTermOffboarding.t.sol file

Run with forge test --match-path ./test/unit/governance/LendingTermOffboarding.t.sol -vvv

function testReonboardWithoutCleanup() public {
    // Close a loan that was opened as a part of a setUp call
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);    

    uint256 debt = term.getLoanDebt(aliceLoanId);

    vm.prank(address(rlcm));
    credit.mint(address(alice), debt);    
    
    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();

    guild.mint(bob, _QUORUM);

    // ------------ OFFBOARD ------------
    vm.startPrank(bob);

    guild.delegate(bob);
    uint256 snapshotBlock = block.number;
    offboarder.proposeOffboard(address(term));

    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    vm.stopPrank();

    uint256 nOffboardings = offboarder.nOffboardingsInProgress();
    assertEq(nOffboardings, 1);
    assertEq(psm.redemptionsPaused(), true);
    
    // Forget to call cleanup
    // Didn't open any loans so we don't need to close 

    // ------------ RE-ONBOARD ------------
    guild.addGauge(1, address(term));

    // Using the offboarder as it has governor privileges
    vm.startPrank(address(offboarder));

    // Roles should be removed while calling cleanup but
    // re-granting roles doens't revert but return false 
    core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term));
    core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term));
    
    vm.stopPrank();

    assertEq(psm.redemptionsPaused(), true);

    uint256 POLL_DURATION_BLOCKS = offboarder.POLL_DURATION_BLOCKS();
    vm.roll(block.number + POLL_DURATION_BLOCKS + 1);

    // ------------ OFFBOARD ------------
    guild.mint(bob, _QUORUM);
    vm.startPrank(bob);

    guild.delegate(bob);
    snapshotBlock = block.number;
    offboarder.proposeOffboard(address(term));

    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    vm.stopPrank();

    nOffboardings = offboarder.nOffboardingsInProgress();
    assertEq(nOffboardings, 2);
    assertEq(psm.redemptionsPaused(), true);
    assertEq(offboarder.canOffboard(address(term)), true);

    offboarder.cleanup(address(term));
    assertEq(offboarder.canOffboard(address(term)), false);

    nOffboardings = offboarder.nOffboardingsInProgress();
    assertEq(nOffboardings, 1);
    assertEq(psm.redemptionsPaused(), true);

    // Using the offboarder as it has governor privileges
    vm.startPrank(address(offboarder));
    psm.setRedemptionsPaused(false);
    // nOffboardings still stuck at 1
    assertEq(nOffboardings, 1);
    assertEq(psm.redemptionsPaused(), false);
}

Tools Used

Manual review

Recommended Mitigation Steps

Ensure that the cleanup has to be called before the term re-onboarding

function proposeOnboard(
    address term
) external whenNotPaused returns (uint256 proposalId) {
    ...
    bool isGauge = GuildToken(guildToken).isGauge(term);
    require(!isGauge, "LendingTermOnboarding: active term");

+   require(!core().hasRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, term), "Term not offboarded properly");
    
    // Generate calldata for the proposal
    (
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    ) = getOnboardProposeArgs(term);

    // propose
    return Governor.propose(targets, values, calldatas, description);
}

Assessed type

Invalid Validation

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1147

[c4-judge]

Trumpero marked the issue as duplicate of #1141

[c4-judge]

Trumpero marked the issue as satisfactory