No slippage protection when depositing funds into strategies

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/main/src/contracts/core/StrategyManager.sol#L655-L672

Vulnerability details

Impact

The internal function _depositIntoStrategy is responsible of depositing funds into strategies (except the BeaconETH strategy) when a user calls one of the functions depositIntoStrategy or depositIntoStrategyWithSignature, because _depositIntoStrategy does not implement a slippage protection the user deposit transaction can be front runned and the user will not receive the shares amount that he has expected but he will receive a smaller amount.

Proof of Concept

When a user wants to make a deposit to a given strategy(except the BeaconETH strategy) he must call one of the functions : depositIntoStrategy or depositIntoStrategyWithSignature, which internally call the internal function _depositIntoStrategy to transfer funds and get shares from the strategy contract.

The issue occurs in the _depositIntoStrategy function :

File: StrategyManager.sol Line 655-672

function _depositIntoStrategy(address depositor, IStrategy strategy, IERC20 token, uint256 amount)
    internal
    onlyStrategiesWhitelistedForDeposit(strategy)
    returns (uint256 shares)
{
    // transfer tokens from the sender to the strategy
    token.safeTransferFrom(msg.sender, address(strategy), amount);

    // deposit the assets into the specified strategy and get the equivalent amount of shares in that strategy
    shares = strategy.deposit(token, amount);
    
    // @audit Does not check the returned shares amount

    // add the returned shares to the depositor's existing shares for this strategy
    _addShares(depositor, strategy, shares);

    emit Deposit(depositor, token, strategy, shares);

    return shares;
}

As it can be seen from the code above the function transfer the funds to the strategy contract and then calls the strategy.deposit function which return the correspanding shares amount (it's worth noting that the strategy.deposit function also does not implement a slippage protection, it just checks that the returned share amount is not equal to 0), after getting the shares amount the function uses it directly without checking for eventual slippage, this will put the user who deposits at risk as anyone can front run his transcation by depositing a large amount and thus when the user transactions proceeds he will receive a very small amount of shares not the amount he was expecting.

Because the strategy.deposit function can be different from one strategy to another as it can be overridden, it is important to implement a slippage protection inside the _depositIntoStrategy function to ensure that users are protected when depositing to any strategy.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend to add a slippage protection in the _depositIntoStrategy function, this can be done by adding a minSharesOut argument to the function (provided by the user) and checking if the returned shares amount from the strategy is above it or not. The _depositIntoStrategy function can be modified as follows :

function _depositIntoStrategy(address depositor, IStrategy strategy, IERC20 token, uint256 amount, uint256 minSharesOut)
    internal
    onlyStrategiesWhitelistedForDeposit(strategy)
    returns (uint256 shares)
{
    // transfer tokens from the sender to the strategy
    token.safeTransferFrom(msg.sender, address(strategy), amount);

    // deposit the assets into the specified strategy and get the equivalent amount of shares in that strategy
    shares = strategy.deposit(token, amount);
    
    // @audit Add slippage protection
    require(
        shares >= minSharesOut,
        "StrategyManager.depositIntoStrategy: too high slippage"
    );

    // add the returned shares to the depositor's existing shares for this strategy
    _addShares(depositor, strategy, shares);

    emit Deposit(depositor, token, strategy, shares);

    return shares;
}

Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[Sidu28]

This is good information for Strategy designers to have, but does not appear to have appreciable impact at the moment, so seems more appropriate to have informational severity. We may consider adding an optional parameter in the future.

[c4-sponsor]

Sidu28 marked the issue as disagree with severity

[GalloDaSballo]

I believe, per this line:
https://github.com/code-423n4/2023-04-eigenlayer/blob/398cc428541b91948f717482ec973583c9e76232/src/contracts/strategies/StrategyBase.sol#L88-L101

        /**
         * @notice calculation of newShares *mirrors* `underlyingToShares(amount)`, but is different since the balance of `underlyingToken`
         * has already been increased due to the `strategyManager` transferring tokens to this strategy prior to calling this function
         */
        if (totalShares == 0) {
            newShares = amount;
        } else {
            uint256 priorTokenBalance = _tokenBalance() - amount;
            if (priorTokenBalance == 0) {
                newShares = amount;
            } else {
                newShares = (amount * totalShares) / priorTokenBalance;
            }
        }

That the Strategies are vulnerable to rebase griefs, so the slippage check is necessary

[GalloDaSballo]

With the information I have available:

• Initial Depositor / Cartel of depositors
• Donation (they get this appreciation, being majority holders, no loss)
• Next Depositor receives less shares -> Loses the rounding error of appreciation when compared to other deposits

Which is a valid Medium Severity, callers should use a router to ensure they receive the intended amount of shares

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

All duplicates are related to rebasing shares as a way to attack depositors

Imo it is best to add a router or a minOut parameter as to avoid people getting funds lost, griefed or stolen

Because this has been mitigated somewhat to only cause a loss of yield, (1e9 magnitude less due to min deposit), I believe Medium Severity to be the most appropriate

[d3e4]

The issue described in #343 is not a lack of slippage protection. This is rather the suggested mitigation. It is claimed that the lack of slippage protection “will put the user who deposits at risk as anyone can front run his transcation by depositing a large amount and thus when the user transactions proceeds he will receive a very small amount of shares not the amount he was expecting.”. Depositing doesn’t change the share price, so this is obviously false, but I’ll (generously) assume that the warden meant to frontrun with a direct transfer of funds as the first depositor.
It is not an issue in itself that the depositor receives a smaller amount than expected. It is only nominally smaller, but still worth as much. Except of course due to rounding losses. So this describes a normal inflation attack. The 1e9 minimum shares requirement is an adequate mitigation against this type of inflation attack. Therefore #343 fails to describe any real issue that hasn’t already been mitigated.
A slippage check cannot even protect against this; that would entail receiving more than one should. The closest less than one should receive is precisely the amount lost due to inflation of the rounding loss. To minimise rounding losses one would have to adjust the amount in rather than the amount out.

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as not selected for report