Yield Witch v2 contest - horsefacts's results

Summary

Alberto asked on Twitter whether the Yield team succeeded in making their contracts as easily auditable as possible. I think so. I appreciated the sequence diagrams and design decisions in the audit README, the general protocol docs, and the narrow focus in this audit on a single contract.

Here are a few more recommendations:

• I find Maker terminology like ilk, ink, and art very useful in the domain of lending protocols, but it can be confusing to the uninitiated. It would be helpful to provide a glossary of these key terms in your project docs. Additionally, it would be helpful to note anywhere that Yield's definition of some concept diverges from Maker's. For example, a Yield ilk is a bytes6 ID, while a Maker ilk is a bytes32.
• Since this audit was limited in scope, it included a fairly narrow set of tests in Witch.t.sol, with external dependencies mocked or stubbed. However, at least one of my findings was related to interactions with other protocol contracts. I wrote a stubbed out test for this finding using the existing test harness, but if it was easy to write an integration/simulation test as a PoC, I would have. The hardest part of writing a test like this for an unfamiliar protocol is orchestrating all the dependencies. Consider providing a test harness that sets up all the core contracts as a tool for auditors.
• Highlighting key changes between Witch V1 and Witch V2 with references to the code might have been useful, especially to motivate why you chose to make certain changes in the new design.

All in all though, your documentation is great. Thank you for investing the time and effort required to make auditing as easy as possible.

Low

Auctioneer reward can be sent to protocol contracts

I don't see a clear exploit path here, but it's possible for the caller of auction to send their auctioneer reward to Yield protocol contracts, for example the Witch itself, or the Join contract corresponding to the liquidated asset. The Join contracts appear to handle unexpected assets correctly, but consider whether there may be places in the protocol where this sort of transfer could interfere with internal accounting.

QA

Emit all auction parameters in an event

The Auction data type created and stored in Witch#_calcAuction includes the initial parameters for a given auction, and writes these values to a storage mapping:

Witch#_calcAuction

            DataTypes.Auction({
                owner: vault.owner,
                start: uint32(block.timestamp), // Overflow is fine
                seriesId: vault.seriesId,
                baseId: series.baseId,
                ilkId: vault.ilkId,
                art: art,
                ink: ink,
                auctioneer: to
            });

However, the Auctioned event emitted from Witch#_auctionStarted includes only the vault ID and timestamp:

Witch#_auctionStarted

    /// @dev Moves the vault ownership to the witch.
    /// Useful as a method so it can be overriden by specialised witches that may need to do extra accounting or notify 3rd parties
    function _auctionStarted(bytes12 vaultId) internal virtual {
        // The Witch is now in control of the vault under auction
        cauldron.give(vaultId, address(this));
        emit Auctioned(vaultId, uint32(block.timestamp));
    }

It's possible to look up these parameters on chain by looking up the auction by vault ID in the auctions mapping, but not to access them from an event. However, since offchain indexers like the Graph primarily rely on event data, it is probably useful to emit all initial auction parameters and subsequent changes to the auction state through events. (I would also recommend including line duration and initialProportion for the auction in this event). Since an ongoing auction's current parameters are a pure function of initial conditions, remaining art/ink and time, this makes it possible to calculate the current state of any auction offchain using only event data.

This recommendation comes from personal experience: I helped develop and maintain an indexing service for Maker liquidation auctions, and having access to necessary data through events rather than having to look it up from contract storage was extremely useful for offchain monitoring tools, liquidation bots, and frontend UIs.

Consider a shared Witch registry

Witch v2 is designed to allow multiple Witch contracts to run in parallel. As part of this design, each Witch maintains its own registry of all sibling Witch contracts:

Witch#setAnotherWitch

    /// @dev Governance function to set other liquidation contracts that may have taken vaults already.
    /// @param value The address that may be set/unset as another witch
    /// @param isWitch Is this address a witch or not
    function setAnotherWitch(address value, bool isWitch) external auth {
        otherWitches[value] = isWitch;
        emit AnotherWitchSet(value, isWitch);
    }

With this design, adding Witch number n requires 2n - 2 transactions: one tx to each of the existing contracts to register the new sibling Witch, plus n - 1 to the new Witch to register all of its siblings. This may be expensive and error prone if there are many Witches. (And it is probably perfectly fine if there are not).

Consider whether a single, shared registry of Witch contracts would simplify the design or save gas.

Use either custom errors or require statements

Both custom errors and require statements are used throughout the codebase. Consider adopting one or the other pattern for handling errors. This is more consistent, lowers the cognitive overhead of reading and understanding the code, and is less prone to error.

(I find it easy to accidentally reverse an error condition when switching between require and custom errors, since their logic is typically reversed: custom error conditions usually evaluate true to revert while require conditions should evaluate false).

Errors/improvements in comments

The comments on L#118-122 related to setLimit seem out of place or outdated. The setLimit function only manages the "maximum collateral" value referenced in these comments, but not duration, proportion, minimum collateral, or decimals:

    ///  - the auction duration to calculate liquidation prices
    ///  - the proportion of the collateral that will be sold at auction start
    ///  - the maximum collateral that can be auctioned at the same time
    ///  - the minimum collateral that must be left when buying, unless buying all
    ///  - The decimals for maximum and minimum

Review whether these comments are relevant to the setLimit function.

I found the comment on L#418 confusing:

        // Update concurrent collateral under auction
        DataTypes.Limits memory limits_ = limits[auction_.ilkId][
            auction_.baseId
        ];

This line loads the current limit into memory, but does not actually update it. The updates happens on L#430 and L#450.

The comment on L#92 might be clearer if it referred to "Time that auctions take to offer max collateral" rather than "go to minimal price":

    /// @param duration Time that auctions take to go to minimal price

Typos

The custom error VaultNotLiqudable should probably be VaultNotLiquidatable, or perhaps something like VaultFullyCollateralized.

Witch.sol#L520

-   /// @dev quoutes hoy much ink a liquidator is expected to get if it repays an `artIn` amount
+   /// @dev quotes how much ink a liquidator is expected to get if it repays an `artIn` amount

Witch.sol#L385

-    /// @dev transfers funds from the ilkJoin to the liquidator (and potentially the auctioneer if they're differente people)
+    /// @dev transfers funds from the ilkJoin to the liquidator (and potentially the auctioneer if they're different people)

Informational

Google Calendar reminder may be insufficiently robust

The Yield team have set a Google Calendar reminder to replace the Witch contract before 7 February 2106:

Witch#L575

        // If the world has not turned to ashes and darkness, auctions will malfunction on
        // the 7th of February 2106, at 06:28:16 GMT
        // TODO: Replace this contract before then 😰
        // UPDATE: Added reminder to Google calendar ✅

However, a Google Calendar reminder may be insufficient to serve as a warning to the future Yield team. In the past, Google has shut down widely used and beloved services (e.g. Reader and Inbox), and there is no guarantee that Google will exist as we know it in the year 2106. Consider taking additional steps to limit this single point of failure.

Suggestions:

• Create a timeless and popular work of art that tells the story of replacing the Witch, like a film, novel, or folk song.
• Hire a Paradigm Fellow and entrust them with the contract upgrade. They will be at most 107 years old in 2106.
• Add a reminder in the "Reminders" app on your iPhone.