A malicious actor can easily front-run `proposeOnboard` and cancel an unfavorable term

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

Any proposal on LendingTermOnboarding.sol can be maliciously front-run and canceled by the attacker, such that the unfavorable proposal will not be voted on. And the attack can be done repetitively.

Proof of Concept

In src/governance/LendingTermOnboarding.sol, there are two vulnerabilities. (1) createTerm() doesn’t associate or store the creator of a term in any ways, each created term is essentially anonymously created; (2) proposeOnboard() will not create the proposal description with the creator or proposer address, which voids open zeppelin’s _isValidDescriptionForProposer() front-run protection;

The two vulnerabilities in combination allows an attacker who views the proposal / term to be unfavorable to front run proposeOnboard() and cancel the proposal, such that the unfavorable proposal will not be voted on.

//src/governance/LendingTermOnboarding.sol
    function createTerm(
        address implementation,
        LendingTerm.LendingTermParams calldata params
    ) external returns (address) {
…
//@audit note: There are no creator or proposer associated with a term during the createTerm() process
        LendingTerm(term).initialize(
            address(core()),
            LendingTerm.LendingTermReferences({
                profitManager: profitManager,
                guildToken: guildToken,
                auctionHouse: auctionHouse,
                creditMinter: creditMinter,
                creditToken: creditToken
            }),
            params
        );
…}

(https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L154-L163)

//src/governance/LendingTermOnboarding.sol
    function proposeOnboard(
        address term
    ) external whenNotPaused returns (uint256 proposalId) {
…
       // Generate calldata for the proposal
        (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        ) = getOnboardProposeArgs(term);
...

(https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L203-L208)

//src/governance/LendingTermOnboarding.sol
    function getOnboardProposeArgs(
        address term
    )
        public
        view
        returns (
            address[] memory targets,
            uint256[] memory values,
            bytes[] memory calldatas,
            string memory description
        )
    {
…
//@audit There is no proposer or creator address used when generating proposal, which will void openzeppelin’s Governor.sol `propose()`’s `_isValidDescriptionForProposer
` frontrun control
 |>       description = string.concat(
            "[",
            Strings.toString(block.number),
            "]",
            " Enable term ",
            Strings.toHexString(term)
        );

(https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/LendingTermOnboarding.sol#L230-L235)

//openzeppelin/Governor.sol
    function propose(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        string memory description
    ) public virtual override returns (uint256) {
        address proposer = _msgSender();
        require(_isValidDescriptionForProposer(proposer, description), "Governor: proposer restricted");
…
}
    function _isValidDescriptionForProposer(
        address proposer,
        string memory description
    ) internal view virtual returns (bool) {
…
//@audit-info note:  LendingTermOnboarding.sol doesn’t include `#proposer=0x` suffix with proposer address in proposal description.
        // If the marker is not found, there is no proposer suffix to check
 |>      if (marker != bytes12("#proposer=0x")) {
            return true;
        }

POC: Step1 : An attacker front run creator proposeOnboard() and propose the unfavorable proposal themselves.

The transaction will succeed since createTerm() and proposeOnboard() are anonymous to the proposer. And _isValidDescriptionForProposer() is voided in this case.

Step2: After transaction succeeds, the attacker will simply cancel the proposal. And now no one can propose until 7 days later. After MIN_DELAY passed, the attacker can perform the same attack if anyone attempts to propose the term or any term.

//openzeppelin’s Governor.sol
    function cancel(
        address[] memory targets,
        uint256[] memory values,
        bytes[] memory calldatas,
        bytes32 descriptionHash
    ) public virtual override returns (uint256) {
        uint256 proposalId = hashProposal(targets, values, calldatas, descriptionHash);
        require(state(proposalId) == ProposalState.Pending, "Governor: too late to cancel");
//@audit-info: note: In Step2, cancel() will allow the attacker to cancel the unfavorable term because the attacker is the proposer.
 |>    require(_msgSender() == _proposals[proposalId].proposer, "Governor: only proposer can cancel");
        return _cancel(targets, values, calldatas, descriptionHash);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommendations: (1) Considering in src/governance/LendingTermOnboarding.sol, in createTerm() add msg.sender(term proposer) to created lending term contract. Allow view queries to the term proposer address;

(2) In proposeOnboard(), call the lending term contract for the proposer address to be added to proposal description in the format of #proposer=0x This will enable front run protection in Governor.sol

Assessed type

Other

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

[c4-judge]

Trumpero marked the issue as grade-c