search

makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
107 stars 43 forks source link

Pe 1177: Target Jan 27, 2023 Exec #308

Closed brianmcmichael closed 1 year ago

brianmcmichael commented 1 year ago

Description

Contribution Checklist

Checklist

The-Arbiter commented 1 year ago

Hash matches (0xf073025c5ea2e689c90bc79dd8ef53f12cc620a53748003ee7de8ca94a5793e7) Flash killer deauth OK Content matching exec OK

gbalabasquer commented 1 year ago

Guys I was glancing the offboarding of the old D3M. I think we are over complicating it. I do not see the need to cage the module or to deauth anything from the module to the core contracts or even less deauth the pause proxy (I don't think we have ever done it). That is what I referred about cutting the whole module from the root instead of dissembling all the internal relationships/ownership.

gbalabasquer commented 1 year ago

Leaving my comment here for the posterity. As I expressed in other channels, I do NOT agree with the unnecessary de-auths, in particular removing the pause proxy from old contracts, which might bring unintended consequences. Everyone in the team was noticed that in my opinion this wasn't the most convenient path. Also, this breaks the standard followed in previous offboardings.

godsflaw commented 1 year ago

Leaving my comment here for the posterity. As I expressed in other channels, I do NOT agree with the unnecessary de-auths, in particular removing the pause proxy from old contracts, which might bring unintended consequences. Everyone in the team was noticed that in my opinion this wasn't the most convenient path. Also, this breaks the standard followed in previous offboardings.

Let me take a moment to lay the reason out, where I think we differ in opinion, and perhaps the path forward to resolve this difference.

The position

The ESM's purpose, as I see it, is to protect the system accounting during emergency shutdown so that users and integrations can function safely. This is almost always a minority threat, made under duress, to protect the system and users from a majority governance attack where dropping the spell is not an option. So, the ESM acts as a credible threat to the majority to find consensus with the minority, as well as a protection of the system so that users can exit their funds safely.

It's in the latter statement that I believe we can derive a best effort intention of the ESM to protect users and integrators. That is, the ESM protects the core accounting and custodial funds, so that it may protect that value for those users and integrators.

We have never articulated the specific reason various modules have authenticated the ESM, just that we intended for the pause proxy to be denied in those modules. My argument, derived from the broad intention of the ESM to protect users and integrators, is that we need not consider the specific reason when off-boarding a module as to why it has ESM auth, but rather that it was intended for the pause proxy to be removed in the case of an ESM activation. We don't specifically know if that intention was to protect the core accounting from a manipulation of that module, or to prevent a general manipulation of that module from impacting users or integrators. I can manufacture any number of ways that a module could be isolated from the core with a minimal deauth scheme as you suggest, and yet still impact users and integrators negatively via ERC20 approvals, keeper manipulation, or exploiting third-party integrations to name a few methods.

Where we differ

So this raises the question when off-boarding a module: Given we cannot divine the original authors intention for adding it to the ESM, do we deauth the pause proxy immediately when the module is off-boarded, or allow some third party to orchestrate this at the time of ESM, when there is a limited time to act, the end process must be orchestrated in tandem, there's no clear registry of past module authentications, and the entire process is under the chaos of one of DeFi's largest projects collapsing?

I would argue that, under our current regime of not documenting all these ESM authorizations, and given the chaos of that moment, we should clean things up to the greatest extent possible in the present. Now, I do think you make a good point about this being inconsistent with how we've done things in the past, and I do think you make a good point that this could prevent governance from taking legitimate actions on that module in the future (for whatever reason). I can address those two issues next.

As for this being inconsistent with how we've done things in the past, I think we can look to the spiraling complexity of the system as to why it makes sense to clean things up as we go along. Already today, we have modules that have fallen out of the chainlog, old chainlogs, as well as old versions of the ESM. There is limited records other than what we can discover with an archive node, of finding all these old authorizations. Many of our old auth routines didn't even log, or logged inconsistently, and these authorizations are not easily indexed as they are ESM auths in other modules, so the sources are spread across that history. It would be exceedingly complex to orchestrate a deauth at ESM activation today, and we are pushing into a multi-chain future with who knows how many SubDAOs. This methodology simply won't safely scale into Maker's future, and for that reason I think it makes sense to break from how we've done things in the past.

To address the legitimate actions governance may be able to take on an orphaned module, I think for many of the reasons above it makes sense to act in the present, when the team's knowledge is more complete. But I would be lying if I said it didn't give me pause that we may have forgotten something where the only avenue to fix it was with the pause proxy's auth on that orphaned module. I think this is the krux of where we differ in opinion, and I do think you make a good point. Here too, I believe in a perfect world, we would pause here and make a call as to how we proceed. My issue is that, even though I think you've made a good point, I don't feel safe carrying on managing this the way we've done in the past, and the solution to maintain this authorization and address my concerns is complex enough to stop spell work for a few weeks.

The path forward

To resolve this in the future, we should either build and maintain a meticulous registry of these ESM authed modules, while requiring module additions of the ESM to add themselves to the registry (perhaps we could even automate this in the DssExecLib), or we should pull that deauthorization work into the present and scuttle the module while the information is fresh in everyone's heads. This latter approach is what we've done here, but I think the team needs to make the call about how we handle this spiraling complexity in the future, and the pros/cons to each approach so that we can choose an approach and not have this be settled by the whims of the spell crafters that week.

A third approach is what we've been discussing with the new D3M AAVE module: that we explicitly articulate in the module's repo how it will be off-boarded in the future. I would hope this takes into account isolation of the core system and any impacts to users and integrations. Then when we offboard a module, it's clear what should happen. This has the benefits of fresh and extremely thoughtful considerations of all parties using the module as well as the core system so that off-boardings can be handled and put behind us, and presumably that module is still in the chainlog, which makes it easy to discover ESM auths during an ESM orchestration.

Conclusion

We should figure out a consistent path forward for the entire team, possibly even disagree and commit if we can't reach consensus. We should probably do this before the addition of the new AAVE D3M module if we intend that offboarding code to exist at module addition time, and possibly even for the next spell with has additional off-boarding considerations. Sorry, we differ in opinion on how to handle old modules, disconnecting or scuttling them, but I think we can find a consistent path forward in the future.

For now, this AAVE module will be scuttled.

gbalabasquer commented 1 year ago

Conclusion

We should figure out a consistent path forward for the entire team, possibly even disagree and commit if we can't reach consensus. We should probably do this before the addition of the new AAVE D3M module if we intend that offboarding code to exist at module addition time, and possibly even for the next spell with has additional off-boarding considerations. Sorry, we differ in opinion on how to handle old modules, disconnecting or scuttling them, but I think we can find a consistent path forward in the future.

For now, this AAVE module will be scuttled

I agree this requires more discussion and consensus. That's why I think that breaking the standard + doing it in a way that there is not way back shouldn't be the decision of a minority group. if I'm not wrong we were even more team members saying we prefer the standard approach (at least from the ones that raised our voice). Anyway it's done, there isn't anything more that I can do than expressing I did not agree with this.