QA Report

[code423n4]

[Low-01] - Add Storage Gaps to facilitate future upgrades

When using inheritance with upgradeability, it is recommended to add storage gaps as it improves readability and is less error prone. Indeed otherwise you can't add a variable in in subcontracts as it breaks the whole storage layout. This concerns at least wfCashBase and wfCashLogic.

Quoting OZ: "It isn’t safe to simply add a state variable because it "shifts down" all of the state variables below in the inheritance chain. This makes the storage layouts incompatible"

For reference: https://docs.openzeppelin.com/contracts/3.x/upgradeable

[jeffywu]

Good suggestion

[Picodes]

Should be upgraded to medium severity based on https://github.com/code-423n4/2022-05-alchemix-findings/issues/44

[gzeoneth]

I think this is still Low Risk until they want to upgrade to a contract that change the storage layout, and that would be another audit.

[Picodes]

@gzeoneth the risk is that they do a tiny upgrade to fix something without audit and upgrade the implementation, creating the storage collision

Anyway it's exactly the same issue as this one https://github.com/code-423n4/2022-05-alchemix-findings/issues/44 which was judged medium, wouldn't it be fair to judge it medium as well ? Is there a process for setting the standard for this kind of classic issues ?

[Picodes]

The exact same issue is also here: https://github.com/code-423n4/2022-06-nibbl-findings/issues/132

[gzeoneth]

Precedence in different contests doesn’t mean this must be the same risk as the context is different. But let me think about it.

@jeffywu what's your take?

[gzeoneth]

we have

contract wfCashERC4626 is IERC4626, wfCashLogic {
abstract contract wfCashLogic is wfCashBase, ReentrancyGuard {
abstract contract wfCashBase is ERC777Upgradeable, IWrappedfCash {

so the storage order is ERC777Upgradeable, wfCashBase, ReentrancyGuard, wfCashLogic, wfCashERC4626

there are no storage slot used in wfCashLogic and wfCashERC4626 ReentrancyGuard use 1 storage slot but it won't break when shifted down to an empty slot (with value 0) since the check is require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); where _ENTERED != 0

It doesn't seems that any upgrade to wfCashBase/wfCashLogic/wfCashERC4626 would break the contract.

[jeffywu]

Based on the judging criteria I believe this falls into QA:

QA (Quality Assurance) Includes both Non-critical (code style, clarity, syntax, versioning, off-chain monitoring (events, etc) and Low risk (e.g. assets are not at risk: state handling, function incorrect as to spec, issues with comments).

Specifically calls out versioning in the description. I can't see either of the issues linked above.

The intention is for all storage slots to be located inside wfCashBase and none of the other contracts, so I think the potential for a storage clash is limited. Certainly I can see how this might be judged higher in a different code base with a different architecture.