code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/SwingTrader.sol#L147-L166

Vulnerability details

Impact

SwingTraderManager.sellMalt will call SwingTrader.sellMalt to sell the Malt purchased earlier and give the profit to profitDistributor to distribute.

    (uint256 basis, ) = costBasis();

    if (maxAmount > totalMaltBalance) {
      maxAmount = totalMaltBalance;
    }

    malt.safeTransfer(address(dexHandler), maxAmount);
    uint256 rewards = dexHandler.sellMalt(maxAmount, 10000);

    uint256 deployed = deployedCapital; // gas

    if (rewards <= deployed && maxAmount < totalMaltBalance) {
      // If all malt is spent we want to reset deployed capital
      deployedCapital = deployed - rewards;
    } else {
      deployedCapital = 0;
    }

    uint256 profit = _calculateProfit(basis, maxAmount, rewards);

I'll use an example to show how the calculation of profit goes wrong. Consider that previously in SwingTrader.buyMalt, the manager bought 10,000 Malt at $0.9 using 9000 DAI, at which point deployedCapital = 9000.

Now that the price of Malt is back to $1, the manager calls SwingTrader.sellMalt to sell 10,000 Malt.

Scenario 1: The manager sells 10000 Malt directly, the result of costBasis() is 9000/10000 = 0.9 and the profit is 10000*(1-0.9) = $1000.

  function costBasis() public view returns (uint256 cost, uint256 decimals) {
    // Always returns using the decimals of the collateralToken as that is the
    // currency costBasis is calculated in
    decimals = collateralToken.decimals();
    uint256 maltBalance = maltDataLab.maltToRewardDecimals(
      malt.balanceOf(address(this))
    );
    uint256 deployed = deployedCapital; // gas

    if (deployed == 0 || maltBalance == 0) {
      return (0, decimals);
    }

    return ((deployed * (10**decimals)) / maltBalance, decimals);
  }

  function _calculateProfit(
    uint256 costBasis,
    uint256 soldAmount,
    uint256 recieved
  ) internal returns (uint256 profit) {
    if (costBasis == 0) {
      return 0;
    }
    uint256 decimals = collateralToken.decimals();
    uint256 maltDecimals = malt.decimals();
    soldAmount = maltDataLab.maltToRewardDecimals(soldAmount);
    uint256 soldBasis = (costBasis * soldAmount) / (10**decimals);

    require(recieved > soldBasis, "Not profitable trade");
    profit = recieved - soldBasis;
  }

Scenario 2: The manager sells 5000 Malt first, the result of costBasis is 9000/10000 = 0.9 and the profit is 5000 * (1-0.9) = $500. Note that at this point deployedCapital = 9000 - 5000 = 4000.

The manager sells the remaining 5000 Malt, the result of costBasis() is 4000/5000 = 0.8, and the profit is 5000 *(1-0.8) = $1000.

Obviously, Scenario 2 is $500 more profitable than Scenario 1, which is actually due to a mistake in calculating deployedCapital in the first step of Scenario 2.

Since deployedCapital represents the amount SwingTrader spent to buy Malt, in sellMalt, deployedCapital should be subtracted from the amount of DAI that SwingTrader actually received, and the rewards include a portion of profit that does not belong to SwingTrader, so the deployedCapital is small, resulting in too much profit being distributed.

Note: This vulnerability may be out of scope, following Scotch's advice to submit it

Proof of Concept

https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/SwingTraderManager.sol#L236-L245 https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/SwingTrader.sol#L147-L166 https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/SwingTrader.sol#L183-L197 https://github.com/code-423n4/2023-02-malt/blob/700f9b468f9cf8c9c5cffaa1eba1b8dea40503f9/contracts/StabilityPod/SwingTrader.sol#L199-L214

Tools Used

None

Recommended Mitigation Steps

Consider adding profit to the deployedCapital calculation

  function sellMalt(uint256 maxAmount)
    external
    onlyRoleMalt(MANAGER_ROLE, "Must have swing trader manager privs")
    onlyActive
    returns (uint256 amountSold)
  {
    if (maxAmount == 0) {
      return 0;
    }

    uint256 totalMaltBalance = malt.balanceOf(address(this));

    if (totalMaltBalance == 0) {
      return 0;
    }

    (uint256 basis, ) = costBasis();

    if (maxAmount > totalMaltBalance) {
      maxAmount = totalMaltBalance;
    }

    malt.safeTransfer(address(dexHandler), maxAmount);
    uint256 rewards = dexHandler.sellMalt(maxAmount, 10000);

+   uint256 profit = _calculateProfit(basis, maxAmount, rewards);

    uint256 deployed = deployedCapital; // gas

    if (rewards <= deployed && maxAmount < totalMaltBalance) {
      // If all malt is spent we want to reset deployed capital
-     deployedCapital = deployed - rewards;
+     deployedCapital = deployed - rewards + profit;

    } else {
      deployedCapital = 0;
    }

-   uint256 profit = _calculateProfit(basis, maxAmount, rewards);

    _handleProfitDistribution(profit);

    totalProfit += profit;

    emit SellMalt(maxAmount, profit);

    return maxAmount;
  }

c4-sponsor commented 1 year ago

0xScotch marked the issue as sponsor confirmed

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory