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

Add EIP: Circuit Breaker #7265

Closed
wants to merge 16 commits into from
Closed

Conversation

diyahir
Copy link

@diyahir diyahir commented Jul 3, 2023

This standard outlines a smart contract interface for a Circuit Breaker that triggers a temporary halt on protocol-wide token outflows when a threshold is exceeded for a predefined metric. This circuit breaker does not assume the structure of the underlying protocol, and mainly serves as a pass-through vehicle for token outflows. In order to maintain correct internal accounting for integrated protocols, and to provide maximum flexibility for developers, developers can specify if the circuit breaker contract should delay settlement and temporarily custody outflows during the cooldown period, or revert on attempted outflows.

  • Example implementation of a circuit breaker
  • Interface
  • 'ProtectedContract' Contract to be inherited by DeFi contract for CB best practices

@diyahir diyahir requested a review from eth-bot as a code owner July 3, 2023 06:55
@github-actions github-actions bot added c-new Creates a brand new proposal s-draft This EIP is a Draft labels Jul 3, 2023
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 3, 2023

File EIPS/eip-7265.md

Requires 1 more reviewers from @axic, @Pandapip1, @SamWilsn, @xinbenlv

@eth-bot eth-bot changed the title Add EIP: Circuit Breaker Standard Add EIP: Circuit Breaker Jul 3, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 3, 2023
@github-actions github-actions bot added the t-erc label Jul 3, 2023
@eth-bot eth-bot added e-consensus Waiting on editor consensus e-review Waiting on editor to review labels Jul 3, 2023
@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jul 3, 2023
@github-actions github-actions bot added the w-ci Waiting on CI to pass label Jul 3, 2023
@github-actions
Copy link

github-actions bot commented Jul 3, 2023

The commit 19807d5 (as a parent of 8e1f744) contains errors.
Please inspect the Run Summary for details.

@github-actions github-actions bot removed the w-ci Waiting on CI to pass label Jul 3, 2023
@hell0men
Copy link

hell0men commented Jul 4, 2023

It's a great idea. I think Forta Network Devs @dylankilkenny and other security analysts should be included in the discussion. Contract protection is their specialty.

@EnbangWu
Copy link

EnbangWu commented Jul 4, 2023

it seems like Admin have way privileged roles here,they can execute functions likeupdateAssetParams, overrideRateLimit startGracePeriod anytime , which may raise centralized risk concerns.

I think it might be a good idea to consider integrating with a timelock function into the major changes here. So that if the threshold is changed significantly and affect users getting their funds out, they can have a grace period to exist the system.

@monokh
Copy link

monokh commented Jul 4, 2023

Would there be some scope here to support more custom circuit breaking? Some protocols don't have inflow/outflows into their contract but they do facilitate exchange and volume. If the circuit breaker could be based on a number managed by the implementing contract, that could work.

@DZGoldman
Copy link

DZGoldman commented Jul 4, 2023

it seems like Admin have way privileged roles here,they can execute functions likeupdateAssetParams, overrideRateLimit
startGracePeriod anytime , which may raise centralized risk concerns.
I think it might be a good idea to consider integrating with a timelock function into the major changes here. So that if the threshold is changed significantly and affect users getting their funds out, they can have a grace period to exist the system.

The proposed standard is agnostic to what the admin is; i.e., it could, for a given app, be decentralized / only operate on a timelock. So I don't think the standard itself needs to be opinionated on this.

EIPS/eip-7265.md Outdated Show resolved Hide resolved
diyahir and others added 3 commits July 7, 2023 13:29
Co-authored-by: bobafetador <samglennss3@gmail.com>
syntax highlighting, thank you!

Co-authored-by: Francisco <fg@frang.io>
constructor(
address _admin,
uint256 _rateLimitCooldownPeriod,
uint256 _withdrawlPeriod,

Choose a reason for hiding this comment

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

typo in _withdrawlPeriod

totalChange += node.amount;
uint256 nextTimestamp = node.nextTimestamp;
// Clear data
limiter.listNodes[currentHead];

Choose a reason for hiding this comment

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

should be delete limiter.listNodes[currentHead]

Comment on lines +101 to +103
// If the list is empty, set the tail and head to current times
limiter.listHead = block.timestamp;
limiter.listTail = block.timestamp;

Choose a reason for hiding this comment

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

Why block.timestamp is used here and not currentTickTimestamp? It would be cleaner that way.

@@ -0,0 +1,16 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,53 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,362 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,198 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,151 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,17 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,50 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,385 @@
// SPDX-License-Identifier: UNLICENSED
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming you have the authority to change the license, we require a more permissive license like:

Suggested change
// SPDX-License-Identifier: UNLICENSED
// SPDX-License-Identifier: CC0-1.0

If you do not have authority to change the license, please remove this file.

@@ -0,0 +1,372 @@
---
eip: 7265
title: Circuit Breaker
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think your title gives enough detail. Perhaps something like:

Suggested change
title: Circuit Breaker
title: Asset Outflow Circuit Breaker Protection


## Motivation

Security breaches have become pervasive in DeFi. Over $6 Billion dollars worth of assets were stolen in exploits. When something goes wrong, protocols are rapidly drained of the majority of TVL often in a matter of seconds. Although many protocols have smart contracts which are fully upgradeable by governance, in practice there is usually no time to upgrade or patch vulnerabilities before large amounts of TVL are drained.
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally prefer that the first use of an abbreviation is expanded:

Suggested change
Security breaches have become pervasive in DeFi. Over $6 Billion dollars worth of assets were stolen in exploits. When something goes wrong, protocols are rapidly drained of the majority of TVL often in a matter of seconds. Although many protocols have smart contracts which are fully upgradeable by governance, in practice there is usually no time to upgrade or patch vulnerabilities before large amounts of TVL are drained.
Security breaches have become pervasive in DeFi. Over $6 Billion dollars worth of assets were stolen in exploits. When something goes wrong, protocols are rapidly drained of the majority of their total value locked (TVL) often in a matter of seconds. Although many protocols have smart contracts which are fully upgradeable by governance, in practice there is usually no time to upgrade or patch vulnerabilities before large amounts of TVL are drained.

@SamWilsn
Copy link
Contributor

I am closing this pull request because we are in the process of separating EIPs and ERCs into distinct repositories. Unfortunately, as far as we are aware, GitHub does not provide any tools to ease this migration, so every pull request will need to be re-opened manually.

As this is a PR to create / modify an ERC, I will kindly ask you to redirect this to the new repository at ethereum/ERCs. We have prepared a guide to help with the process.

If there is relevant history here, please link to this PR from the new pull request.

On behalf of the EIP Editors, I apologize for this inconvenience.

@SamWilsn SamWilsn closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal e-consensus Waiting on editor consensus e-review Waiting on editor to review s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.