Missing storage gap in AbstractStrategy may affect contract upgradeability

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/AbstractStrategy.sol#L8

Vulnerability details

Summary

The AbstractStrategy contract is intended to be used as a base contract for upgradeable strategies but doesn't contain any storage gap to ensure enough space for future revisions.

Impact

In the AfEth protocol, strategies extend a base contract named AbstractStrategy. Currently, there's only one implemented strategy (VotiumStrategy) that inherits from this base contract.

While the AbstractStrategy contract currently doesn't provide any concrete functionality, is it possible to extend its implementation in a future version, as this is an abstract contract that can allow partial implementations.

Being part of an inheritance chain of upgradeable contracts, it is crucial to maintain the storage layout. Any base contract that might eventually require storage space must reserve it in advance.

The current implementation of AbstractStrategy overlooks this consideration. Any future revision of this contract that may require defining and using storage variables will have its storage space clashing with the contract next in the chain.

Recommendation

To resolve this issue, add a dummy storage gap array to reserve storage space for future additions to the contract.

  abstract contract AbstractStrategy is
      Initializable,
      ReentrancyGuardUpgradeable,
      ERC20Upgradeable
  {
+     uint256[50] private __gap;

Assessed type

Upgradable

[elmutt]

asymmetryfinance/afeth#161

[c4-judge]

0xleastwood marked the issue as primary issue

[0xleastwood]

Technically, ERC20Upgradeable already has a storage gap so could be modified to allow for new variables in AbstractStrategy but I think it makes sense to have this also in the abstract contract.

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[d3e4]

Technically, ERC20Upgradeable already has a storage gap so could be modified to allow for new variables in AbstractStrategy but I think it makes sense to have this also in the abstract contract.

Doesn't this mean that the issue is invalid? Since AbstractStrategy is ERC20Upgradeable and ERC20Upgradeable has a storage gap, then AbstractStrategy does have a storage gap, after all.

[romeroadrian]

Technically, ERC20Upgradeable already has a storage gap so could be modified to allow for new variables in AbstractStrategy but I think it makes sense to have this also in the abstract contract.

Doesn't this mean that the issue is invalid? Since AbstractStrategy is ERC20Upgradeable and ERC20Upgradeable has a storage gap, then AbstractStrategy does have a storage gap, after all.

That's space reserved for the ERC20Upgradeable contract, which is a third-party library. Taking space from that would eventually clash storage with that contract itself. This is even considering the protocol is aware of the issue, which is correctly raised here :)

[MiloTruck]

The finding is valid, but according to the org repo "Missing storage gap" issues have always been judged as Low/NC:

• Severity standardization - Contract unupgradeability org#55

[romeroadrian]

The finding is valid, but according to the org repo "Missing storage gap" issues have always been judged as Low/NC:

• Severity standardization - Contract unupgradeability org#55

I have this one as a reference of med severity
code-423n4/2023-01-biconomy-findings#261

I think this particular case is quite relevant since AbstractStrategy is the foundation of strategies, an d strategies are expected to be added in the future.

[0xleastwood]

Didn't realise we were standardizing these types of issues. Good to see that's happening, however, I think context of the issue is always important. I would agree that the assumption being made here is that a future upgrade would be done incorrectly and would not be tested in any way. I'm gonna side with @MiloTruck on this one.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)