`StrategyBase.explanation()` cannot be overridden to intended mutability

[code423n4]

Lines of code

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

Vulnerability details

Impact

An implementation of explanation(), as inherited from StrategyBase.sol, cannot (possibly contrary to intentions) make state modifications. This implies that StrategyBase.sol may become useless as the intended 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 explanation() is declared as pure:

/**
* @notice Currently returns a brief string explaining the strategy's goal & purpose, but for more complex
* strategies, may be a link to metadata that explains in more detail.
*/
function explanation() external pure virtual override returns (string memory) {
    return "Base Strategy implementation to inherit from for more complex implementations";
}

This means that any inheriting contract overriding this function also must be pure. However, an implementation might need a mutability of at least view. This is suggested by it's being declared view in IStrategy.sol. For example, the explanation of the strategy might want to incorporate the value of some variable in the strategy, rather than just being an immutable string.

There is a similar issue with sharesToUnderlying() and underlyingToShares(), both reported separately.

Recommended Mitigation Steps

Declare explanation() as the default nonpayable.

- function explanation() external pure virtual override returns (string memory) {
+ function explanation() external view virtual override returns (string memory) {

Assessed type

Context

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

Sidu28 marked the issue as sponsor disputed

[Sidu28]

This was already reported in the Consensys Diligence audit that is linked in the contest's README: https://consensys.net/diligence/audits/2023/03/eigenlabs-eigenlayer/#strategybase--inheritance-related-issues

[GalloDaSballo]

Would be a Refactoring at best, closing as Known

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope

[d3e4]

This was already reported in the Consensys Diligence audit that is linked in the contest's README: https://consensys.net/diligence/audits/2023/03/eigenlabs-eigenlayer/#strategybase--inheritance-related-issues

explanation() was not reported in the Consensys Diligence audit; only sharesToUnderlying() and underlyingToShares() were.

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

Low

[c4-judge]

GalloDaSballo marked the issue as grade-a

[GalloDaSballo]

After awaring the 2 QAs and the other finding, am raising to A for the high quality descriptions