The term can be re-onboarded using a not allowed implementation

[c4-bot-3]

Lines of code

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

Vulnerability details

Lines of code

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

Impact

The term can be re-onboarded using the LendingTermOnboarding::proposeOnboard() function, the problem is that the term may be using a implementation which was removed by the LendingTermOnboarding::allowImplementation()

Proof of Concept

Please see the next scenario:

1. A termA is created using the implementation=address(123).
2. Time goes by 1 year and the termA is removed using the LendingTermOffboarding.sol contract.
3. The team found a bug in the implementation=address(123) an the implementation is removed using the LendingTermOnboarding::allowImplementation(address(123), false) function
4. Time goes by another 6 months an the termA, which is using the implementation=address(123), is re-onboarded using LendingTermOnboarding::proposeOnboard() function. That may be a problem because the active termA may be using an implementation (address(123)) which is not allowed.

Tools used

Manual review

Recommended Mitigation Steps

The term proposed using the LendingTermOnboarding::proposeOnboard() function should be a recent created[term]

    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");
++      require(created[term] + MIN_LIVE > block.timestamp);
        // Check that the term was not subject to an onboard vote recently
        require(
            lastProposal[term] + MIN_DELAY_BETWEEN_PROPOSALS < block.timestamp,
            "LendingTermOnboarding: recently proposed"
        );
        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);
    }

Assessed type

Context

[0xSorryNotSorry]

While my take is the mitigation might not be a solution for this, it seems that a require statement is not existing as referred in the submission.

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[eswak]

Acknowledging this, but think this is more fit for QA. The governance can check if a term proposed to be onboarded is using a deprecated implementation or not, and vote in consequence. It's worth doing the fix directly in the code because it's a bit inconsistent with the "first onboarding" flow.

[c4-sponsor]

eswak (sponsor) acknowledged

[c4-sponsor]

eswak marked the issue as disagree with severity

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a