SurplusGuildMinter.getReward() is susceptible to DoS due to unbounded loop

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L239

Vulnerability details

Impact

The SurplusGuildMinter.getReward() function invokes ProfitManager.claimRewards() that in a loop claims reward through all gauges/terms for SurplusGuildMinter as follows.

function claimRewards(
    address user
) external returns (uint256 creditEarned) {
    address[] memory gauges = GuildToken(guild).userGauges(user);
    for (uint256 i = 0; i < gauges.length; ) {
        creditEarned += claimGaugeRewards(user, gauges[i]);
        unchecked {
            ++i;
        }
    }
}

Whereas SurplusGuildMinter works with all gauges, and there is no upper limit for the GuildToken.setMaxGauges(max), the length of loop could be unbounded.

Each call to stake(), unstake(), or getReward() of SurplusGuildMinter will either consume excessive amount of gas, or revert with Out-Of-Gas reason after certain number of gauges/terms were added.

Proof of Concept

1. Alice stakes Credit tokens in SurplusGuildMinter.
2. Some number of terms are added to the protocol.
3. When further Alice tries to call stake(), unstake(), or getReward(), the call reverts due to Out-Of-Gas reason.

// Put inside test/unit/loan/SurplusGuildMinter.t.sol
function test_dos() public {
    address alice = address(789);

    // Number of terms that triggers OOG for stake/unstake/getReward
    uint256 numTerms = 6500;
    address[] memory terms = new address[](numTerms);

    guild.setMaxGauges(numTerms + 1);

    credit.mint(alice, 10e18);

    // Alice stakes Credit tokens
    vm.startPrank(alice);
    credit.approve(address(sgm), 10e18);
    sgm.stake(term, 10e18);
    vm.stopPrank();

    // Create terms
    credit.mint(address(this), 10e18 * numTerms);
    credit.approve(address(sgm), 10e18 * numTerms);
    for (uint256 i; i < numTerms; i++) {
        address _term = address(new MockLendingTerm(address(core)));
        terms[i] = _term;
        guild.addGauge(1, _term); // gaugeType = 1
        sgm.stake(_term, 10e18);
    }

    uint256 gasBefore =  gasleft();

    // Alice tries to call getRewards()
    sgm.getRewards(alice, term);

    uint256 gasAfter =  gasleft();

    uint256 BLOCK_GAS_LIMIT = 30e6;

    // getRewards() consumes more gas than block gas limit of 30Mil
    // reverts with OOG
    require(gasBefore - gasAfter > BLOCK_GAS_LIMIT);
}

Run Poc with the following command.

forge test --mp test/unit/loan/SurplusGuildMinter.t.sol --mt test_dos

Tools Used

Manual review.

Recommended Mitigation Steps

Inside SurplusGuildMinter.getReward(user, term) call

ProfitManager(profitManager).claimRewards(address(this), term)

instead of

ProfitManager(profitManager).claimRewards(address(this))

Since the purpose of SurplusGuildMinter.getReward(user, term) is to update the profit index only for a specific term, so there is no need to update profit indexes across all available terms.

Assessed type

DoS

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1110

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as selected for report

[Trumpero]

I chose this issue to be the primary issue since it has a clear description and contains a PoC.

[0xbtk]

Hey @Trumpero, this issue stem from an admin mistake making low based on c4 severity standardization:

Reckless admin mistakes are invalid.

PS: It is very unlikely that the GAUGE_PARAMETERS role will set max gauges to 6500.

[serial-coder]

Hi @0xbtk,

You misunderstood. The configuration in the deployment script allows the SurplusGuildMinter contract to subscribe to terms over the maximum gauges setting (unlimited!!).

Please refer to my issue for more details: https://github.com/code-423n4/2023-12-ethereumcreditguild-findings/issues/1110.

[stalinMacias]

@serial-coder I would like to understand a little bit more about this. So, the configuration deployment script allows the SurplusGuildMinter to subscribe to an unlimited number of gauges, but, isn't the admin role the one who has the permissions to subscribe a SurplusGuildMinter to terms?

I think @0xbtk has a point here, if the admin is the only one who can subscribe the SurplusGuildMinter to terms, then, subscribing to such an excessing amount of terms would be an admin mistake, isn't it?

[serial-coder]

Hi @stalinMacias,

You might have an incorrect assumption. The protocol lets GUILD community voters vote for onboarding lending terms. Specifically, the GUILD holders can vote to onboard and register lending terms to the SurplusGuildMinter contract through the LendingTermOnboarding::proposeOnboard().

This is not specific to protocol admins. As long as the term receives a sufficient voting quorum, it will be registered to the SurplusGuildMinter contract.

[stalinMacias]

@serial-coder But also, the GUILD community voters have the incentive to vote against onboarding a lending term that is not beneficial for them, right? So, what is the incentive for a "malicious user" or "malicious voter" to max out the number of terms that a SuplusGuildMinter can have? Considering they need to pay gas for all those transactions, plus, they need to put down PeggedTokens so they can earn CreditTokens to eventually earn GUILD tokens as rewards?

[carrotsmuggler]

Looks quite unlikely imo. If we look at the uniswapV2 factory contract, we see there was only ever 1956 pair deployments on a permissionless free-for-all contract. Expecting a semi-permissioned system to hit 6k+ is extremely unrealistic. OOG errors should only be valid if there is a straight-forward way to trigger them. While the impact is high, the likelihood is extremely low, and should be downgraded.

[Trumpero]

I understand that the likelihood of this issue occurring is very low, but there is no evidence to prove that it cannot happen in the future. The PoC test used 6500 terms, but they didn't transfer any rewards. Therefore, the number of terms needed to exceed the block gas limit in a real case is much lower, as transferring tokens will consume a significant amount of gas. The protocol might have a large number of active lending terms across multiple markets in the future, so there is no guarantee that this number is always safe. This vulnerability has a significant impact, which will cause losses to mitigate, so I believe medium severity is appropriate. Additionally, sponsor didn't dispute its severity.