search

makerdao / dss-gov

GNU Affero General Public License v3.0
4 stars 6 forks source link

Plot to pause #8

Closed gbalabasquer closed 4 years ago

gbalabasquer commented 4 years ago

I'm starting this PR to follow the discussion about transforming the DssChief from being an authority to a direct executor in the Pause.

This implementation considers the new DssPause will not have a tag for simplicity, thing that we can discuss if actually possible here: https://github.com/makerdao/dss-pause/issues/4

For dropping proposals the only viable idea that I had until now is having an "emergency" voting address (address(0)) which allows that any proposal can be dropped. It is the only way I can see how Governance could react in less than 12 hours to a malicious massive spam attack of bad proposals. Let me know your thoughts.

jparklev commented 4 years ago

Re voting on address(0) to drop proposals – what are your thoughts on, rather than having chief call only specific methods like pause.plot and pause.drop, having it make a generic smart contract call like pause.call(<calldata>)? Then a call to drop could be a normal proposal (which would probably have an id like keccak256(abi.encodePacked(pause, calldata)))

or, hmm, i guess eta becomes a problem then since it has to be calculated on the fly within chief for plot, but not for drop. Out of curiosity, why do we pass in the eta for plot anyway? Does anyone know what the argument was for not having pause calculate that for you?

gbalabasquer commented 4 years ago

What I could find about eta (which was not called with that name at that moment): https://github.com/dapphub/ds-pause/pull/8

There is also this tag value which I'm considering not to have it anymore. So definitely a simpler pause will need to happen if we go with this option.

gbalabasquer commented 4 years ago

I've added a simpler delay contract to this repo to continue exploring this idea. In this model the idea is the SpellAction is a contract with an execute() function that will be delegated call from the delay which is the contract that will have authorization to the mcd core ones.

gbalabasquer commented 4 years ago

Btw this is a very much draft so it is quite probably has bugs on the code, so please let me know if you find any.

gbalabasquer commented 4 years ago

About avoiding drop function as it is, not sure if I'm following what you propose. The problem of drops to specific proposals is that governance can not easily fight against a spam attack.

Let's say an attacker gets access to the system holding temporary majority, then can insert multiple proposals in the pause/delay just moving the voting from one side to other. Even if good governance recovers fast the control, it could be impossible to have to remove hundreds of bad plotted proposals in the pause/delay having to coordinate different people voting on each of these.

jparklev commented 4 years ago

What I could find about eta (which was not called with that name at that moment): dapphub/ds-pause#8

Nice, ty. "protects users from attacks where their transactions are replayed against a reorged / eclipsed chain" I certainly don't know enough about this to say for sure that this isn't a problem , but, from what i do understand, it seems like this wouldn't apply to us for eta if we're not passing a user-defined eta directly into plot. Does that sound right?

it could be impossible to have to remove hundreds of bad plotted proposals in the pause/delay having to coordinate different people voting on each of these

ahh fair point. That helps me understand the approach here. My only concern was from thinking that people would expect the zero address to represent an "abstain" since that's how some people do it in the current chief. But, that's probably not necessarily a great pattern to encourage anyway when you don't have to be voting for anything.

gbalabasquer commented 4 years ago

ahh fair point. That helps me understand the approach here. My only concern was from thinking that people would expect the zero address to represent an "abstain" since that's how some people do it in the current chief. But, that's probably not necessarily a great pattern to encourage anyway when you don't have to be voting for anything.

Yes maybe address(0) is not the best choice, we could use another custom address. In this chief, the way to abstain voting is actually keeping locked MKR and no voting anything.

gbalabasquer commented 4 years ago

Nice, ty. "protects users from attacks where their transactions are replayed against a reorged / eclipsed chain" I certainly don't know enough about this to say for sure that this isn't a problem , but, from what i do understand, it seems like this wouldn't apply to us for eta if we're not passing a user-defined eta directly into plot. Does that sound right?

Need to actually give a bit more thinking as I do not get 100% how this is useful yet.

gbalabasquer commented 4 years ago

Closing this PR as these changes were included in already merged snapshot branch.