047.md

Glamorous Canvas Camel

Medium

Mutable Indices in marketConfigs May Lead to Incorrect Market Config Selection

Summary

The removeMarketConfig function in the ReputationMarket contract changes the indices of configurations within the marketConfigs array when a configuration is removed. This is due to the swapping mechanism used before popping the last element. Since functions like createMarketWithConfig rely on indices to specify configurations, changing indices can cause users to inadvertently select incorrect configurations, leading to unexpected market parameters.

Root Cause

The removeMarketConfig function removes a configuration by swapping it with the last element in the marketConfigs array and then calling pop():

https://github.com/sherlock-audit/2024-12-ethos-update/blob/c3a2b007d0ddfcb476f300f8b766808f0e3e2dfd/ethos/packages/contracts/contracts/ReputationMarket.sol#L388C1-L405C4

function removeMarketConfig(uint256 configIndex) public onlyAdmin whenNotPaused {
    // Cannot remove if only one config remains
    if (marketConfigs.length <= 1) revert InvalidMarketConfigOption("Must keep one config");

    // Check if the index is valid
    if (configIndex >= marketConfigs.length) revert InvalidMarketConfigOption("index not found");

    emit MarketConfigRemoved(configIndex, marketConfigs[configIndex]);

    // If this is not the last element, swap with the last element
    uint256 lastIndex = marketConfigs.length - 1;
    if (configIndex != lastIndex) {
        marketConfigs[configIndex] = marketConfigs[lastIndex];
    }

    // Remove the last element
    marketConfigs.pop();
}

This method changes the indices of other configurations in the array. Any subsequent calls to functions that use these indices may inadvertently reference different configurations than intended.

Internal Pre-conditions

No response

External Pre-conditions

No response

Attack Path

1. An admin removes a configuration from the marketConfigs array using removeMarketConfig, causing indices to shift.

2. A user, unaware of the index changes, calls createMarketWithConfig with an index that now points to a different configuration.

   // User intends to use the configuration at index 1
   reputationMarket.createMarketWithConfig(1);

3. The user’s market is created with unintended parameters (e.g., different liquidity or base price), which could affect the market's behavior and the user's funds.

Impact

Markets could be created with higher creation costs or unfavorable liquidity parameters, leading to potential financial loss.

PoC

1. Initial State: Three configurations exist with indices [0, 1, 2].

   // Config at index 0
   MarketConfig({ liquidity: 1000, basePrice: DEFAULT_PRICE, creationCost: 0.2 ether });
   
   // Config at index 1
   MarketConfig({ liquidity: 10000, basePrice: DEFAULT_PRICE, creationCost: 0.5 ether });
   
   // Config at index 2
   MarketConfig({ liquidity: 100000, basePrice: DEFAULT_PRICE, creationCost: 1.0 ether });

2. Admin Removes Config at Index 1:

   reputationMarket.removeMarketConfig(1);

   • The configuration at index 1 is swapped with the last configuration (index 2) and then removed.

   • New indices after removal:

     • Index 0: Unchanged
     • Index 1: Originally at index 2
     • Length of marketConfigs is now 2.

3. User Attempts to Create Market with Original Config at Index 1:

   // User intends to use the 'Deluxe' configuration
   reputationMarket.createMarketWithConfig(1);

   • The user expects to use the configuration with liquidity: 10000, but instead gets liquidity: 100000 due to the index shift.
   • This leads to a higher creation cost and different market dynamics than intended.

---

Mitigation

• Use Unique Identifiers: Assign a unique identifier (e.g., an auto-incrementing ID) to each MarketConfig and store configurations in a mapping. This ensures that each configuration can be referenced consistently, regardless of array index changes.

  mapping(uint256 => MarketConfig) public marketConfigs;
  uint256 public nextConfigId;
  
  function addMarketConfig(...) public {
      marketConfigs[nextConfigId] = MarketConfig(...);
      nextConfigId++;
  }

• Avoid Index-Based References: Modify functions like createMarketWithConfig to accept the unique identifier instead of an index.

  function createMarketWithConfig(uint256 configId) public {
      MarketConfig storage config = marketConfigs[configId];
      require(config.liquidity != 0, "Invalid config");
      // Proceed to create market
  }

• Mark Configurations as Inactive: Instead of removing configurations and shifting indices, mark configurations as inactive (e.g., by adding an active flag). This preserves indices and allows checks for active configurations.

  struct MarketConfig {
      uint256 liquidity;
      uint256 basePrice;
      uint256 creationCost;
      bool active;
  }
  
  function removeMarketConfig(uint256 configIndex) public {
      marketConfigs[configIndex].active = false;
  }