User can directly offboard a term without having reached quorum if a term was re-onboarded before cleanup() is called

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

The protocol has the functionality that during the process of offboarding if a lending term is re-onboarded before the cleanup() function is called, then the transaction should revert. This functionality is flawed, because it does not really prevent the offboarding process from completing and breaks the functionality of the nOffboardingsInProgress storage variable. A user can just call offboard() and then cleanup() again, and the transaction would be successful because the contract storage state has not beed updated. Additionally, if a user was to abuse this, then the nOffboardingsInProgress storage variable would break and would become permanently stricktly larger than zero. This would break the functionality of PSM redemptions being unpaused when there are no offboardings in progress in that gauge type, causing the permanent need of an additonal governance proposal to unpause PSM redemptions every time a lending term is offboarded.

Proof of Concept

In the process of offboarding a term there are several steps. In the LendingTermOffboarding first proposeOffboard() is called, then users vote for the proposal via supportOffboard. Once there are enough votes for the offboarding, the canOffboard[term] variable is set to true in the supportOffboard() function. This storage variable will remain set to true until the complete offboarding process is completed and cleanup() is called.
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOffboarding.sol#L116

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        .
        .
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
        .
        .
    }

Once there are enough votes, the offboard() is called and the loans auctioning can begin. Once all loans are closed, the cleanup() function is closed.

However, there is the caveat that a term can be re-onboarded before cleanup() is called. When this happens the term gauge is marked as no longer deprecated in the GuildToken, the offboarding process must be stopped and the cleanup function must be prevented from being called. In the cleanup() function there is a check if this has happened as we can see below.

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

    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"
        );
        .
        .
    }

However, the canOffboard[term] variable is still set to true, even after the term has been re-onboarded. This means that a user can just call offboard() to reset the GuildToken(guildToken).isDeprecatedGauge(term) to return true and just call cleanup() again. This circuimvents creating a new proposal for offboarding that term by allowing the user to directly offboard a term that has just been reonboarded.

There is the additional side effect of this that breaks the storage variable nOffboardingsInProgress. There are only two ways to increment/decrement this variable.

In offboard() once quorum is reached, we increment this variable by 1:

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

    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);
    }

Then in cleanup() this variable is decremented by 1.

    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);
    }

As we can see this variable is used to track wether redemptions in the SimplePSM are allowed or not. If the variable is 0, then the redemptions are allowed, if it is set to 1 or more, they are paused.

This means, that in the process of offboarding lending terms the offboard() function should be called the exact same number of times as the cleanup() function. To keep this variable at 0 when there are no offboardings running. The above described case would cause the offboard() function being called one more time than the cleanup() function, which would break this invariant and will permanently set the nOffboardingsInProgress storage variable to be stricktly larger than 0.

This would mean that in the consecutive call of cleanup() the PSM redemptions would be set to allowed, even though in practice, there are no offboardings in progress. This variable can in practice still be externally set to true again, but it needs to be done through a separate governing proposal in SimplePSM:

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

    function setRedemptionsPaused(
        bool paused
    ) external onlyCoreRole(CoreRoles.GOVERNOR) {
        redemptionsPaused = paused;
        emit RedemptionsPaused(block.timestamp, paused);
    }

This additional governor proposal action would need to happen every time a term is offboarded in that gauge type is offboarded to keep the functionality of the that market running.

To test this bug copy the below lines of code at the end of the testCannotCleanupAfterReonboard() unit test like this:
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/test/unit/governance/LendingTermOffboarding.t.sol#L358

    function testCannotCleanupAfterReonboard() public {
        .
        .
        // cleanup
        vm.expectRevert("LendingTermOffboarding: re-onboarded");
        offboarder.cleanup(address(term));
        
        offboarder.offboard(address(term)); // @audit new line
        offboarder.cleanup(address(term)); // @audit new line
    }

To run this test run the command: forge test --match-test testCannotCleanupAfterReonboard
The test passes and this example invalidates this test, as it shows that in fact a user can cleanup after reonboard. He just needs to call offboard() before he calls cleanup()

Tools Used

Manual review and unit testing.

Recommended Mitigation Steps

That depends on the intended functionality of the protocol. Re-onboarding should not be possible while offboarding is in progress, or for example calling cleanup() should reset the contract storage state instead of reverting, making it necessary to again follow through the complete process of offboarding, starting from calling proposeOffboard(). There are many ways to approach this bug, but that would depend on how the developers want to handle this functionality.

Assessed type

Governance

[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