Abracadabra Mimswap - Breeje's results

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/oracles/aggregators/MagicLpAggregator.sol#L45

Vulnerability details

Impact

A malicious actor can manipulate LP pricing, similar to the exploit witnessed in the Warp Finance Hack.

Proof of Concept

The latestAnswer function in MagicLpAggregator is used to calculate the current price of the LP token for a given pool.

    function latestAnswer() public view override returns (int256) {
        uint256 baseAnswerNomalized = uint256(baseOracle.latestAnswer()) * (10 ** (WAD - baseOracle.decimals()));
        uint256 quoteAnswerNormalized = uint256(quoteOracle.latestAnswer()) * (10 ** (WAD - quoteOracle.decimals()));
        uint256 minAnswer = baseAnswerNomalized < quoteAnswerNormalized ? baseAnswerNomalized : quoteAnswerNormalized;

        (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
        baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
        quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));

@->     return int256(minAnswer * (baseReserve + quoteReserve) / pair.totalSupply());
    }

Calculation Process:

1. Obtain the current price of the base token and normalize it to 18 decimals.
2. Obtain the current price of the quote token and normalize it to 18 decimals.
3. Retrieve the current reserves of base and quote tokens of the pool and normalize them to 18 decimals.
4. Calculate the price using the formula:

minAnswer * (baseReserve + quoteReserve) / pair.totalSupply()

Here, The issue lies in the 3rd point: return value from the _getReserves() function, which is susceptible to manipulation. This function retrieves the spot reserves of the pool. So When a malicious user dumps large amount of any token into the pool, baseReserve + quoteReserve will be significantly increased. Thus, this value should not be utilized for pricing LP tokens due to its volatility caused by trades executed within the pool.

Warp Finance used the formula: r0 * p0 + r1 * p1 to calculate their TVL similar to how MagicLpAggregator contract is using: min(p0, p1). r0 + min(p0, p1). r1, a method that has proven susceptible to exploitation.

Given the ability to alter spot reserves, this vulnerability enables anyone to artificially inflate the price within a single block without incurring any risk, utilizing flashloans. Thus it is very important to mitigate this risk.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use different approach to price the LP in better way as shown here

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/main/src/mimswap/MagicLP.sol#L470

Vulnerability details

Impact

An attacker can steal some funds from the LPs when owner updates parameter like _I_.

Proof of Concept

_I_ is basically the market price provided by an oracle. So updating the value of _I_ directly influences the price.

      function setParameters(
        address assetTo,
        uint256 newLpFeeRate,
        uint256 newI,
        uint256 newK,
        uint256 baseOutAmount,
        uint256 quoteOutAmount,
        uint256 minBaseReserve,
        uint256 minQuoteReserve
    ) public nonReentrant onlyImplementationOwner {
        if (_BASE_RESERVE_ < minBaseReserve || _QUOTE_RESERVE_ < minQuoteReserve) {
            revert ErrReserveAmountNotEnough();
        }
        if (newI == 0 || newI > MAX_I) {
            revert ErrInvalidI();
        }
        if (newK > MAX_K) {
            revert ErrInvalidK();
        }
        if (newLpFeeRate < MIN_LP_FEE_RATE || newLpFeeRate > MAX_LP_FEE_RATE) {
            revert ErrInvalidLPFeeRate();
        }

        _LP_FEE_RATE_ = uint64(newLpFeeRate);
        _K_ = uint64(newK);
        _I_ = uint128(newI);

        _transferBaseOut(assetTo, baseOutAmount);
        _transferQuoteOut(assetTo, quoteOutAmount);
        (uint256 newBaseBalance, uint256 newQuoteBalance) = _resetTargetAndReserve();

        emit ParametersChanged(newLpFeeRate, newI, newK, newBaseBalance, newQuoteBalance);
    }

Now consider the following scenario:

1. Consider the pool for Quote Q and Base B with a initial ideal price I_Old.
2. Assume _K_ negligible for easier calculation, Price of B wrt Q now equals I * Q.
3. Let's assume owner wants to update I_Old to I_New value where I_New > I_Old.
4. Malicious user Alice observes the transaction in the mempool and exploits the parameter change.
5. Alice executes a front-running transaction, purchasing all available supply of the base asset (B).
6. Subsequently, Alice executes a back-run transaction, selling all previously acquired base assets at the higher price set by the parameter update.

In case, the owner's transaction is to decrease _I_, Alice can do the opposite and still end up getting the profit at the expense of Liquidity providers.

Tools Used

VS Code

Recommended Mitigation Steps

Implement a Mechanism such that calls to sellBaseToken and sellQuoteToken should revert whenever pool parameters are changed in that block.

Assessed type

MEV

[L-01] Division before multiplication can result to rounding errors

There are multiple areas where Multiplication is performed on the result of a division in Math.sol contract.

One such example is in _SolveQuadraticFunctionForTrade function:

    uint256 part2 = (((k * V0) / V1) * V0) + (i * delta); // kQ0^2/Q1-i*deltaB

As denoted by the accompanying comment, the intention is to multiply k by the square of V0. However, the current implementation deviates from this intention by initially multiplying k with V0, then dividing the result by V1, followed by another multiplication by V0.

In scenarios where the value of k = 1 and V0 < V1, the initial expression (k * V0) / V1) could be rounded down to zero due to integer division truncation. Consequently, the entire initial expression kQ0^2/Q1 might be rounded down to zero, resulting in a loss of value due to precision.

This discrepancy is noteworthy since k = 1 is a legitimate value that creators might opt to employ.

[L-02] MagicLpAggregator won't support >18 decimal tokens

There are token with more than 18 decimals precision and MagicLpAggregator can't handle it as WAD - decimals expression will always revert.

    function latestAnswer() public view override returns (int256) {
      // SNIP

      (uint256 baseReserve, uint256 quoteReserve) = _getReserves();
@->   baseReserve = baseReserve * (10 ** (WAD - baseDecimals));
@->   quoteReserve = quoteReserve * (10 ** (WAD - quoteDecimals));

      // SNIP
    }