c4-bot-7 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Router.sol#L271 https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/periphery/Router.sol#L287 https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/mimswap/MagicLP.sol#L424

Vulnerability details

Impact

This vulnerability in the router contract enables users to withdraw more liquidity than their own total actual share, potentially leading to financial losses and compromising the system's integrity. Malicious actors will exploit this loophole to manipulate liquidity pools and disrupt market stability as they will get more 'baseAmount' and 'quoteAmount' than they would have gotten with their actual shareAmount balance.

Proof of Concept

Inside removeLiquidity function of Router contract, the router.sol contract gets set as msg.sender when router contract calls IMagicLP(lp).sellShares(sharesIn, to, minimumBaseAmount, minimumQuoteAmount, "", deadline);

So in contract MagicLP.sol in function sellShares when it checks the condition if (shareAmount > balanceOf(msg.sender)) { revert ErrNotEnough(); } at that time it checks if shareAmount passed is bigger than the balance of msg.sender ie in this situation balance of router contract.

But here protocol needs to check if the original msg.sender of removeLiquidity function in Router.sol contract is having the actual share amount that he is passing in function shareAmount. If not then it will revert!

Proof Of Concept - Here's an exploit scenario involving two users, Alice and Bob

Setup:
Alice has 100 LP tokens in MagicLP.
Bob has 50 LP tokens in MagicLP.
The router contract currently holds 200 LP tokens (from previous transactions).

Exploit:
Alice calls removeLiquidity on the router contract, requesting to remove 150 LP tokens, which is more than her balance but less than the router's balance.

The router contract calls safeTransferFrom, transferring 150 LP tokens from Alice to itself. This succeeds because the router has enough LP tokens.

The router contract then calls sellShares on the MagicLP contract with 150 LP tokens as sharesIn.

The sellShares function checks the router's balance, which is sufficient, and proceeds with the transaction.

Alice receives assets for 150 LP tokens, despite only owning 100 LP tokens.

Outcome:
Alice has exploited the system to withdraw more assets than her share of the liquidity pool.

Bob and other liquidity providers are affected because the pool's asset balance is reduced by more than Alice's actual share, potentially diluting their remaining shares.

Tools Used

Manual Review

Recommended Mitigation Steps

To prevent this exploit, the router contract's removeLiquidity function must ensure that the sharesIn amount does not exceed the caller's actual LP token balance before performing the safeTransferFrom operation.