Initial depositor can drastically affect share value

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-eigenlayer/blob/5e4872358cd2bda1936c29f460ece2308af4def6/src/contracts/strategies/StrategyBase.sol#L78-L101

Vulnerability details

Impact

In the StrategyBase contract, an initial depositor can drastically affect how shares account for underlying token value by directly transferring tokens to the contract ahead of making the initial minimum deposit.

This creates an issue when considered in the context with the MIN_NONZERO_TOTAL_SHARES mechanism that is used. The minimum number of shares is meant to reduce the impact of inflation/donation attacks, but as a consequence, it also potentially locks 1e9-1 shares in the contract.

This means that while the number of shares locked is limited, the actual value of those shares is unbounded and can be inflated by an initial depositor, leading to much more locked funds than expected.

Proof of Concept

The deposit function is implemented as follows:

function deposit(IERC20 token, uint256 amount)
    external
    virtual
    override
    onlyWhenNotPaused(PAUSED_DEPOSITS)
    onlyStrategyManager
    returns (uint256 newShares)
{
    require(token == underlyingToken, "StrategyBase.deposit: Can only deposit underlyingToken");

    /**
      * @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;
        }
    }

    // checks to ensure correctness / avoid edge case where share rate can be massively inflated as a 'griefing' sort of attack
    require(newShares != 0, "StrategyBase.deposit: newShares cannot be zero");
    uint256 updatedTotalShares = totalShares + newShares;
    require(
        updatedTotalShares >= MIN_NONZERO_TOTAL_SHARES,
        "StrategyBase.deposit: updated totalShares amount would be nonzero but below MIN_NONZERO_TOTAL_SHARES"
    );

    // update total share amount
    totalShares = updatedTotalShares;
    return newShares;
}

The way that this is implemented, an initial depositor can make a minimum deposit of 1e9 tokens, which will result in 1e9 shares being minted, and the updatedTotalShares >= MIN_NONZERO_TOTAL_SHARES check being passed.

However, the initial depositor can then transfer an arbitrary amount of tokens to the contract, which will increase the value of the 1e9 shares that they hold. For future depositors, as their tokens are calculated, the priorTokenBalance will be much higher while the totalShares will remain the same.

A byproduct of this is that the 1e9 shares that are potentially locked in the contract now represents an outsized value. This can be an extreme amount depending on the quantity of tokens transferred by the initial depositor.

This can be used maliciously in a few scenarios. One, for example, is that the strategy owner can drastically inflate the value of shares, ensuring that their strategy is always substantially funded, they can withdraw their shares as others deposit, recovering their funds over time.

Tools Used

Manual review

Recommended Mitigation Steps

Use a different mechanism, such as virtual shares, to mitigate inflation/donation attacks.

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #193

[c4-judge]

GalloDaSballo marked the issue as duplicate of #343

[d3e4]

This is invalid for the same reasons as #110. 1e9 shares can be inflated to represent something on the order of 1e13 tokens. This is still a very small amount, which is about as acceptable to have remaining in the contract as 1e9. 1e9 was choses because it is many orders of magnitude greater than 1 but also equally many orders of magnitude smaller than 1e18.

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[c4-judge]

GalloDaSballo marked the issue as grade-a