Ethereum Credit Guild - whitehat-boys's results

Lines of code

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

Vulnerability details

Impact

if lastGaugeLoss is non-zero for a gauge and if a user trues to stake multiple times after that then slashed will always be true even if userStake.lastGaugeLoss > lastGaugeLoss and is stripped of guildReward when it shouldn't & user is unable to unstake too

Proof of Concept

when a user stakes, getRewards is called and has this comparision if (lastGaugeLoss > uint256(userStake.lastGaugeLoss)) but this comparision is done with the userStake struct defined in memory here which means userStake.lastGaugeLoss will always be 0 irespective of the value in storage

paste the following code in SurplusGuildMinterUnitTest.t.sol & run forge test --mt test_poc_user_does_not_get_gauge_rewards_after_staking_multiple_times_after_gauge_loss_realized -vvvv

function test_poc_user_does_not_get_gauge_rewards_after_staking_multiple_times_after_gauge_loss_realized_and_user_cannot_unstake() public {
        // setup
        credit.mint(address(this), 150e18);
        credit.approve(address(sgm), 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);

        // notify loss in gauge
        profitManager.notifyPnL(term, -27.5e18);

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

        // the guild token earn interests again and now the gauge is in net profit
        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );
        credit.mint(address(profitManager), 25e18);
        profitManager.notifyPnL(term, 25e18);

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

        uint256 _guildBalanceBeforeStake = guild.balanceOf(address(this));
        
        sgm.stake(term, 50e18);

        // next block
        vm.warp(block.timestamp + 13);
        vm.roll(block.number + 1);
        
        sgm.stake(term, 50e18);

        // guild reward + minted guild (values derived by using logging in SurplusGuildMinter.sol)
        uint256 guildReward = 3e20;
        uint256 mintedGuild = 1e20;


        uint256 _guildBalanceAfterStake = guild.balanceOf(address(this));

        // no guild reward even when gauge is in net profit when staking after multiple times
        assertLt(_guildBalanceAfterStake - _guildBalanceBeforeStake, guildReward + mintedGuild); 

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

        uint256 _guildBalanceBeforeUnstake = guild.balanceOf(address(this));

        // unable to unstake
        sgm.unstake(term, 50e18);

        uint256 _guildBalanceAfterUnstake = guild.balanceOf(address(this));
        assertEq(_guildBalanceAfterUnstake - _guildBalanceBeforeUnstake, 0);
    }

Tools Used

foundry

Recommended Mitigation Steps

while setting slashed to true compare lastGaugeLoss with the storage value

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L114 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L114/loan/SurplusGuildMinter.sol#L158 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L114/governance/ProfitManager.sol#L409

Vulnerability details

Summary

GuildToken are provided as first-loss capital to surplus buffer of chosen terms, and in exchange, usees can participate in the gauge voting system at a reduced capital cost & without exposure to guild token's price fluctuations. When borrowers pay interest in the SGM, these rewards go directly in the staker's wallet depending upon the staker's weight in a specific gauge i.e guild rewards are distributed proportionally to guild tokens staked in guage

Any user can front run the distribution of profits i.e ProfitManager.notifyPnL(gauge,amount) whenever positive amount and increase the stake in the gauge to gain more rewards than passive lenders/stakers

vice-versa in case of negative amount a stakers could unstake guildTokens leaving loss to passive lenders/stakers

Impact

One of main purpose of holding guild tokens is add weight to he gauge and earn rewards while a malicious attackers could gain majority of rewards leaving minor rewards to true lenders/stakers of protocol

Proof of Concept

File: src/loan/SurplusGuildMinter.sol
 function stake(address term, uint256 amount) external whenNotPaused {
 ..........................................
        // self-mint GUILD tokens
        uint256 _mintRatio = mintRatio;
        uint256 guildAmount = (_mintRatio * amount) / 1e18;
        RateLimitedMinter(rlgm).mint(address(this), guildAmount);
        GuildToken(guild).incrementGauge(term, guildAmount);
...........................................
    }

https://github.com/volt-protocol/ethereum-credit-guild/blob/main/src/loan/SurplusGuildMinter.sol#L114 Any one can stake/increment weight to a gauge with the guildTokens

  function notifyPnL(
        address gauge,
        int256 amount
    ) external onlyCoreRole(CoreRoles.GAUGE_PNL_NOTIFIER) {
        uint256 _surplusBuffer = surplusBuffer;
        uint256 _termSurplusBuffer = termSurplusBuffer[gauge];
        address _credit = credit;
        .................................
        // handling profit
        else if (amount > 0) {
            ProfitSharingConfig
                memory _profitSharingConfig = profitSharingConfig;

            uint256 amountForSurplusBuffer = (uint256(amount) *
                uint256(_profitSharingConfig.surplusBufferSplit)) / 1e9;

            uint256 amountForGuild = (uint256(amount) *
                uint256(_profitSharingConfig.guildSplit)) / 1e9;

            uint256 amountForOther = (uint256(amount) *
                uint256(_profitSharingConfig.otherSplit)) / 1e9;

            uint256 amountForCredit = uint256(amount) -
                amountForSurplusBuffer -
                amountForGuild -
                amountForOther;

            // distribute to surplus buffer
            if (amountForSurplusBuffer != 0) {
                surplusBuffer = _surplusBuffer + amountForSurplusBuffer;
                emit SurplusBufferUpdate(
                    block.timestamp,
                    _surplusBuffer + amountForSurplusBuffer
                );
            }
            ...............................
            // distribute to lenders
            if (amountForCredit != 0) {
                CreditToken(_credit).distribute(amountForCredit);
            }

            // distribute to the guild
            if (amountForGuild != 0) {
                // update the gauge profit index
                // if the gauge has 0 weight, does not update the profit index, this is unnecessary
                // because the profit index is used to reattribute profit to users voting for the gauge,
                // and if the weigth is 0, there are no users voting for the gauge.
                uint256 _gaugeWeight = uint256(
                    GuildToken(guild).getGaugeWeight(gauge)
                );
                if (_gaugeWeight != 0) {
                    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
                    if (_gaugeProfitIndex == 0) {
                        _gaugeProfitIndex = 1e18;
                    }
                    gaugeProfitIndex[gauge] = 
                        _gaugeProfitIndex +
                        (amountForGuild * 1e18) /
                        _gaugeWeight;
                }//@audit distributing rewards to guild stakers
            }
        }

        emit GaugePnL(gauge, block.timestamp, amount);
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L396 As we can see a portion of rewards is distributed to guild stakers/users who incremented weights for that gauge. This increases gaugeProfitIndex[gauge] which is base of distributing rewards to stakers

Further

   function claimGaugeRewards(
       address user,
       address gauge
   ) public returns (uint256 creditEarned) {
       uint256 _userGaugeWeight = uint256(
           GuildToken(guild).getUserGaugeWeight(user, gauge)
       );
       if (_userGaugeWeight == 0) {
           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);
       }
   }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/ProfitManager.sol#L409 Distribution of rewards

    function unstake(address term, uint256 amount) external {
        // apply pending rewards
        (, UserStake memory userStake, bool slashed) = getRewards(
            msg.sender,
            term
        );

        // if the user has been slashed, there is nothing to do
        if (slashed) return;

        // check that the user is at least staking `amount` CREDIT
        require(
            amount != 0 && userStake.credit >= amount,
            "SurplusGuildMinter: invalid amount"
        );

        // update stake
        uint256 userMintRatio = (uint256(userStake.guild) * 1e18) /
            userStake.credit; /// upcast guild to prevent overflow
        uint256 guildAmount = (userMintRatio * amount) / 1e18;

        if (amount == userStake.credit) guildAmount = userStake.guild;

        userStake.credit -= SafeCastLib.safeCastTo128(amount);
        userStake.guild -= SafeCastLib.safeCastTo128(guildAmount);

        if (userStake.credit == 0) {
            userStake.stakeTime = 0;
            userStake.lastGaugeLoss = 0;
            userStake.profitIndex = 0;
        } else {
            // if not unstaking all, make sure the stake remains
            // greater than the minimum stake
            require(
                userStake.credit >= MIN_STAKE,
                "SurplusGuildMinter: remaining stake below min"
            );
        }
        _stakes[msg.sender][term] = userStake;

        // withdraw & transfer CREDIT
        ProfitManager(profitManager).withdrawFromTermSurplusBuffer(
            term,
            msg.sender,
            amount
        );

        // burn GUILD
        GuildToken(guild).decrementGauge(term, guildAmount);
        RateLimitedMinter(rlgm).replenishBuffer(guildAmount);
        GuildToken(guild).burn(guildAmount);

        // emit event
        emit Unstake(block.timestamp, term, amount);
    }

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/loan/SurplusGuildMinter.sol#L158 Similarily if user hasn't been slashed he can unstake the guild token for being safe from lossess i.e can opt out first before losing value of credit tokens

Tools Used

Manual and Foundruy testing

Recommended Mitigation Steps

it's difficult to propose a solution for this exploit with major changes in contract a simple solution could be

1. take snapshots of user's staked value from some time ago
2. depending upon the weight at the time of last snapshot distribute the rewards so that no one can gain rewards from frontrun

Assessed type

MEV

Judge has assessed an item in Issue #514 as 2 risk. The relevant finding follows:

L-01 Anyone could increase interpolation endTime by 30 days just by distributing 1 wei.

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/ERC20RebaseDistributor.sol#L364

Impact

Anyone can call distribute with 1 Wei as amount and update the interpolation of __rebasingSharePrice & __unmintedRebaseRewards by 30 days

Tools Used

manual review

Recommended Mitigation Steps

Add a minimum amount check to the distribute function