code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

An implementation of underlyingToShares(), as inherited from StrategyBase.sol, cannot (contrary to intentions) make state modifications. This implies that StrategyBase.sol may become useless as a base contract to inherit from.

Proof of Concept

StrategyBase.sol "is designed to be inherited by more complex strategies, which can then override its functions as necessary". Its function underlyingToShares() is explicitly intended to allow for an implementation that "may make state modifications":

/**
* @notice Used to convert an amount of underlying tokens to the equivalent amount of shares in this strategy.
* @notice In contrast to `underlyingToSharesView`, this function **may** make state modifications
* @param amountUnderlying is the amount of `underlyingToken` to calculate its conversion into strategy shares
* @dev Implementation for these functions in particular may vary signifcantly for different strategies
*/
function underlyingToShares(uint256 amountUnderlying) external view virtual returns (uint256) {
    return underlyingToSharesView(amountUnderlying);
}

However, it is declared as view. Overriding functions can only restrict state mutability, never expand it. This means that any overriding function can only be view or pure, and therefore cannot make state changes. Note that IStrategy correctly declares underlyingToShares() as nonpayable.

There is a similar issue with sharesToUnderlying(), and probably also with explanation(), both reported separately.

Recommended Mitigation Steps

Declare underlyingToShares() as the default nonpayable.

- function underlyingToShares(uint256 amountUnderlying) external view virtual returns (uint256) {
+ function underlyingToShares(uint256 amountUnderlying) external virtual returns (uint256) {