Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contract pausing #164

Merged
merged 74 commits into from
Feb 22, 2024
Merged

Contract pausing #164

merged 74 commits into from
Feb 22, 2024

Conversation

m-chrzan
Copy link
Contributor

@m-chrzan m-chrzan commented Sep 21, 2023

Description

Adds a mechanism for pausing the protocol to prevent/mitigate damages in case of an exploit.

The basic design is as follows:

  • Pausable contracts extend Pausable.sol
    • This adds two new storage slots, using the ERC-1967 pattern, rather than storage variables, which would mangle storage.
    • pauser is the address that is permissioned to pause/unpause the contract.
    • paused is a boolean indicating whether or not the contract is paused.
    • Functions that should be paused specify the onlyWhenNotPaused modifier.
  • Pauser.sol is a new contract, it should be set as the pauser address in Pausable contracts. It can only be called by the MultiSig.
  • MultiSig has new functions, pauseContracts and unpauseContracts, which pause/unpause a list of contracts.
  • pauseContracts can be called by any MultiSig signer to immediately pause the chosen contracts.
  • unpauseContracts can be called by Governance (via referendum or hotfix) to immediately unpause chosen contracts.
  • TODO: add pausing to other protocol contracts.
  • TODO: add pausing-related events.
  • TODO: Make Vote.sol pausable. This one has a different test setup than other contracts, making it more difficult to test. Will need to look into how to do this cleanly.

Tested

Unit tests.

Other changes

Set unlimited timeout for GroupHealth tests, as they started timing out in GitHub Actions.

Cleaned up some tests to use the named owner account and to explicitly specify the caller with .connect().

Vote needed some extra cleanup, as it used the production "core" fixture, differently than other contract unit tests. Updated to use a new test deployment.

Related issues

@m-chrzan m-chrzan marked this pull request as draft September 21, 2023 11:01
contracts/Account.sol Outdated Show resolved Hide resolved
contracts/Account.sol Outdated Show resolved Hide resolved
@m-chrzan m-chrzan marked this pull request as ready for review February 4, 2024 13:23
@m-chrzan m-chrzan changed the title [WIP] Contract pausing Contract pausing Feb 4, 2024
Copy link
Contributor

@soloseng soloseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, but in general looks good! 🚀

contracts/Pausable.sol Outdated Show resolved Hide resolved
contracts/Pausable.sol Outdated Show resolved Hide resolved
contracts/Account.sol Outdated Show resolved Hide resolved
contracts/StakedCelo.sol Outdated Show resolved Hide resolved
contracts/RebasedStakedCelo.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@pahor167 pahor167 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of having standalone smart contract as a pauser ?

We could have multisig as a Pauser (since multisig needs to set pauser anyways and only owner = multisig of pauser can actually pause the smart contract). If multisig would be the pauser, we could simplify the design quite a bit since pausing would be allowed only to owner of the smart contracts which is already a multisig.

@m-chrzan
Copy link
Contributor Author

What is the purpose of having standalone smart contract as a pauser ?

You're right, now that I think about it, I don't think it's necessary and could be simplified to put all the functionality in the MultiSig. I think this architecture was necessary when I had some different ideas about how to disable the MultiSig executing proposals and unpausing before, but shouldn't be necessary anymore.

I guess the one argument for keeping the separate contract would be for better modularity (e.g. if in the future we'd want to swap out our custom MultiSig for something like a Gnosis Safe).

Copy link
Contributor

@soloseng soloseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💚 Looks cleaner now without the Pauser contract!

test-ts/multisig.test.ts Show resolved Hide resolved
@m-chrzan m-chrzan merged commit fee3791 into master Feb 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow MultiSig to pause parts of the protocol without timelock
4 participants