`SwingTraderManager.addSwingTrader()` shouldn't push the `traderId` to `activeTraders` array if `active = false`.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/main/contracts/StabilityPod/SwingTraderManager.sol#L415

Vulnerability details

Impact

After adding an inactive trader using addSwingTrader(), activeTraders array will contain an inactive trader.

Furthermore, if the inactive trader is toggled to active using toggleTraderActive(), activeTraders array will contain the trader twice and the main functions like buyMalt() and sellMalt() will work wrongly as they assume activeTraders contains traders only once.

Proof of Concept

addSwingTrader() adds new traders with a custom active state.

  function addSwingTrader(
    uint256 traderId,
    address _swingTrader,
    bool active,
    string calldata name
  ) external onlyRoleMalt(ADMIN_ROLE, "Must have admin privs") {
    SwingTraderData storage trader = swingTraders[traderId];
    require(traderId > 2 && trader.id == 0, "TraderId already used");
    require(_swingTrader != address(0), "addr(0)");

    swingTraders[traderId] = SwingTraderData({
      id: traderId,
      index: activeTraders.length,
      traderContract: _swingTrader,
      name: name,
      active: active
    });

    activeTraders.push(traderId); //@audit shouldn't add tradeId when active = false

    emit AddSwingTrader(traderId, name, active, _swingTrader);
  }

So traderId will be added to activeTraders array if active = false.

After noticing the trader is inactive, toggleTraderActive() might be called to activate it.

  function toggleTraderActive(uint256 traderId)
    external
    onlyRoleMalt(ADMIN_ROLE, "Must have admin privs")
  {
    SwingTraderData storage trader = swingTraders[traderId];
    require(trader.id == traderId, "Unknown trader");

    bool active = !trader.active;
    trader.active = active;

    if (active) {
      // setting it to active so add to activeTraders
      trader.index = activeTraders.length;
      activeTraders.push(traderId);
    } else {
      // Becoming inactive so remove from activePools
      uint256 index = trader.index;
      uint256 lastTrader = activeTraders[activeTraders.length - 1];

      activeTraders[index] = lastTrader;
      activeTraders.pop();

      swingTraders[lastTrader].index = index;
      trader.index = 0;
    }

    emit ToggleTraderActive(traderId, active);
  }

Then it will become worse because the same traderId will be added to activeTraders again as active = true.

As a result, activeTraders will contain the same trader twice and it will bring serious problems by using the same trader twice in several functions like buyMalt() and sellMalt().

Tools Used

Manual Review

Recommended Mitigation Steps

We shouldn't add traderId to activeTrader when active = false.

  function addSwingTrader(
    uint256 traderId,
    address _swingTrader,
    bool active,
    string calldata name
  ) external onlyRoleMalt(ADMIN_ROLE, "Must have admin privs") {
    SwingTraderData storage trader = swingTraders[traderId];
    require(traderId > 2 && trader.id == 0, "TraderId already used");
    require(_swingTrader != address(0), "addr(0)");

    swingTraders[traderId] = SwingTraderData({
      id: traderId,
      index: activeTraders.length,
      traderContract: _swingTrader,
      name: name,
      active: active
    });

    if(active) {
        activeTraders.push(traderId);
    }

    emit AddSwingTrader(traderId, name, active, _swingTrader);
  }

[c4-judge]

Picodes marked the issue as duplicate of #12

[c4-sponsor]

0xScotch marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)