Ethereum Credit Guild - NentoR's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L51, https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/tokens/GuildToken.sol#L197-L200

Vulnerability details

Impact

GuildToken assumes that there is only one ProfitManager for all markets deployed, therefore it is initialized with a single ProfitManager on deployment. This is factually incorrect because all markets will get a separate deployment for it. This results in the holder being unable to interact with fundamental gauge-related functions for all markets.

Proof of Concept

GuildToken is implemented in a way that it requires a ProfitManager instance to be linked to it on deployment. Then it uses ProfitManager::claimGaugeRewards() inside _decrementGaugeWeight() and _incrementGaugeWeight(). However, this is a faulty implementation, and results in the user being unable to interact with incrementGauge() and decrementGauge() in all markets. The sponsor said they were refactoring the code because it was previously supposed to be just one market. This is an artifact left from back then that was forgotten.

POC (GuildToken.t.sol):

function testIncrementGaugeWeightFromDifferentProfitManager() public {
  // grant roles to test contract
  vm.startPrank(governor);
  core.createRole(CoreRoles.GUILD_MINTER, CoreRoles.GOVERNOR);
  core.grantRole(CoreRoles.GUILD_MINTER, address(this));
  core.createRole(CoreRoles.GAUGE_ADD, CoreRoles.GOVERNOR);
  core.grantRole(CoreRoles.GAUGE_ADD, address(this));
  core.createRole(CoreRoles.GAUGE_PARAMETERS, CoreRoles.GOVERNOR);
  core.grantRole(CoreRoles.GAUGE_PARAMETERS, address(this));
  core.createRole(CoreRoles.GAUGE_PNL_NOTIFIER, CoreRoles.GOVERNOR);
  core.grantRole(CoreRoles.GAUGE_PNL_NOTIFIER, address(this));
  vm.stopPrank();

  // setup
  token.setMaxGauges(2);
  token.addGauge(1, gauge1);

  // change profitManager
  ProfitManager newProfitManager = new ProfitManager(address(core));
  vm.prank(governor);
  token.setProfitManager(address(newProfitManager));

  // add new gauge after profit manager has been changed
  token.addGauge(1, gauge2);

  // mint alice 1 GUILD
  token.mint(alice, 1e18);

  // confirm gauges' existence
  assert(token.maxGauges() == 2);
  assert(token.isGauge(gauge1) == true);
  assert(token.balanceOf(alice) == 1e18);

  // alice can't increase votes for the gauge
  // ProfitManager::claimGaugeRewards() reverts
  vm.prank(alice);
  vm.expectRevert();
  token.incrementGauge(gauge1, 1e18);

  // alice can't decrease votes for the gauge
  // ProfitManager::claimGaugeRewards() reverts
  vm.prank(alice);
  vm.expectRevert();
  token.decrementGauge(gauge1, 1e18);
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

Refactor it so that GuildToken has access to all markets' profit managers. Maybe it could be done through a mapping where passing the gauge would return the respective profit manager.

Assessed type

Context