Dilution of Initial Liquidity Providers Shares in MagicLP::buyShares Disincentivizes other LPs

[c4-bot-6]

Lines of code

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

Vulnerability details

Summary

The MagicLP contract implements a unique mechanism where, upon the first provision of liquidity, it mints 1001 shares to the address(0) and subsequently reduces the shares allocated to the initial liquidity provider by the same amount. While this appears to serve as a safeguard to ensure a minimal non-removable liquidity base, it inadvertently dilutes the initial liquidity provider's ownership in the liquidity pool.

Details

Upon the initial provision of liquidity (when totalSupply() == 0), the contract calculates the shares based on the provided base and quote token amounts. Instead of allocating all calculated shares to the liquidity provider, it mints 1001 shares to address(0) and reduces the provider's shares by the same amount.

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L375C9-L394C28

if (totalSupply() == 0) {
    // Initial liquidity provision logic
    shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance;
    _BASE_TARGET_ = shares.toUint112();
    _QUOTE_TARGET_ = DecimalMath.mulFloor(shares, _I_).toUint112();

    if (shares <= 2001) {
        revert ErrMintAmountNotEnough();
    }

    _mint(address(0), 1001); // Minting to address(0)
    shares -= 1001; // Reducing initial provider's shares
}

Requiring that a portion of the liquidity cannot be removed might make certain manipulative strategies less feasible or attractive by ensuring that the pool always maintains a basic level of operation, regardless of external attempts to manipulate its liquidity or price. This attack vector seems to be what warrants this feature but there are better ways to deal with such edge cases

Impact

The initial liquidity provider receives fewer shares than calculated for their liquidity contribution. This dilution reduces their proportionate ownership in the pool and their share of future fee earnings.

Steps to Reproduce

1. Deploy the MagicLP contract and initialize it with base and quote tokens.
2. Call buyShares(address to) as the first liquidity provider and provide base and quote tokens.
3. Observe that the shares minted to to are 1001 less than the expected amount based on the provided liquidity, with 1001 shares minted to address(0).

Poc

# Example Variables
baseBalance = 10_000  # Example base token amount provided by the initial liquidity provider
quoteBalance = 5_000  # Example quote token amount provided by the initial liquidity provider
_I_ = 0.5  # Example price ratio

def original_issue_simulation(baseBalance, quoteBalance, _I_):
    initial_shares = min(quoteBalance / _I_, baseBalance)
    minted_to_zero_address = 1001

    if initial_shares <= 2001:
        return "Error: Mint amount not enough", 0

    final_shares_provider = initial_shares - minted_to_zero_address
    return initial_shares, final_shares_provider

def proposed_workaround_simulation(baseBalance, quoteBalance, _I_):
    minimalBaseLiquidity = 1001
    initial_shares_including_safeguard = min(quoteBalance / _I_, baseBalance) + minimalBaseLiquidity
    final_shares_provider = initial_shares_including_safeguard

    return initial_shares_including_safeguard, final_shares_provider

original_initial_shares, original_final_shares_provider = original_issue_simulation(baseBalance, quoteBalance, _I_)
workaround_initial_shares_including_safeguard, workaround_final_shares_provider = proposed_workaround_simulation(baseBalance, quoteBalance, _I_)

print("Original Code:")
print(f"Initial Shares: {original_initial_shares}")
print(f"Final Shares for Initial Liquidity Provider: {original_final_shares_provider}")

print("\nMitigated Code:")
print(f"Initial Shares (Including Safeguard): {workaround_initial_shares_including_safeguard}")
print(f"Final Shares for Initial Liquidity Provider: {workaround_final_shares_provider}")

Output

Original Code:
Initial Shares: 10000.0
Final Shares for Initial Liquidity Provider: 8999.0

Mitigated Code:
Initial Shares (Including Safeguard): 11001.0
Final Shares for Initial Liquidity Provider: 11001.0

Mitigation

Create a separate mechanism that explicitly locks a minimal amount of liquidity in a transparent and fair manner. This approach can avoid the direct dilution of the initial provider's shares while still providing the pool with a safeguard against rapid liquidity withdrawal or manipulation.

Proposed code changes:

contract MagicLPEnhanced is MagicLP {
    uint256 public constant MINIMUM_LOCKED_LIQUIDITY_AMOUNT = 1001;
    uint256 public lockedLiquidityBase;
    uint256 public lockedLiquidityQuote;
    // Optionally, include a lock expiry timestamp or other conditions for unlocking

    // Override the buyShares function to include locked liquidity logic
    function buyShares(address to) external nonReentrant returns (uint256 shares, uint256 baseInput, uint256 quoteInput) {
        // Original buyShares logic up to the calculation of shares
        ...

        if (totalSupply() == 0) {
            // In the initial supply case, instead of minting to address(0), lock a portion of liquidity
            require(quoteBalance >= MINIMUM_LOCKED_LIQUIDITY_AMOUNT, "Insufficient initial liquidity");

            // Calculate shares normally without subtracting 1001
            shares = quoteBalance < DecimalMath.mulFloor(baseBalance, _I_) ? DecimalMath.divFloor(quoteBalance, _I_) : baseBalance;

            // Lock a portion of the liquidity instead of minting it to address(0)
            lockedLiquidityBase = (shares <= baseBalance ? shares : baseBalance) * MINIMUM_LOCKED_LIQUIDITY_RATIO / 1e18;
            lockedLiquidityQuote = DecimalMath.mulFloor(lockedLiquidityBase, _I_);

            shares -= DecimalMath.divFloor(lockedLiquidityQuote, _I_); // Adjust shares for locked liquidity
        } else {
            // Normal case logic remains the same
            ...
        }

        _mint(to, shares);
        _setReserve(baseBalance, quoteBalance);
        emit BuyShares(to, shares, balanceOf(to));
    }
}

Explanation:

1. Introduce lockedLiquidityBase and lockedLiquidityQuote to track the amount of base and quote tokens that are locked and cannot be immediately removed from the pool. This approach provides clarity and transparency on how much liquidity is permanently in the pool.

2. During the first liquidity provision, instead of minting and subtracting shares, calculate the amount of base and quote tokens to be locked based on the initial liquidity. This ensures the pool has a minimal operational buffer without reducing the initial provider's shares.

3. When calculating shares for the initial provider, adjust the calculation to account for the locked liquidity, ensuring the provider's shares represent their actual contribution minus the locked portion.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[141345]

dilute initial lp

seems expected behavior, and the loss is not significant

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[0xCalibur]

expected behavior, it's a fork of dodo v2 and it's working like this

[c4-sponsor]

0xCalibur (sponsor) disputed

[thereksfour]

Invalid, the first staker always owns 100% of the shares

This is the solution to the inflation attack, which is also used in Uniswap v2.

• Implement or recommend mitigations for ERC4626 inflation attacks OpenZeppelin/openzeppelin-contracts#3706 (comment)
• https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair.sol#L121

And still invalid, this will just have dust loss, which is acceptable for solving inflation attacks

[c4-judge]

thereksfour marked the issue as unsatisfactory:
Invalid

[anthonyshiks]

Hi I believe this is valid issue

[anthonyshiks]

Nevermind I thought pjqa was open.. I'll wait to prove submission then