Inability to offboard term twice in a 7-day period may lead to bad debt to the market

mediumCode4rena

Lines of code

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

Vulnerability details

Description

In Ethereum Credit Guild, when a LendingTerm is considered too risky, it's GUILD voters are incentivized to offboard that term via the LendingTermOffboarding contract. This mechanism is designed to be a fast and secure way to remove a LendingTerm from a market before it accrues bad debt.

Each time that GUILD voters want to offboard a term, someone will have to call proposeOffboard() in order to start a poll. The other voters will have the ability to support this poll by calling supportOffboard(). As soon as the votes reach to a quorum, the offboard() function will have to be called to remove that term from the market and allow all loans from that term to be called.

The poll for offboarding a term has a max duration of 7 days, this restriction is enforced on proposeOffboard():

solidity📋
    require(
        block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
        "LendingTermOffboarding: poll active"
    );

This check will enforce that nobody can propose the offboarding of a LendingTerm twice in a 7-day period.

Given that the system allows for re-onboarding terms, it's possible that a LendingTerm can be offboarded and re-onboarded in less than 7 days. In such case, if market conditions turn adverse, the GUILD voters won't be able to offboard the same term for a second time, therefore allowing for bad debt to be created and impact all the market.

It's not unfeasible to assume this situation can occur in a future given that the protocol aims for supporting thousands of terms in a market, as it's stated in the docs:

Major lending pools today support at most a few dozen unique loan terms, the Credit Guild is intended to support thousands [...]

Impact

GUILD voters won't be able to offboard the same term twice in a 7-day window period and that will lead to bad debt that will impact all the market and it's voters. In the scenario where loans start to default, all voters for that term will be slashed and the CREDIT token of that market will experience losses.

Proof of Concept

The following coded POC can be pasted in LendingTermOffboarding.t.sol. The test can be run with the command: forge test --match-test testCannotOffboardTwiceIn7Days

solidity📋
function testCannotOffboardTwiceIn7Days() public {
    // Offboard term
    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 and cleanup
    vm.startPrank(alice);
    credit.approve(address(term), debt);
    term.repay(aliceLoanId);
    vm.stopPrank();
    offboarder.cleanup(address(term));

    // After ~5 days @ 13s/block...
    vm.roll(block.number + 33230);
    vm.warp(block.timestamp + 5 days);

    // Re-onboard
    guild.addGauge(1, address(term));

    // After ~1 day...
    vm.roll(block.number + 6646);
    vm.warp(block.timestamp + 1 days);

    // It's not possible to offboard a second time
    vm.expectRevert("LendingTermOffboarding: poll active");
    offboarder.proposeOffboard(address(term));   
}

Tools Used

Manual Review

Recommended Mitigation Steps

It's recommended to modify the check in proposeOffboard() in order to allow a second offboarding of a term if the previous offboarding is already complete.

As a potential solution, consider adding a mapping isPollCompleted[term] that marks true when the quorum is reached for a certain poll on supportOffboard(). This could be the new check on proposeOffboard() to mitigate this issue:

diff📋
    require(
-       block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS,
+       block.number > lastPollBlock[term] + POLL_DURATION_BLOCKS || isPollCompleted[term],
        "LendingTermOffboarding: poll active"
    );

Assessed type

Timing