code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

An implementation of sharesToUnderlying(), 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 sharesToUnderlying() is explicitly intended to allow for an implementation that "may make state modifications":

/**
* @notice Used to convert a number of shares to the equivalent amount of underlying tokens for this strategy.
* @notice In contrast to `sharesToUnderlyingView`, this function **may** make state modifications
* @param amountShares is the amount of shares to calculate its conversion into the underlying token
* @dev Implementation for these functions in particular may vary signifcantly for different strategies
*/
function sharesToUnderlying(uint256 amountShares) public view virtual override returns (uint256) {
    return sharesToUnderlyingView(amountShares);
}

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 sharesToUnderlying() as nonpayable.

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

Recommended Mitigation Steps

Declare sharesToUnderlying() as the default nonpayable.

- function sharesToUnderlying(uint256 amountShares) public view virtual override returns (uint256) {
+ function sharesToUnderlying(uint256 amountShares) public virtual override returns (uint256) {