I told myself I'd take a couple days break between the DEIT site/Github Org launch before diving into the diamond-hardhat refactor... but I haven't gone to bed yet. I really want to see the teased feature set of shinyblocks built out as referenced, and intend to wrap it up in diamond-hardhat, so... we begin.

Baseline & First Triage Run

cd ~/deit
git clone https://github.com/proggR/diamond-hardhat.git diamond-hardhat-audit
cd diamond-hardhat-audit
branch -m audit-0
npm install

With Slither already installed, to start, I want to see just a count of everything flagged in its current state. For that its simplest to just run slither . --print human-summary. The count lands at:

• 0 optimization issues

• 81 informational issues

• 11 low issues

• 8 medium issues

• 0 high issues

That's actually better than I anticipated, and some of that might be resolved with some of the other planned work, but before I do that I'm curious what the current contracts could score with some effort.

To dig deeper, I'll enter into triage mode with slither . --triage-mode to step through the flags raised.

The first flag is referencing some uninitialized local variables in LibDiamond and DiamondLoupeFacet, which is an easy one I just patched in deit-contracts, so I'll get it out of the way now. I say "patched", but if you look at the code, the uninitialized i's in question would be treated as 0, and pose no real concern. It does however help score better so, done, next.

My MockERC1155Facet._checkOnERC1155Received, OnBatchReceived and my 721 contract's OnERC721Received functions ignore the return value. Ooo, bad form. I'll dig into these at the same time later.

The next set of flags relate to some local scoped _owner variables shadowing my _owner internal function in my Mock721 contract... which ya, is super gross. Funnily enough, as I looked at each of the referenced lines I noticed the one occurrence doesn't even use the _owner var for anything... will definitely be digging through the 721 contract for logical strangeness now.

The next 9 flags raised are all related to the previously highlighted OnRecieved functions of the 1155 and 721 contracts, all relating to vars potentially being used before their declaration.... probably has something to do with that ignored return value thing. Given those implementations were already highlighted, that should hopefully mean a decent chunk of these flags get cleared from those 3 code sources.

The next set of flags all relate to Inline Assembly being used. These are unavoidable with Diamond contracts, and will show up in your Diamond, LibDiamond, and Storage Libraries. That said, it's always good to comb through the highlighted assembly because it's no joke... bad things can happen when assembly is disrespected, and it's entirely possible you've used it elsewhere in your code, bringing in additional considerations. These are all flags where I'd expect to see them, and I'll be refactoring the storage libraries later anyway so I can double-check them then.

The next 2 flags are unused functions, _burn and _mint in the MockERC20Facet which it suggests removing. Fair enough, and is fine given part of this refactor will be to better break up ERC20 interfaces.

The next set of flags is about pragma ^0.8.0. The current suggested version is 0.8.16 so I'll rip through the contracts updating that now to get it out of the way.

The next flag is about a low level call in LibDiamond due to delegatecall. Like the inline assembly flags above, this is unavoidable in a Diamond contract, but should also be investigated when other low level call flags emerge since they can be the source of all kinds of bad things, not least of which is re-entrancy.

The next set of flags are all about parameters in LibDiamond and the other core facets not being in mixedCase, which honestly still confuses me a hair because... are they not?... I thought _ prefixing was still valid mixedCase, and it's used throughout the code elsewhere with no flag?... Either way, that's a style flag and one I haven't been bothered to care enough to attempt refactoring out, so I skim through to see if any related flags show up in my code instead of the lib/core diamond code, and move on when they don't.

Next up the MockERC20 construct() function uses a literal with too many digits: _ds._totalSupply = 1000000 * 10**_ds._decimals. This one I actually had to think about for a second until I clicked through the link Slither spat out and realized... this too is largely a style thing. Here I was for a moment thinking I'd used some strange int type for _totalSupply. It makes sense you'd want to minimize confusion, and long numerals do get confusing without commas. In this case though, the refactor will be parameterizing that value so the flag will disappear then.

Next up, the AppStorageFacet.s variable isn't used in the contracts extending it, which is fair given it was a really contrived example when I made it. Will aim to improve the AppStorage implementation to be more borderline useful/omit it where unused.

And that's the end of the triage walkthrough. With only a couple of the issues actually touched so far, rerunning slither . --print human-summary the count is now down to 48 informational flags (from 81), 11 low issues (unchanged), and 4 medium issues (from 8). I'm finally off to bed for the night, but tomorrow I can start to dig into those unresolved flags and see how low this count lands after the planned refactoring work.

StorageFacet => Storage Library Refactor

Back at it, and now with the list of work to be done in hand, I'll start on the refactoring, do another run of Slither to see if anything new emerged, and get to work resolving the remaining flags.

The first element of the refactor here is tearing apart the storage to conform to the Library pattern used commonly with Diamonds. The pattern of the work will all be the same: move the contract code to a libraries folder, renaming the file and making it a library instead of a contract, and then moving the struct definitions into that new library file, switching both the struct definition and get function names to a standard form (Layout/layout()). Basically... it's largely a bunch of copy paste/renaming work. That said, I will leave the AppStorage as an abstract contract, given it's meant to be extended in order to sync your 0 slot reference to a shared storage layout across facets, and it will live in the storage folder root rather than the storage/libraries folder the rest are located in.

Those files are moved/renamed accordingly, and now comes time to migrate the contracts to use this new structure. This work is all pretty straightforward, largely renaming/updating references to things as a result of the library structure vs the contract-oriented storage methods before. I also update the standard contracts to take in parameters instead of hardcoding values. With the storage refactoring now done, I re-run Slither and seem to have shed 1 informational flag (probably the long literal one), but picked up 1 optimization flag, with all else the same as expected.

Slither Triage Followup

With the core refactoring out of the way, I can dig back into those flags.

Since the _mint and _burn functions not being called raised a flag and a Mintable and Burnable interface/implementation is in order anyway, I start by just moving those functions to the appropriate new contracts and calling them internally... while not actually using them yet or adding any logic to control how/who can mint/burn. This also required some additional refactoring to place commonly needed functions (like _transfer and _requireFunds) and events into a base class/IF.

I seem to have shed 4 informational flags re-running Slither after those changes. From here I'll have to actually think/read through the logic of the contracts, so given ERC721 has the checkOnReceived flags, along with its own variable shadowing issue, I'll dive in there since it should cover the most ground with the shortest path.

First the variable shadowing... easy fix, especially in the case where it's not even being used. Changed the variable from _owner to _ownerAccount and culled the useless one. Down to 9 low issues (from 11... also sorry, this is how I audit... tediously re-running Slither every minor change to see how the "score" adjusts... it's a fun game :P).

Digging into the checkOnReceived function should not only clear some flags in this file, but unlock the secrets to doing the same in the 1155 implementation. For this, I'll want to compare this implementation to a known good one to make sure I haven't done anything wrong, so I hit up OpenZeppelin annndd... it's the same... I'd literally already copy pasted OZ's implementation for this. That should mean it's fine, but let's see if anyone's worked out a way to satisfy the Slither gods.

Re-entering triage mode, I first fix the uninitialized counter I'd missed in the DiamondLoupeFacet during that first pass last night, and then double-check the link Slither spits out for the flag in question but it doesn't really help. Then searching online I find this, confirming Slither's handling of try/catch code presents a known false positive. Still, I double check the other sources of this bug and cross reference the implementations to OpenZeppelin's, and they all seem to check out, each of them being just copy/pastes of OpenZeppelin's implementation. Sweet.

It's currently sitting at 1 optimization, 45 informational, 9 low, and 3 medium issues. But given the unused return flags as a Medium issue, and I have 3 of those flags, I should actually be sitting at 0 medium issues (which I'll confirm later by commenting out the source).

The 9 low issues I suspect relate to the 9 flags also stemming from these checkOnReceived functions. Walking through the triage again, the link Slither provides is useful information, and indeed feels like an extension of the previous try/catch strangeness with Slither, likely assessing the scope of retval incorrectly. In practice, this var won't be assessed until the return from the function call, and the reason var in the catch won't be assessed unless that previous call fails, so it shouldn't be an issue. The link also confirmed this gets flagged as Low, making it the culprit for the 9 Low issue flags, and bringing the adjusted count down to 1 optimization, 45 informational flags, and 0 of everything else. Awesome :)

The only remaining flags that aren't either expected (assembly/delegatecall as per Diamond requirements), a false positive, or an overly zealous style issue (which make up 27 of the remaining informational flags on its own) are the flags related to AppStorage, first the unused s in the contracts that extend it, and then it deciding s should be a constant, which is actually because I forgot to rename AppStorage to Layout, so it was thinking it was a reference to an AppStorage contract instead of the struct on the following line. Corrected that, and for now I'm actually going to just remove the AppStorage reference from the Facets that don't need it, and switch the appStorage() calls in the other to instead use the s variable that reserves the 0 slot for the AppStorage.Layout, removing that function to avoid a fresh "unused" flag. That said, seeing how this is now written, I like it less because now s remains uninitialized, which it just seemed to not like for the i counters in the core Diamond contracts. Realistically that's not any different from how DiamondStorage itself would work, but as a personal preference, I'd always opt for DiamondStorage over AppStorage, and even though you can use the two strategies together, I'd prefer to just make an App level DiamondStorage library and leverage that wherever needed, or if I wanted to fetch it internally from an abstract base contract would still prefer a DiamondStorage style getter over referencing a variable I have to trust occupies slot 0. It just feels cleaner and less error prone to me, whereas AppStorage feels like it could throw a weird curveball by trusting slot 0 is what it should be.... why not slot keccak256("app") as returned by layout() instead?... just my 2¢ :P

The final Slither flag count is now: 0 optimizations, 39 informational (27 of which are from the "mixedCase" flags, 11 of which are from diamond related assembly, and 1 is from the delegatecall), 9 low issues (all stemming from the try/catch false positives), and 3 medium issues (also all try/catch false positives). Adjusting to exclude the false positives/known style issues the count is now down to... 0. The basic Slither review is now complete, and any necessary flags have been addressed. There's more to be reviewed, but clearing those flags surfaced in the human summary is always step 0 on your way to passing a complete audit.

Introducing shinyblocks

With the contracts now passing their Slither review, I'll need to properly comb through the logic of them all before I can call them fully audited. That however is always easier when you can interact with and run tests on the contracts to spot inconsistent/unexpected returns/behaviour, which brings me to the second part of this refactor project: re-rolling tasks/tests with shinyblocks.

shinyblocks is a WIP Typescript lib I've started to work on that aims to over time replicate the internal structure of a Diamond, and that reduces interactions to their bare implementation details, tucking all the boilerplate within. Or that's the end goal... for now its 3 core functions in varying states of feature parity/utility: Diamond, Facet, and Facets (with the A helper function seeing a decent amount of use as well).

To get started, I'll clone the existing build of shinyblocks as a submodule so I can push any changes back to its repo later (and there will be changes here, I'm sure). Since the tasks are the easiest means to see it in action and start interacting with the now refactored and reviewed contracts, I'll start there.

To make sure all of my tasks are able to make use of shinyblocks, I'll do a quick and dirty job, tossing this near the top of my hardhat.config.ts file:

extendEnvironment((hre) => {
  const { Diamond, Facet, Facets, Signer, CA, A } = require("./lib/shinyblocks/shinyblocks")    
  hre.Diamond = Diamond;
  hre.Facet = Facet;
  hre.Facets = Facets;
  hre.Signer = Signer;
  hre.CA = CA;
  hre.A = A;
});

Later on I'll tuck that away inside the lib, and provide a variation that injects the shorthand aliased versions of D, F, Fs, and S instead, but will use the proper names here for clarity.

Most tasks at the moment look like something to the effect of:

    const accounts = await ethers.getSigners()
    const signer = accounts[0]

    const Facet = await ethers.getContractFactory("MockERC20Facet")
    const facet = new ethers.Contract(taskArgs.diamond,Facet.interface, signer)

    const tx = await facet.symbol()
    console.log("RESPONSE: ",tx)

Some of that could be trimmed down using named accounts, or by using hardhat-ethers' getContractAt, but now leveraging shinyblocks, the ERC20 symbol task above becomes:

const tx = await (await Facet(hre, A(taskArgs.diamond), "ERC20")).instance.symbol()
console.log("RESPONSE: ",tx)

And... that's it. If your signer is your 0 account, that's enough to attach to the "ERC20" interface at the given address, and then call its symbol() function. If your signer isn't address 0, you can just pass it as the parameter following "ERC20". You can also split it up, assigning the return of the Facet await to an intermediary var if you'd prefer/need that var for anything else. You can also use i in place of instance, which is what I've actually done in the codebase. As you can imagine, it doesn't take long to swap out the existing tasks. Most until the diamond related tasks anyway, where hooks into the previous 3 random JS files were helping before.

Turns out this is where I hit the first wall integrating shinyblocks, because the way I handled deploys/upgrades from the tasks before would deploy first, then cut by calling a separate task and passing it the diamond/facet addresses. The object returned by the Facet function does include a Cut function so in theory I should be able to do the same thing, but upon looking at how I've implemented that it's not correct, and would end up redeploying the facet which isn't what I want. So for now instead of the facet-upgrade task handling the deploy, and cut-replace handling the cut, or facet-deploy followed by cut-add, the cut tasks will be ignored to use the deploy + cut abilities of shinyblocks' Facets instead. I've noted this limitation though, since I should be able to load a Facet object by address and then cut it to a Diamond by address without using Facets to do both, so that's added to the roadmap. I've aimed to replicate the past task behaviour though, with facet-deploy supporting deploys without cuts by omitting the diamond. That said... there's currently no task to then cut a deployed contract, so I'll still have to circle back.

if(taskArgs.diamond > 0){
    const facets = await Facets(hre, A(taskArgs.diamond),[taskArgs.name],CA.Add)
    console.log(facets[taskArgs.name].i.address)
}else{
    const facet = await (await Facet(hre, false, taskArgs.name)).Deploy()
    console.log(facet.address)

    //^ note the lack of `i`. this will be ammended 
    //Deploy currently returns ethers object, ie: what `i` references
}

facet-upgrade always required the diamond, which sort of made the cut-replace task pointless from this task's perspective to begin with, and is now simply handled with:

const facets = await Facets(hre, A(taskArgs.diamond),[taskArgs.name],CA.Replace)

With that out of the way, I'll cobble together a deploy script that can throw together some facets to play with. Fortunately, the recent project from the last post has such a script that with minor modification should work (the param order of Facets was changed between then and now to make it shorter when the signer is accounts[0]).

const _totalSupply = hre.ethers.utils.parseEther("100000000000")
const _signer = await Signer(hre)

const _test = await Diamond(hre, 'Diamond')        
const _testFacets = await Facets(hre, _test,
                                        [
                                            "ERC20",
                                            "GreeterFacet"
                                        ],
                                        CA.Add
                                    )                                     

await _testFacets.ERC20.instance.construct("Test Token", "TEST", 
                                        18,
                                        _totalSupply, 
                                        _signer.address
                                    )

And it does! The above short deploy script deploys a diamond, cuts 2 facets to it, and then calls a construct function to set the values of the ERC20 token from the context of the Diamond contract. Note: this is wildly gas inefficient vs what's possible with Diamond deployments, but future improvements to solve for that are highlighted on the shinyblocks repo's README.

After finding some pre-existing bugs copy pasted through some tasks (oh gorsh, its not tarskArgs lol), the ERC20 contract checks out, and does everything I expect including approve/allowance support, now doing it all with shinyblocks instead of pure ethers or the assortment of JS files this repo used before. Sweet.

The 721/1155 implementations I expect to run into concerns with, especially the 721 one after that orphaned shadowed var, but after changing my deploy script to cycle between them, and some similar minor task spelling edits, each function of each token seems to behave as I'd expect. That's at least a user test of the token standard contracts seemingly passing. I'll still need to see proper unit tests, and to properly comb through the code myself before I'll call this repo properly audited, but static analysis review + UX testing passing is a good place to be.

The last UX test for the tasks are the diamond related tasks. The cut tasks, as mentioned, for now won't work, but the rest should... and do! Almost... the remove function of shinyblocks was missing an await, but with that small patch I can successfully call facet-deploy to cut a new facet to the diamond, or facet-upgrade to deploy a new contract and make a replace cut, and facet-remove removes a facet, which can be confirmed when its functions no longer respond. Likewise the Loupe tasks return the addresses, or selectors, etc, and as with the other tasks all using short and to the point shinyblocks calls now.

Wrap Up & Next Steps

Rather than running longer, I'll leave it there covering the audit/refactor/task-driven UX testing. Before I dive into integrating shinyblocks with the unit tests, I'll want to circle back to resolving that Cut deficiency identified during the integration since I tend to like using cuts in tests, running through a series of add/replace/remove/add and checking that each step behaves as expected. I'll also be updating the tasks to allow optional params that let you specify an account index for the signer, since without that it becomes tedious to test using tasks from any but the 0 address.

I'm also torn on param order for Facet... it's currently hre, diamond, facetname, because I'd imagined it being used primarily to reference a Facet from its Diamond context to then interact with it, yet... there's many cases where you might want to just load a Facet's interface and have no Diamond where you have to add a pointless feeling false in the current version... I may end up swapping that to be hre, facetname, diamond so you could load a Facet interface more tersely with Facet(hre,'FacetName') but then that gets weird when the equivalent call to Diamond currently also deploys ... its all in flux as a lib so there will be many breaking changes between here and the end goal form I'm sure.

For now these changes are tracked in the audit-0 branch of the repo, and once the tests are refactored with shinyblocks to cull the diamond/deploy/facets.js files (hopefully with a more stable spec established for shinyblocks by then), that branch will be merged to become main.

Keep shiny friends :)