Platform: Code4rena
Start Date: 07/06/2022
Pot Size: $75,000 USDC
Total HM: 11
Participants: 77
Period: 7 days
Judge: gzeon
Total Solo HM: 7
Id: 124
League: ETH
Rank: 27/77
Findings: 2
Award: $144.37
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: berndartmueller
Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xNineDec, 0xf15ers, 0xkatana, 0xmint, Bronicle, Chom, Cityscape, Deivitto, Funen, GimelSec, GreyArt, IllIllI, JC, Lambda, Meera, Nethermind, Picodes, PierrickGT, Ruhum, Sm4rty, Tadashi, TerrierLover, TomJ, Trumpero, Waze, _Adam, antonttc, ayeslick, c3phas, catchup, cccz, cloudjunky, cryptphi, csanuragjain, delfin454000, dipp, ellahi, fatherOfBlocks, hake, hansfriese, hyh, joestakey, jonah1005, kenzo, minhquanym, oyc_109, sach1r0, saian, simon135, slywaters, sorrynotsorry, sseefried, unforgiven, xiaoming90, z3s, zzzitron
96.7678 USDC - $96.77
[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
#0 - jeffywu
2022-06-16T12:21:38Z
Good suggestion
#1 - Picodes
2022-06-18T00:20:21Z
Should be upgraded to medium severity based on https://github.com/code-423n4/2022-05-alchemix-findings/issues/44
#2 - gzeoneth
2022-06-26T15:42:16Z
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.
#3 - Picodes
2022-06-26T21:09:10Z
@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 ?
#4 - Picodes
2022-06-27T08:47:45Z
The exact same issue is also here: https://github.com/code-423n4/2022-06-nibbl-findings/issues/132
#5 - gzeoneth
2022-06-27T13:06:00Z
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?
#6 - gzeoneth
2022-06-27T19:40:13Z
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.
#7 - jeffywu
2022-06-28T12:13:58Z
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.
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xSolus, 0xf15ers, 0xkatana, 0xmint, 8olidity, Chom, Cityscape, DavidGialdi, Deivitto, ElKu, Fitraldys, Funen, GreyArt, Lambda, Meera, Picodes, PierrickGT, Sm4rty, Tadashi, TerrierLover, TomJ, Tomio, UnusualTurtle, Waze, _Adam, antonttc, asutorufos, berndartmueller, c3phas, catchup, csanuragjain, delfin454000, djxploit, ellahi, fatherOfBlocks, hake, hansfriese, hyh, joestakey, kaden, minhquanym, oyc_109, rfa, sach1r0, saian, samruna, simon135, slywaters, ynnad
47.6016 USDC - $47.60
[Gas - 01] - Settling of account could be saved locally
After maturation, each time someone burns, an external call NotionalV2.settleAccount(address(this)); is made. Even if it's a noop, it would be cheaper to have a local variable to store this and eventually skip the call as it needs to be done only once.
#0 - jeffywu
2022-06-16T11:54:18Z
Good call