code-423n4 / 2023-12-ethereumcreditguild-findings

17 stars 11 forks source link

Part of gauge profit could be lost and not consumed by any profit "splitter" #201

Closed c4-bot-2 closed 11 months ago

c4-bot-2 commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L383-L401

Vulnerability details

Impact

Users who hold Guild tokens can vote for certain gauges, this voting mechanism increases the weight of those gauges. If a loan was successfully repaid + interest, the lending term/gauge gains some profit from that repayment process, this profit is distributed into multiple splitters, notice profitSharingConfig, one of them is the users who voted for that profited gauge, where each user get a part of that split. This logic is handled in notifyPnL in the ProfitManager contract, however, this function checks if the profited gauge has a non-zero weight (there are some users who voted) before "registering" the profit for them, but if that gauge has 0 weight that split is not handled and is just thrown away, causing it to be lost and unregistered forever. ( Note we confirmed this with the sponsors )

Proof of Concept

function test_PartOfProfitIsLost() public {
    // Initially, the surplus buffer is 0
    assertEq(profitManager.surplusBuffer(), 0);

    // Initially, the gauge has 0 weight and no profit index
    assertEq(profitManager.gaugeProfitIndex(term), 0);
    assertEq(guild.getGaugeWeight(term), 0);

    // The gauge gains a profit of 3e18, the split is 50/50
    // for the surplus buffer and the guild
    vm.prank(governor);
    profitManager.setProfitSharingConfig(0.5e18, 0, 0.5e18, 0, address(0));
    credit.mint(address(profitManager), 3e18);
    profitManager.notifyPnL(term, 3e18);

    // The surplus buffer is 1.5e18
    assertEq(profitManager.surplusBuffer(), 1.5e18);

    // The gauge has 0 weight and no profit index,
    // i.e. 1.5e18 is lost
    assertEq(profitManager.gaugeProfitIndex(term), 0);
    assertEq(guild.getGaugeWeight(term), 0);
}

Tools Used

Manual review + vscode

Recommended Mitigation Steps

Add an else block that donates that split to any other splitter, surplusBuffer for example, something similar to the following:

if (_gaugeWeight != 0) {
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
    if (_gaugeProfitIndex == 0) {
        _gaugeProfitIndex = 1e18;
    }
    gaugeProfitIndex[gauge] =
        _gaugeProfitIndex +
        (amountForGuild * 1e18) /
        _gaugeWeight;
} else {
    surplusBuffer += amountForGuild;
}

Assessed type

Other

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 11 months ago

0xSorryNotSorry marked the issue as duplicate of #1103

c4-judge commented 10 months ago

Trumpero changed the severity to QA (Quality Assurance)

c4-judge commented 10 months ago

Trumpero marked the issue as grade-b

c4-judge commented 10 months ago

Trumpero marked the issue as grade-c