code-423n4 / 2024-03-abracadabra-money-findings

9 stars 7 forks source link

No Slippage Protection in `buyShares` can lead to Losses for User #197

Closed c4-bot-7 closed 8 months ago

c4-bot-7 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

Loss of funds for users due to slippage during the execution of the buyShares function.

Proof of Concept

The buyShares, is a external accessed function, serves as the entry point for liquidity providers to contribute liquidity and receive LP shares in return.

Here's it's implementation:


      function buyShares(address to) external nonReentrant returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {
        uint256 baseBalance = _BASE_TOKEN_.balanceOf(address(this));
        uint256 quoteBalance = _QUOTE_TOKEN_.balanceOf(address(this));
        uint256 baseReserve = _BASE_RESERVE_;
        uint256 quoteReserve = _QUOTE_RESERVE_;

        baseInput = baseBalance - baseReserve;
        quoteInput = quoteBalance - quoteReserve;

        // SNIP
        if (totalSupply() == 0) {
            // SNIP
        } else if (baseReserve > 0 && quoteReserve > 0) {
            // case 2. normal case
            uint256 baseInputRatio = DecimalMath.divFloor(baseInput, baseReserve);
            uint256 quoteInputRatio = DecimalMath.divFloor(quoteInput, quoteReserve);
            uint256 mintRatio = quoteInputRatio < baseInputRatio ? quoteInputRatio : baseInputRatio; // min of both
@->         shares = DecimalMath.mulFloor(totalSupply(), mintRatio);

            // SNIP
        }

        _mint(to, shares);
        _setReserve(baseBalance, quoteBalance);

        emit BuyShares(to, shares, balanceOf(to));
    }

Now consider the following scenario:

  1. Initial State:

    • Alice wants to provide liquidity to the pool by buying LP shares.
    • The pool currently has a certain reserve of base and quote tokens.
  2. Change of Reserve:

    • Prior to Alice's transaction, anyone calls sellBase or sellQuote which can change the reserve ratio of the pool.
  3. Alice's Transaction:

    • Alice's transaction executes to buy LP shares using the buyShares function.
    • However, due to the changed reserve ratio, the amount of shares Alice receives for providing for liquidity differs significantly because of change in current reserve ratio.

Because of scenarios like this, user will receive fewer shares than anticipated, resulting in losses due to slippage. Therefore it's important to protect users by reverting in case of higher slippage.

Tools Used

VS Code

Recommended Mitigation Steps

Add minShares parameter and validate it in buyShares function itself just like how it is done in Router.

Assessed type

MEV

0xm3rlin commented 8 months ago

should be accessed through router no factor

c4-pre-sort commented 8 months ago

141345 marked the issue as primary issue

141345 commented 8 months ago

seems invalid, the share amount is adjusted in router

c4-pre-sort commented 8 months ago

141345 marked the issue as sufficient quality report

c4-sponsor commented 8 months ago

0xCalibur (sponsor) disputed

c4-judge commented 8 months ago

thereksfour marked the issue as unsatisfactory: Invalid