-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QA Report #227
Comments
Good suggestion |
Should be upgraded to medium severity based on code-423n4/2022-05-alchemix-findings#44 |
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. |
@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 code-423n4/2022-05-alchemix-findings#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 ? |
The exact same issue is also here: code-423n4/2022-06-nibbl-findings#132 |
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? |
we have
so the storage order is there are no storage slot used in wfCashLogic and wfCashERC4626 It doesn't seems that any upgrade to wfCashBase/wfCashLogic/wfCashERC4626 would break the contract. |
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. |
[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
andwfCashLogic
.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
The text was updated successfully, but these errors were encountered: