-
Notifications
You must be signed in to change notification settings - Fork 2
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 DSM and modularize CircuitBreaker #3
Add DSM and modularize CircuitBreaker #3
Conversation
Use latest Ownable contract version
Looks like the tests are no longer passing @Keinberger @Philogy Error (2973): Wrong argument count for modifier invocation: 1 arguments given but expected 0.
--> src/core/CircuitBreaker.sol:67:7:
|
67 | ) Ownable(_initialOwner) {
| ^^^^^^^^^^^^^^^^^^^^^^ To test run, you may need to delete the lib or run forge install openzeppelin/openzeppelin-contracts --no-git
forge install foundry-rs/forge-std --no-git
forge test |
Hey @diyahir ! Also make sure to run If the issue still persists:
git clone https://github.com/ikigai-labs-xyz/defi-circuit-breaker-eips.git
forge install Openzeppelin/openzeppelin-contracts
forge install foundry-rs/forge-std --no-git
forge update This should solve the issue :) |
Works now! Thanks! It's super cool to have a layer of abstraction that fits the goals of the token limiter, I am really liking the way it's laid out logically. A few comments:
A lof these things we had already came to a consensus on, so unless there's a compelling reason to omit/change them, I don't think we should deviate too much, particularly on grace periods, overriding limits, and global limits |
Hey again @diyahir ! Thanks very much for the thorough feedback, really appreciate it. We are very happy that you like our modularized approach. In regards to the points:
All in all, we very much welcome your feedback, we are happy that you like our overall approach. We also do not want to deviate from decisions, where consensus has already been found previously. These should definitely stay the same! |
…reaker add override circuit breaker
Rename TokenCircuitBreaker to AssetCircuitBreaker
Code-wise this is just an additional check here as well as in the DSM location (actually it would be better to refactor this): if (!firewallTriggered || _inGracePeriod())
_safeTransferIncludingNative(_token, _recipient, _amount);
else
DSM()
function onNativeAssetOutflow(
address _recipient
) external payable override onlyProtected onlyOperational returns bool {
bool firewallTriggered = _decreaseParameter(
getTokenIdentifier(NATIVE_ADDRESS_PROXY),
msg.value,
_recipient,
msg.value,
new bytes(0)
);
...
return firewallTriggered
}
I would expect most protocols to strictly use the token inflow and outflow metrics since at the very end of the day, the only thing you care about is the assets are not stolen quickly through a hack. Even in your GMX example, the price oracle manipulation's goal is to steal assets, so the standard Token CB would suffice. The oracle change can and will be manipulated as much as possible to steal the assets, so creating heuristics around that would not work super well (for example, stop everything if oracle moves quickly by 50% in 5 minutes or whatever, the hacker will just do up to 49% and use that margin to steal everything). So the ultimate backstop is inflow and outflows because the attack surface is infinite. If there is a way to create any margin, then that is a window to steal everything basically. Grace periods, overrides, and limits should be global parameters in the reference implementation because this is the expected behavior the majority of protocols will want. For the reference implementation, we are trying to keep it as robust, simple, intuitive, and minimalist as possible while meeting the expectations and needs of the majority of protocols. Super interesting to have good separations on a technical level, but this is really a power-user level of controls that most protocols won't need/want. |
Hey again @diyahir ! Thanks for your feedback, we added back the grace period, cooldown period and global Regarding your last point, we agree that the primary use case is tracking token inflow and outflow metrics, and the implementation should simplify this process as much as possible. Using CircuitBreaker in Defi is still very much an experiment. We have assumption about the way protocols are going to use this piece of infrastructure, but at this stage we think our standard should be as future-proof as possible concerning potential use cases. This PR is already big enough, so let's not address everything simultaneously :) |
I agree with not fixing everything here, there is one issue I see with the way the grace period was implemented: function overrideRateLimit() external onlyOwner {
if (!isRateLimited) revert CircuitBreaker__NotRateLimited();
isRateLimited = false;
// Allow the grace period to extend for the full withdrawal period to not trigger rate limit again
// if the rate limit is removed just before the withdrawal period ends
gracePeriodEndTimestamp = lastRateLimitTimestamp + WITHDRAWAL_PERIOD;
} This is susceptible to a 'false flag -> hack' attack. The hacker triggers the rate limit, starts the grace period, and steals everything while in the grace period. So overriding the limit should not trigger a grace period, instead overriding should basically sync the tvl to the current time such that now the BPS drawdown has been reset. The difference is that in this case overriding a rate limit does not make the protocol susceptible to an instant attack and just 'resets' the drawdown window. I know a lot of these sound silly, but if you give a hacker an inch, he will take a mile. I do agree we can merge this once this is fixed (we can just delete that line), and have a base for people to improve upon with the improved refactoring and separation. |
Add DSM implementation and modularize CircuitBreaker
@vdaubry and me have been working the past few weeks to implement the new CircuitBreaker specifications and optimize modularity. We believe the EIP should provide a modular standard for anyone to extend upon and build individualized implementations.
The main characteristic of hacks is “liquidity being drained in a short time frame”. That’s why it makes sense to track that metric on-chain and introduce a CircuitBreaker standard to provide a way to handle unusual TVL activity. Nevertheless, there are multiple other metrics that are also highly indicative of hacks, most and foremost unusual price changes within AMM’s or other protocol specific values that should not be able to change by unrealistic amounts under normal circumstances.
That lead us to our conclusion: The CircuitBreaker standard should provide a way to track multiple metrics on-chain. It might be token outflows, but it should also enable protocols to track other metrics, that are crucial for protocol health. Protocol’s should not be bound to only tracking TVL.
Furthermore, that will allow others to build extensions on top of the CircuitBreaker for their specific needs.
Crucially, this philosophy is very common when it comes to EIP’s. Two of the most prominent, EIP20 and EIP721, both follow this philosophy: A minimalistic base implementation, that maximizes compatibility and modularization to allow others to build individualistic implementations and extensions.
The new approach in a nutshell🥜
The new CircuitBreaker base implementation allows the protocol to configure security parameters (a more generic naming for tokenLimiter) for their protocol. Each security parameter has a unique
bytes32
identifier (previously a token address). This still allows protocol’s to configure it to track token addresses, but it also allows protocols to track other metrics. Each security parameter also has to be configured with aSettlementModule
contract. This contract gets called to “settle” transactions whenever the CircuitBreaker gets triggered. The CircuitBreaker will automatically call theprevent
function of the SettlementModule. It is up to the SettlementModule implementation to specify what will happen. The prevent function can be coded to always revert, schedule a future transfer (delay the settlement) or do something else. This allows protocols to implement highly individualistic settlement strategies for different metrics. Delaying the transfer or reverting are not the only options anymore. Protocol’s could also choose to let governance settle the transactions or implement other settlement strategies.And the cool thing about this: Our proposed approach is perfectly capable of doing the exact same thing as the original CircuitBreaker did beforehand: It can track token in- & outflows and delay or revert potential hack transactions. Even the same, existing Foundry tests pass with our proposed approach.
To summarize: We are just modularizing the CircuitBreaker to allow for more customization. It still achieves the existing goals of delaying or reverting transactions.
What has been changed
Our code changes followed the philosophy of introducing minimal code changes to the existing code, while still implementing our new approach.
EIP7265 Interface
AssetRegistered
event toSecurityParameterAdded
AssetInflow
event toParameterIncrease
AssetWithdraw
event toParameterDecrease
AssetRateLimitBreached
event toRateLimited
registerAsset
function toaddSecurityParameter
updateAssetParams
function toupdateSecurityParameter
onTokenInflow
function toincreaseParameter
onTokenOutflow
function todecreaseParameter
onNativeAssetOutflow
function is being handled bydecreaseParameter
overrideRateLimit
SettlementModule
implementationoverrideExpiredRateLimit
SettlementModule
implementationCircuitBreaker.sol
This is the new base CircuitBreaker implementation. The new contract only has 236 lines instead of 349 and allows for more modularization.
Changes
bytes32
as the key instead of an addressOwnable
for more security when it comes to admin handling_decreaseParameter
and_increaseParameter
functions call the prevent function on the SettlementModule in case of the CircuitBreaker being triggered. The prevent function implementation then specifies how to handle the trigger situation, as opposed to locking the funds by default._safeTransferIncludingNative
New: ISettlementModule.sol
The interface defines generic method for a Settlement module. We provided 2 sample settlement modules:
Protocol can choose which SettlementModule to attach to each security parameter they’re tracking.
But protocol's can also provide their own SettlementModule if they need to.
New: IDelayedSettlementModule.sol + DelayedSettlementModule.sol
This implementation is a thin wrapper around OpenZeppelin TimelockController. The DelayedSettlementModule prevent transaction from going through by locking funds in a TimeLock. Funds can be unlocked by the “execute” function.
New: ITokenCircuitBreaker.sol
This interface implements the existing event and function naming of the EIP for the TokenCircuitBreaker extension.
New: TokenCircuitBreaker.sol
This contract is a wrapper for the CircuitBreaker contract. This wrapper allows for implementing the existing core functionality: Tracking asset inflows and outflows. It showcases how our proposed new CircuitBreaker contract can be extended with only minimal changes to allow for the same functionality.