MIP45 Audit Summary

Four different auditors were engaged for Liquidations 2.0, aka MIP45. These were:

  • Quantstamp
    • methodologies: manual code review, static analysis
    • final report
  • Trail of Bits
    • methodologies: manual code review, property-based fuzzing, static analysis, other programmatic methods
    • final report
  • ChainSecurity
  • Gauntlet
    • methodologies: economic simulations of the new and old liquidation systems
    • final report

This post from the Protocol Engineering Team will give an overview and discussion of the findings of each report. Please note that the following summary and the audits themselves are not fool-proof; rather, they attempt to review technical drawbacks and potential limitations with Liquidations 2.0. But they are limited. Thus, all users should heed the risks highlighted in the reports and the risks inherent in all experimental software design and use their independent judgment as to whether they are sufficiently sophisticated or financially capable of engaging with Liquidations 2.0 and the Maker Protocol.

Quantstamp

Quantstamp’s final report contains 12 findings. Quantstamp classified the severity of these as: two (2) Medium, four (4) Low, five (5) Informational, and one (1) Undetermined. Two of these issues (QSP-3 and QSP-4) were addressed at the smart contract level; the rest are addressed via documentation (e.g. in MIP45 or code comments). The issues are listed below; for each, a brief summary is given, along with a justification for our chosen response action. More details can be found in the report itself.

QSP-1: Dog and Clipper could potentially have mismatched components

This issue refers to the fact that Dog.vow() and Clipper.vow() could differ in the deployed system. Similar possibilities in fact exist throughout the Maker protocol; the risk is very low as the components that could be mismatched are set only by governance and rigorous off-chain validation is done on any such action. On the other hand, constraining the configurations could actually be undesirable if the design changes in the future, or if a migration is needed. Thus, we acknowledge the possibility of a mismatch but don’t believe the risk is high enough to warrant addressing.

QSP-2: Misaligned incentives may encourage aberrant behavior

This issue raises the previously-known possibility that chip and tip, the parameters that determine the reward given to liquidators, could allow “incentive farming” if set to values that make it profitable to create Vaults for the express purpose of liquidating them. This is documented in MIP45, and it is expected that the Risk team will thoughtfully recommend values of tip and chip to governance that do not make incentive farming viable. Thus, we acknowledge the issue, but choose not to take action at the smart contract level to mitigate it.

QSP-3: Division by zero

In the version of the code shared with the auditors, if the parameter encoding the liquidation penalty (chop) were set to zero, it would have caused liquidation attempts to revert. While the risk of this is very low (chop is set only by governance), we decided to add parameter validation to mitigate this risk, as chop has an inarguably sensible lower bound of 1 * WAD and should generally, in fact, be set higher to prevent auction grinding attacks.

QSP-4: Auction may halt if peek returns zero

If the oracle price used to set the initial price of an auction was zero, the auction would have been “stuck” until it reached its configured time-based endpoint. This was fixed by requiring that the starting price of an auction be non-zero.

QSP-5: Missing input validation

This issue points out that the file functions which allow governance or trusted modules to set parameters generally do not validate their arguments. Not doing input validation is actually the default across the MCD contracts, for a few reasons:

  1. Much more detailed validation of governance actions or trusted modules can, should be, and is done off-chain. Off-chain validation is also more gas-efficient.
  2. Overly strict input validation is itself a risk as it might limit our options in unforeseen scenarios when it is actually advantageous to set parameters outside of their expected ranges. Core contracts are difficult and risky to update, so we bias low-level components toward greater generality.
  3. Input validation can always be added easily later in the form of intermediary contracts that limit the access of governance to specific modules.

In rare cases we do add input validation (see QSP-3) if there are strong arguments in favor of doing so (in particular, high severity consequences and straightforward delineation between valid and invalid values). However, for the reasons outlined above, we prefer not to add input validation for the majority of parameters in Liquidations 2.0.

QSP-6: Auction parameters may change mid-flight

The various parameters that govern auction re-initialization and price evolution are subject to change even while an auction is running. No action was taken for this issue for the following reasons:

  1. The current auctions also have parameters that could change in-flight, and it hasn’t been a problem.
  2. Auction parameter updates are expected to be very infrequent.
  3. The auction interfaces provide features for bidders to protect themselves from some potential consequences of parameter changes, for example the ability to specify the maximum price they are willing to pay for collateral.
  4. This possibility is clearly documented in MIP45, and integrators, the Risk team, and governance are expected to be aware of this issue and to act accordingly to avoid negative outcomes.
  5. It is possible to construct governance actions in such a way that running auctions can be checked for prior to making changes.
  6. The suggested alternative (caching parameters on a per-auction basis) would greatly increase gas costs for auction participants and could have negative consequences of its own (i.e. in some scenarios it may be desirable to update the parameters of running auctions).

We may consider adding the capability for governance to force resets of active auctions in the future to partially mitigate this issue.

QSP-7: Unlocked Pragma

We may lock the compiler pragma in the future as makes sense, but currently there is no need to do so from a tooling perspective. This has no effect on the deployed contracts.

QSP-8: Privileged Roles and Ownership

This issue notes that the authorization mechanisms in the core contracts do not conform to more widely used ones such as a per-contract owner role. We prefer MCD’s wards authorization pattern for its simplicity and flexibility. It is standard within Maker protocol code, well-understood throughout the Maker developer community, and covered in the MCD documentation, so we feel that no additional action is needed to address this issue.

QSP-9: Contracts may have no authorized ward

It is possible for the number of authorized callers of a contract to be reduced to zero. This applies to all system contracts and hasn’t presented any practical issue. See also the comments under QSP-6 regarding input validation in governance functions. Note in particular that the ability to revoke all administrative access to a contract may in some cases be desirable.

QSP-10: Tips may not be enough to cover gas fees when network is congested

The tip parameter, a fixed DAI reward given to the successful liquidator (or resetter) of an auction, is intended to help defray the costs these actors incur given their uncertain success in bidding. This issue points out that in conditions of extreme congestion, a non-adaptive incentive may cease to adequately encourage desired behaviors. This is a known risk, and must be balanced with concerns around possible incentive farming, which tip is especially prone to. On-chain incentives are viewed as experimental, particularly given concerns around front-running, and in the short-term we believe it’s best if Risk/governance directly manages setting this parameter. Assuming tip proves useful, and an optimal algorithm can be settled on, the contracts can be upgraded, or an instant access module can be added to manage the tip parameter. In particular, it makes sense to wait for EIP-1559 to potentially provide more stable gas pricing before investing significant resources in dynamic reward mechanism design.

QSP-11: Oracle delay may lead to redo-ing and bad debt

This issue highlights that since oracle prices are delayed (to guard against oracle attacks), it’s possible for auctions to start at prices so far above the market price that the auction does not catch up to the market price before needing to be reset, and may eventually settle at a low enough price that bad debt is locked in. This a well-known and much-discussed possibility, covered in MIP45. This possibility must be guarded against choosing risk parameters well.

QSP-12: Incorrect initialization behavior

This issue notes that certain configuration (authorization of the Dog to call auth functions of a Clipper and the need for cut to be nonzero for nonzero prices to be returned by StairstepExponentialDecrease) is necessary for correct functioning of Liquidations 2.0 post-deployment. Recommendations were made to authorize the Dog in the Clipper constructor and to set cut to a non-zero value by default. We chose not to do the former because it could, in fact, be unsafe or have unpredictable consequences in some scenarios to blindly authorize a supplied Dog address. Further, it is a pattern of Maker contracts that constructors only authorize msg.sender; adhering to this pattern makes the code as a whole easier and safer to reason about and deploy. For the latter, there is not a clear “safe” default, and governance is required to set this parameter in real scenarios anyway. Some points made in the response to QSP-5 regarding input validation apply here as well. A comment was added to the StairstepExponentialDecrease contract to document the need to set cut and step, and the importance of setting cut to a non-zero value was emphasized in MIP45.

Trail of Bits

The final report from Trail of Bits contains nine (9) findings. Trail of Bits classified the severity of these as: three (3) Medium, three (3) Low, two (2) Informational, and one (1) Undetermined. The issues are listed below; for each, a brief summary is given, along with a justification for our chosen response action. More details can be found in the report itself.

ToB-1: Very low values of milk.chop can block liquidation

This issue was related to the observation made by QSP-3, although slightly more general (noting that just very low values of chop are sufficient to block liquidations). The lower bound validation added for chop (see response to QSP-3) fully mitigates this concern.

ToB-2: An urn at the 0x0 address cannot be liquidated

Vault ownership is based on Ethereum account address (and collateral type); due to a check in Clipper.kick, a Vault owned by the zero address cannot be liquidated. This issue does not need to be addressed for the following reasons:

  1. Only addresses authorized against the Vat contract (i.e. governance or a privileged module) could possibly create debt at the zero address. Thus a liquidatable Vault belonging to 0x0 is unlikely to arise, and if it does, it will most likely be in a controlled, intentional context.
  2. If there is an undercollateralized Vault belonging to 0x0 due to the edge cases mentioned above, governance can use the Vat's grab API to manually liquidate the Vault, or create a special liquidation module calling that API.
  3. Ordinary users can encumber collateral at the 0x0 address, but a) this poses no risk to the system, b) it does not create debt and thus does cannot create and undercollateralized Vault, and c) governance can intervene via grab to extract the encumbered collateral if needed.

ToB-3: Integer division rounding can reduce or eliminate the collateral received during an auction

Since the collateral disbursed in a purchase is sometimes adjusted due to the maximum DAI target being reached or dust considerations, it is possible for rounding error from dividing a DAI quantity by the auction’s price to result in zero collateral disbursed despite a nonzero DAI payment. No modifications were made based on this for the initial launch of Liquidations 2.0 for the following reasons:

  1. It is extremely unlikely that this edge case occurs in practice, in particular, but not only, because DAI quantities have 45 digits of precision whereas prices only have 27.
  2. Bidders are generally assumed to be doing profit evaluation prior to bidding.
  3. Bidders can permissionlessly bid through a proxy contract to add a check on collateral received for only a slight increase in gas costs.

We will consider addressing this possibility in future upgrades of the liquidation system if it makes sense to do so.

ToB-4: Incentives for starting auctions can cause gas prices to increase

This issue notes that since anyone can call Dog.bark or Clipper.redo and receive an incentive for starting or restarting an auction, frontrunners are likely to compete to make these calls against more dedicated Keepers. The priority gas auctions that result from these zero-sum dynamics may effectively negate the incentive, with only miners profiting. While it is not bad that miners earn from securing the network, the asymmetric effort between identifying a liquidation or restart opportunity (which requires extra work beyond simply running a node), and frontrunning or otherwise taking advantage of an opportunity found by someone else, could discourage anyone from doing the identification work (as it is strictly less profitable to do so). This could result in a fragile ecosystem with few or no participants proactively triggering liquidations.

This is a well-known and much-discussed concern. After launch, we’ll closely observe what outcomes occur “in the wild” once the auctions are live; it’s possible that the use of the incentives proves unnecessary, or perhaps they’ll be relatively uninteresting to frontrunners due to higher-value opportunities elsewhere. If zero-sum dynamics are observed to negatively impact the Keeper ecosystem, e.g. by discouraging participation as mentioned, stopgap measures for the DAO to consider include using out-of-band mechanisms to compensate Keepers for protecting the protocol (e.g. directly paying select participants for triggering liquidations). Longer-term, we will continue our research into improvements on the current mechanism. Small changes such as adding a delay to reward claimability, or making liquidations and restarts two-step (with a commit-reveal pattern), are already under consideration for a near-future upgrade. Larger game-theoretic changes such as giving Keepers with certain stakes in the system (e.g. governance-locked MKR or large Vault positions of their own) strategic advantages in liquidations are also being discussed, and non-zero-sum liquidation mechanisms like those pioneered by projects such as B.Protocol and KeeperDAO are being actively researched as well.

The full implications of miner-extractable value (MEV) are still being worked out by the Ethereum community. We’re staying up-to-date on all developments, and will be ready to evolve the protocol as necessary to ensure it achieves its design goals. Currently, we believe the greater liquidity available to a Dutch auction mechanism due to composability outweighs the potential negative impacts of MEV for the reliability of the liquidation system.

ToB-5: Important price parameters can be changed during auctions or calls to take

See discussion under QSP-6, which is an identical observation.

ToB-6: No lower bound on the amount of collateral to buy

Race conditions between calls to take can result in bidders purchasing less collateral than they expect, with no lower bound, potentially losing money due to gas costs. It is impossible to fully mitigate this (as even a reverted transaction will consume gas), but similar to ToB-3, bidders can use a permissionless proxy contract to cancel their purchase if the amount of collateral received is lower than what they are willing to accept. We may consider adding a lower limit for collateral purchased as an argument to take in future upgrades of the liquidation system.

ToB-7: Insufficient validation of parameters during updates

Refer to discussion under QSP-5, which is essentially the same issue. A consistency check was added in the Dog to ensure clip addresses are filed for the correct ilk.

ToB-8: Solidity compiler optimizations can be dangerous

During development, compiler optimizations were briefly turned on as we wanted to investigate potential gas savings. This issue points out that many Solidity compiler bugs have been found in the optimizer. After assessing the gas savings of compiling with optimizations to be at best a few percent, we decided that compiling with optimizations was not worth the increased risk (given that we haven’t had time to formally verify Liquidations 2.0 prior to launch), and disabled the optimizer for the initial deployment.

ToB-9: Deployment scripts lack proper validation and are error-prone

This finding highlights a known area of technical debt for the team. We plan to improve the validation in the deployment scripts long-term. It is worth noting that these scripts are only used to fully deploy the system from scratch, e.g. on testchains or fresh mainnet deployments. Enabling MIP45 on mainnet will be done via an executive spell, which does not involve these deployment scripts, and will be extensively validated.

ChainSecurity

ChainSecurity’s final report contains 10 findings. ChainSecurity classified the severity of these as: four (4) Medium and six (6) Low. The issues are listed below; for each, a brief summary is given, along with a justification for our chosen response action. Their report also included a number of notes–relevant considerations which are not necessarily issues–to which we will also give responses here. More details can be found in the report itself.

CS-1: Incorrect Description of Dog.bark()

This issue pointed out that we forgot to update the comments describing the bark function after making a change to the code. We fixed the comment, resolving the issue. This might seem minor, but keeping comments and other documentation congruent with the code they describe is actually quite critical–inaccurate documentation could cause confusion and delays, and in the worst case could lead to costly mistakes.

CS-2: Dirt Remains After Bad Auction

This issue was a bug in an edge case of the auction code: bidding always reducedDirt and ilk.dirt by the DAI actually collected. However, if all collateral from an auction were purchased without covering its target amount of DAI (e.g. because the price has declined too severely), the difference between the collected and target amounts would remain in Dirt and ilk.dirt, building up over time. While governance could have acted to correct this, it could potentially have artificially slowed the pace of liquidations in critical moments. This bug has been fixed in the code–when all collateral is removed from an auction, the full remaining DAI target is subtracted from Hole and ilk.hole.

CS-3: Potential Reentrancy During Emergency Shutdown

This was probably the most serious and interesting issue uncovered in the course of the audits. After an Emergency Shutdown, it was possible to corrupt various aspects of the running auctions via reentrancy. This would have been done by calling take on an active auction, passing instructions to call an external contract that would call the End.snip function on the same auction passed to take. End.snip in turn calls Clipper.yank, which cancels the auction. The main negative effect was that some remaining auctions would have been unable to fully complete or be canceled, which would translate further into accounting inaccuracies in the global settlement process. This issue was fixed by adding a reentrancy guard to the yank function.

CS-4: Specification Mismatches

There were initially a number of inconsistencies between MIP45 and the code itself. These issues have been resolved by correcting and clarifying the text of MIP45.

CS-5: Specification Mismatches Due to Final Changes

Changes made in response to other issues were not initially reflected in MIP45; this has been corrected.

CS-6: Dust Retrieval Is Relatively Expensive

Initially, the dust value for the relevant ilk was read from the Vat in Clipper.take. Since this value is a member of a struct and the Vat does not expose an individual getter function, this required reading all values from the struct, a very gas-inefficient operation. Thus, we added a caching mechanism to the Clipper for dust, so that the stored value only needs to be (permissionlessly) updated upon a parameter change.

CS-7: Duplicate Functions in Clipper

The Clipper initially contained two functions for accessing the value at a particular index of the active auction array. One of these was removed to reduce code size.

CS-8: Gas Inefficiency During Auction Removal

This issue noted that when removing the last auction in the active list, more storage operations were performed than necessary, increasing costs. It was suggested that we check for this case and avoid these operations. While this change slightly increases gas costs when any auction besides the last one is removed, the savings in the last-removal case was deemed large enough to justify its implementation.

CS-9: Liquidation of Dusty Vaults

In certain edge cases of the original code, the liquidation of dusty Vaults (Vaults with total debt less than Vat.ilk.dust) would have been blocked, even though there was technically room for them. The liquidation logic was re-architected and now dusty Vaults can be liquidated as long as they do not cause Dirt to exceed Hole or ilk.dirt to exceed ilk.hole.

CS-10: room > 0 Check Can Be Omitted

This issue found that a check being done in the code was redundant as it was covered equally well by another check being done. This statement was removed in the process of addressing CS-9.

CS-Note-1: Blocked Calls From Clipper

The external call made from the Clipper carries the risk of allowing funds controlled by the Clipper to be moved in an inappropriate way. For this reason, calls to certain other system contracts are blocked. However, in the future, new functionality could be added to the system that creates a vulnerability if there is a function selector collision. We intend to automate the process of checking functions of new contracts added for such collisions.

CS-Note-2: Creation of Many Small Auctions

When ilk.dirt is close to ilk.hole (or Dirt is close to Hole), an attacker could cheaply create many small auctions. This is done by, within a single transaction, repeatedly purchasing a small amount of collateral from an existing auction, and then partially liquidating a Vault. EIP-2929 (implemented in the Berlin hard fork) makes this even cheaper to perform. This could potentially be used for incentive farming, especially if tip is set too large relative to dust and gas costs. While incentive farming is a known risk of MIP45, this method is noteworthy as being potentially more cost-effective than others.

A discussion of this possibility has been added to MIP45.

CS-Note-3: Debt Queue Not Updated Automatically

This note observes that there is no synchronization between the time it takes bad debt recorded from liquidations to “mature” into a form that is eligible for cancelation, and the collection of DAI from auctions. This is a known behavior of the system, and well-managed through appropriate tuning of risk parameters such as Vow.wait and those that control the duration of auctions.

CS-Note-4: Ethereum Is a Dark Forest

This note observes the fact that transactions aiming to collect the kick or redo incentive can be front-run, potentially discouraging Keeper participation. It is also possible that certain calls to take may be front-runnable as well (especially if they rely entirely on DEXes or flash loans for liquidity). A detailed discussion of such concerns is undertaken in the response to ToB-4.

CS-Note-5: Incentive Farming Might Be Possible Due to Misconfiguration

See QSP-2 and CS-Note-2.

CS-Note-6: Initialization and Deployment Requires Extra Care

This is an observation that non-trivial initialization and authorization procedures are necessary to integrate the new contracts into the system. Such actions are done in executive spells that undergo extremely thorough review and testing, but we hope to improve on this process in the future. In particular, we’d like to start earlier on codifying and testing deployment logic so that it can be covered by audits as well as the core contracts themselves.

Related issues include QSP-1, QSP-12, and ToB-9.

CS-Note-7: Monotonicity of Price Functions

See discussion under QSP-6 (also equivalent to ToB-5).

CS-Note-8: No Stability Fee During Auctions

Since stability fees are not collected after a liquidation, it’s theoretically possible for the proceeds of an auction to fail to offset the fees that would have been collected during the time the auction was running. Not only are very unrealistic values required to achieve this scenario (high fees and long auctions), in most cases the “fees” would actually be bad debt as the worth of the collateral would be less than the debt owed. For practical values of fees (0 to 50%) and practical auction durations (a few hours to days), this is mathematically impossible.

CS-Note-9: Vat Debt Tracking Not Automatically Synchronized

Debt accounting is indeed asynchronous in the Maker protocol; this is by design and allows for great modularity and flexibility in the components of the system, as well as better gas efficiency (e.g. debt cancellation from many auctions can be achieved in a single call to Vow.heal). While this is sometimes a challenging and counterintuitive concept for newcomers to grasp, the aforementioned benefits more than justify the design.

Gauntlet

Gauntlet performed a very different type of audit compared with the other three–economic simulation. Their modeling combined a few different pieces:

  • a private, customized Ethereum testnet with a deployment of MCD
  • agent bots for various system actors, in particular auction Keepers with different bidding strategies
  • price models for ETH and DAI
  • a simulated external exchange that the agents could trade on
  • a mempool model to allow simulation of stressed network conditions.

This audit tackled two broad tasks: comparing Liquidations 1.2 and 2.0, and examining optimal parameter choices for 2.0. For the former, it was found that: “Liquidations 2.0 system outperforms Liquidations 1.2 in every unambiguous metric of throughput and collateralization risk” (see here). This was the intention of the Liquidations 2.0 design, so this was a satisfying result. For the latter (investigating optimal parameter choices for 2.0), we refer readers to the report for full details, and to the Risk team’s forum post on initial LINK-A parameters, which used the Gauntlet report to inform parameter choices alongside the Risk team’s complementary simulations and considerations.

15 Likes