2023-01-15 Protego Review

[The-Arbiter]

Hi, here is my Protego review. My foundry is a bit broken right now so I didn't double check if the tests work locally but I don't think I made any major functional changes.

Abstract and Rationale

Protego allows spell deployment with the intention of dropped existing plans (not spells). This can be used to mitigate damage from governance attacks. A plan is a scheduled delegatecall that can be permissionlessly executed. (Note that I'm unsure why PP exec is authed when there is a permissionless way to call exec - need context on that...)

The rationale for Protego is that a plan can only be dropped by authorised users (i.e. the chief). I presume that the issue here is that a 'spam' style attack by malicious governance would involve plotting more actions than the chief can conceivably drop in a given window. Protego would allow the 'spam' to be prevented by temporarily making dropping any plan permissionless. While this would stall malicious governance attacks, it would also allow any plan to be dropped permissionlessly and therefore halt all progress.

Protego also contains the ability to drop an individual plan (rather than making all drop operations permissionless via electing as the chief).

The core logic here is that deploy takes either a DsSpell or the plan arguments and then creates a Spell instance which can do three things:

• description() returns the hash of the plan being targeted by the drop spell
• planned() returns true if the spell is planned
• cast() calls drop on DsPause for the targeted plan (cancels the plan)

Overall, protego solves two problems: 1) Spell creation to drop a plan in Pause is now permissionless (i.e. anyone can create the spell which can then be voted to hat position, meaning that individual community members have the ability to easily 'veto' a spell if this were necessary). 2) Spam attacks by malicious governance can now be fought more effectively

Docs

> ameliorate

I added context since if this is meant to be permissionless it shouldn't take people an hour to understand what is going on and it is now more user-friendly for less technical folks. Use this at your discretion but I made it very easy to understand for normal folks (IMO).

Base Contract

• Interfaces OK
• NOTE Protego immutable protego; has no explicit visibility, should we use public or internal (default)?
• NOTE: bytes public sig; doesn't have the immutable tag but cannot be changed in any way; I assume this is Solidity not liking dynamically sized bytes being immutable? Or is there some other reasoning here?
• NOTE: Do we need description and planned in the drop spell? They're already in protego. It would save us 1 storage slot per drop spell (IMO I would have it in protego only) ==> I did this before in this branch but I added it back in, up to you if you want to remove it
• NOTE: I renamed 'NewDropSpell' to 'DropSpellCreated' and 'NewDrop' to 'DroppedPlan' to make the events reflect what actually happened
• I changed 'planned' in the comments to 'scheduled for execution' to reduce confusion
• NOTE: DssSpell in the comments, DsSpell per the code here - what do we want to use?
• Originally I didn't like how fax was sig and usr was action but I see that they refer to the values that the spell gives us and make a trust assumption. We could call it spell_usr or something since otherwise there is no differentiating factor for stuff like eta but I am OK with it as is.
• Everything else OK, I added some comments and changed some wording for use at your discretion

Test Contract

• Tests are pretty comprehensive and check for all cases I would check for (I added some comments)

[brianmcmichael]

• NOTE Protego immutable protego; has no explicit visibility, should we use public or internal (default)?

I don't see any harm in making this explicitly public.

• NOTE: bytes public sig; doesn't have the immutable tag but cannot be changed in any way; I assume this is Solidity not liking dynamically sized bytes being immutable? Or is there some other reasoning here?

bytes is an array under the hood, so can't be made immutable.

• NOTE: Do we need description and planned in the drop spell? They're already in protego. It would save us 1 storage slot per drop spell (IMO I would have it in protego only) ==> I did this before in this branch but I added it back in, up to you if you want to remove it

The child spell would presumably be added to a voting UI, vote.makerdao.com currently expects a spell description, and would be a necessary description for that purpose. Open to alternative return values though or making it an immutable.

• NOTE: I renamed 'NewDropSpell' to 'DropSpellCreated' and 'NewDrop' to 'DroppedPlan' to make the events reflect what actually happened

Probably want to just call these events Deploy and Drop to follow convention of naming events after function names. ex. https://github.com/makerdao/dss/blob/master/src/clip.sol#L106-L124

• NOTE: DssSpell in the comments, DsSpell per the code [here](https://github.com/dapphub/ds-spell/blob/master/src/spell.sol) - what do we want to use?

I view DsSpell as a dapphub construction that is minimally sufficient to interact with ds-pause, but a DssSpell as a PE team construction that follows naming conventions and expected functionality of officially-sanctioned spells. (returns eta(), action(), sig(), etc. )

[The-Arbiter]

I agree with the above; changes to event names and visibility are good. LGTM besides this for the time being.

[amusingaxl]

Closing this PR because it is going to be superseded by #2.