BuyShares Slippage Parameters Cannot be Enforced by Return Values

Summary

This issue has 2 parts: First, demonstarting that the BuyShares function is vulnerable to frontrunning attacks. Secondly, showing that if a reasoning that slippage parameters can be set through a router, slippage protection actually won't work because the return values are always the same no matter the slippage of the reserve ratio.

Vulnerability Detail

Buy Shares is Vulnerable to Frontrunning Attacks:

buyShares clearly needs slippage protections as the underlying reserves can be manipulated makes it vulnerable to a frontrunning attack which makes it mint less shares.

One way to do this is if a frontrunner swaps through the pool significantly imbalancing the reserves. Since the caller of buyShares sent tokens in a ratio based on the expected reserve ratio at the time rather than the manipulated ratio, they send an excess of one token and too much of the other, leading to less share minting and fund loss.

This over purchasing of shares is implicitly distributed to stakers. An attacker could execute the following sequence of steps especially on a large buyShares action:

deposit large amount of liquidity buyShares
swap to imbalance pool reserves
victims buyShares trasaction goes through, underminting shares

The extra tokens increases the value of the shares so.

attacker withdraws shares which now yield more tokens

Slippage Protection Doesn't Work:

Let's look at an example of how slippage is implemented in a Dodo router. This is taken from Etherscan live contracts and has a router for Dodo V2. According to discussion with sponsor in the public sherlock channel: "if u need to refer to router and factory contract, please use V2 contracts as references"

    function addDSPLiquidity(
        address dspAddress,
        uint256 baseInAmount,
        uint256 quoteInAmount,
        uint256 baseMinAmount,
        uint256 quoteMinAmount,
        uint8 flag, // 0 - ERC20, 1 - baseInETH, 2 - quoteInETH
        uint256 deadLine
    )

The problem is that baseMinAmount and quoteMinAmount will always be equal to the baseInAmount and quoteInAmount respectively. That is because no matter how much of the tokens sent in that are "used" and no matter what the underlying reserve ratio is used, the return values are baseInput and quoteInput which are just equal the tokens sent to the contract. Therefore when these slippage parameters are "checked", they will always pass.

The core problem isn't in the router itself, but in the return values not reflecting the slippage of the reserves during liquidity deposit. Even if there is large slippage loss causing less liquidity to be deposited, the return values will stay exactly the same as if there was no slippage loss.

Impact

Large frontrunning losses when using buyShares to add liquidity. The slippage based off return values will not work.

Code Snippet

https://github.com/sherlock-audit/2023-12-dodo-gsp/blob/main/dodo-gassaving-pool/contracts/GasSavingPool/impl/GSPFunding.sol#L31C12-L38

Tool used

Manual Review

Recommendation

We can make a slippage parameter where the underlying reserves of the pool are checked, which is similar to the sqrtPriceLimit check in Uniswap v3. Alternatively, the return value calculation could be changed such that a router would actually be able to use the return values to prevent slippage.

Duplicate of #109

sherlock-audit / 2023-12-dodo-gsp-judging

Bandit - BuyShares Slippage Parameters Cannot be Enforced by Return Values #61