From d874d6db0f957036061f2c1b406e0382e90af44d Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Tue, 1 Nov 2022 13:08:01 -0700 Subject: [PATCH] Progress on IBC rate limit spec (#3190) * Progress on IBC rate limit spec * Fix CI * Fix lints * More updates * fix mdlinter * More notes * one more todo * More README update * Update x/ibc-rate-limit/README.md Co-authored-by: Adam Tucker Co-authored-by: Adam Tucker --- .github/workflows/contracts.yml | 4 +- .github/workflows/lint.yml | 7 +- .markdownlint.yml | 2 - tests/e2e/e2e_test.go | 2 +- x/ibc-rate-limit/README.md | 205 +++++++++++++++--- .../{testdata => bytecode}/rate_limiter.wasm | Bin x/ibc-rate-limit/testutil/wasm.go | 2 +- 7 files changed, 174 insertions(+), 48 deletions(-) rename x/ibc-rate-limit/{testdata => bytecode}/rate_limiter.wasm (100%) diff --git a/.github/workflows/contracts.yml b/.github/workflows/contracts.yml index 50dc9addb06..63031881121 100644 --- a/.github/workflows/contracts.yml +++ b/.github/workflows/contracts.yml @@ -12,11 +12,11 @@ on: jobs: test: - name: Test Suite + name: Test Cosmwasm Contracts runs-on: ubuntu-latest strategy: matrix: - contract: [{workdir: ./x/ibc-rate-limit/, output: testdata/rate_limiter.wasm, build: artifacts/rate_limiter-x86_64.wasm, name: rate_limiter}] + contract: [{workdir: ./x/ibc-rate-limit/, output: bytecode/rate_limiter.wasm, build: artifacts/rate_limiter-x86_64.wasm, name: rate_limiter}] steps: - name: Checkout sources diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 4baa5ff168e..2debe754e0b 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -54,9 +54,4 @@ jobs: # within `super-linter`. fetch-depth: 0 - name: Run documentation linter - uses: github/super-linter@v4 - env: - VALIDATE_ALL_CODEBASE: false - VALIDATE_MARKDOWN: true - DEFAULT_BRANCH: main - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: make mdlint diff --git a/.markdownlint.yml b/.markdownlint.yml index 926e6625649..cbe0c3ceea8 100644 --- a/.markdownlint.yml +++ b/.markdownlint.yml @@ -12,8 +12,6 @@ MD042: true MD048: true MD051: true # MD004: false -# # Can't disable MD007 :/ -# MD007: false # MD009: false # MD010: # code_blocks: false diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index ec98a290877..97be9690324 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -162,7 +162,7 @@ func (s *IntegrationTestSuite) TestIBCTokenTransferRateLimiting() { // co up two levels projectDir := filepath.Dir(filepath.Dir(wd)) fmt.Println(wd, projectDir) - err = copyFile(projectDir+"/x/ibc-rate-limit/testdata/rate_limiter.wasm", wd+"/scripts/rate_limiter.wasm") + err = copyFile(projectDir+"/x/ibc-rate-limit/bytecode/rate_limiter.wasm", wd+"/scripts/rate_limiter.wasm") s.NoError(err) node.StoreWasmCode("rate_limiter.wasm", initialization.ValidatorWalletName) chainA.LatestCodeId += 1 diff --git a/x/ibc-rate-limit/README.md b/x/ibc-rate-limit/README.md index f9a91cb8c1c..22a41d3d7f9 100644 --- a/x/ibc-rate-limit/README.md +++ b/x/ibc-rate-limit/README.md @@ -1,26 +1,119 @@ -# # IBC Rate Limit +# IBC Rate Limit -The ``IBC Rate Limit`` middleware implements an [IBC Middleware](https://github.com/cosmos/ibc-go/blob/f57170b1d4dd202a3c6c1c61dcf302b6a9546405/docs/ibc/middleware/develop.md) -that wraps a [transfer](https://ibc.cosmos.network/main/apps/transfer/overview.html) app to regulate how much value can -flow in and out of the chain for a specific denom and channel. +The IBC Rate Limit module is responsible for adding a governance-configurable rate limit to IBC transfers. +This is a safety control, intended to protect assets on osmosis in event of: -## Contents +* a bug/hack on osmosis +* a bug/hack on the counter-party chain +* a bug/hack in IBC itself -1. **[Concepts](#concepts)** -2. **[Parameters](#parameters)** -3. **[Contract](#contract)** -4. **[Integration](#integration)** +This is done in exchange for a potential (one-way) bridge liveness tradeoff, in periods of high deposits or withdrawals. -## Concepts +The architecture of this package is a minimal go package which implements an [IBC Middleware](https://github.com/cosmos/ibc-go/blob/f57170b1d4dd202a3c6c1c61dcf302b6a9546405/docs/ibc/middleware/develop.md) that wraps the [ICS20 transfer](https://ibc.cosmos.network/main/apps/transfer/overview.html) app, and calls into a cosmwasm contract. +The cosmwasm contract then has all of the actual IBC rate limiting logic. +The Cosmwasm code can be found in the [`contracts`](./contracts/) package, with bytecode findable in the [`bytecode`](./bytecode/) folder. The cosmwasm VM usage allows Osmosis chain governance to choose to change this safety control with no hard forks, via a parameter change proposal, a great mitigation for faster threat adaptavity. -### Overview +The status of the module is being in a state suitable for some initial governance settable rate limits for high value bridged assets. +Its not in its long term / end state for all channels by any means, but does act as a strong protection we +can instantiate today for high value IBC connections. -The `x/ibc-rate-limit` module implements an IBC middleware and a transfer app wrapper. The middleware checks if the -amount of value of a specific denom transferred through a channel has exceeded a quota defined by governance for -that channel/denom. These checks are handled through a CosmWasm contract. The contract to be used for this is -configured via a parameter. +## Motivation -### Middleware +The motivation of IBC-rate-limit comes from the empirical observations of blockchain bridge hacks that a rate limit would have massively reduced the stolen amount of assets in: + +- [Polynetwork Bridge Hack ($611 million)](https://rekt.news/polynetwork-rekt/) +- [BNB Bridge Hack ($586 million)](https://rekt.news/bnb-bridge-rekt/) +- [Wormhole Bridge Hack ($326 million)](https://rekt.news/wormhole-rekt/) +- [Nomad Bridge Hack ($190 million)](https://rekt.news/nomad-rekt/) +- [Harmony Bridge Hack ($100 million)](https://rekt.news/harmony-rekt/) - (Would require rate limit + monitoring) +- [Dragonberry IBC bug](https://forum.cosmos.network/t/ibc-security-advisory-dragonberry/7702) (can't yet disclose amount at risk, but was saved due to being found first by altruistic Osmosis core developers) + +In the presence of a software bug on Osmosis, IBC itself, or on a counterparty chain, we would like to prevent the bridge from being fully depegged. +This stems from the idea that a 30% asset depeg is ~infinitely better than a 100% depeg. +Its _crazy_ that today these complex bridged assets can instantly go to 0 in event of bug. +The goal of a rate limit is to raise an alert that something has potentially gone wrong, allowing validators and developers to have time to analyze, react, and protect larger portions of user funds. + +The thesis of this is that, it is worthwile to sacrifice liveness in the case of legitimate demand to send extreme amounts of funds, to prevent the terrible long-tail full fund risks. +Rate limits aren't the end-all of safety controls, they're merely the simplest automated one. More should be explored and added onto IBC! + +## Rate limit types + +We express rate limits in time-based periods. +This means, we set rate limits for (say) 6-hour, daily, and weekly intervals. +The rate limit for a given time period stores the relevant amount of assets at the start of the rate limit. +Rate limits are then defined on percentage terms of the asset. +The time windows for rate limits are currently _not_ rolling, they have discrete start/end times. + +We allow setting separate rate limits for the inflow and outflow of assets. +We do all of our rate limits based on the _net flow_ of assets on a channel pair. This prevents DOS issues, of someone repeatedly sending assets back and forth, to trigger rate limits and break liveness. + +We currently envision creating two kinds of rate limits: + +* Per denomination rate limits + - allows safety statements like "Only 30% of Stars on Osmosis can flow out in one day" or "The amount of Atom on Osmosis can at most double per day". +* Per channel rate limits + - Limit the total inflow and outflow on a given IBC channel, based on "USDC" equivalent, using Osmosis as the price oracle. + +We currently only implement per denomination rate limits for non-native assets. We do not yet implement channel based rate limits. + +Currently these rate limits automatically "expire" at the end of the quota duration. TODO: Think of better designs here. E.g. can we have a constant number of subsequent quotas start filled? Or perhaps harmonically decreasing amounts of next few quotas pre-filled? Halted until DAO override seems not-great. + +## Instantiating rate limits + +Today all rate limit quotas must be set manually by governance. +In the future, we should design towards some conservative rate limit to add as a safety-backstop automatically for channels. +Ideas for how this could look: + +* One month after a channel has been created, automatically add in some USDC-based rate limit +* One month after governance incentivizes an asset, add on a per-denomination rate limit. + +Definitely needs far more ideation and iteration! + +## Parameterizing the rate limit + +One element is we don't want any rate limit timespan thats too short, e.g. not enough time for humans to react to. So we wouldn't want a 1 hour rate limit, unless we think that if its hit, it could be assessed within an hour. + +### Handling rate limit boundaries + +We want to be safe against the case where say we have a daily rate limit ending at a given time, and an adversary attempts to attack near the boundary window. +We would not like them to be able to "double extract funds" by timing their extraction near a window boundary. + +Admittedly, not a lot of thought has been put into how to deal with this well. +Right now we envision simply handling this by saying if you want a quota of duration D, instead include two quotas of duration D, but offset by `D/2` from each other. + +Ideally we can change windows to be more 'rolling' in the future, to avoid this overhead and more cleanly handle the problem. (Perhaps rolling ~1 hour at a time) + +### Inflow parameterization + +The "Inflow" side of a rate limit is essentially protection against unforeseen bug on a counterparty chain. +This can be quite conservative (e.g. bridged amount doubling in one week). This covers a few cases: + +* Counter-party chain B having a token theft attack + - TODO: description of how this looks +* Counter-party chain B runaway mint + - TODO: description of how this looks +* IBC theft + - TODO: description of how this looks + +It does get more complex when the counterparty chain is itself a DEX, but this is still much more protection than nothing. + +### Outflow parameterization + +The "Outflow" side of a rate limit is protection against a bug on Osmosis OR IBC. +This has potential for much more user-frustrating issues, if set too low. +E.g. if theres some event that causes many people to suddenly withdraw many STARS or many USDC. + +So this parameterization has to contend with being a tradeoff of withdrawal liveness in high volatility periods vs being a crucial safety rail, in event of on-Osmosis bug. + +TODO: Better fill out + +### Example suggested parameterization + +## Code structure + +As mentioned at the beginning of the README, the go code is a relatively minimal ICS 20 wrapper, that dispatches relevant calls to a cosmwasm contract that implements the rate limiting functionality. + +### Go Middleware To achieve this, the middleware needs to implement the `porttypes.Middleware` interface and the `porttypes.ICS4Wrapper` interface. This allows the middleware to send and receive IBC messages by wrapping @@ -28,23 +121,14 @@ any IBC module, and be used as an ICS4 wrapper by a transfer module (for sending Of those interfaces, just the following methods have custom logic: -* `ICS4Wrapper.SendPacket` adds tracking of value sent via an ibc channel -* `Middleware.OnRecvPacket` adds tracking of value received via an ibc channel -* `Middleware.OnAcknowledgementPacket` undos the tracking of a sent packet if the acknowledgment is not a success -* `OnTimeoutPacket` undos the tracking of a sent packet if the packet times out (is not relayed) +* `ICS4Wrapper.SendPacket` forwards to contract, with intent of tracking of value sent via an ibc channel +* `Middleware.OnRecvPacket` forwards to contract, with intent of tracking of value received via an ibc channel +* `Middleware.OnAcknowledgementPacket` forwards to contract, with intent of undoing the tracking of a sent packet if the acknowledgment is not a success +* `OnTimeoutPacket` forwards to contract, with intent of undoing the tracking of a sent packet if the packet times out (is not relayed) All other methods from those interfaces are passthroughs to the underlying implementations. -### Contract Concepts - -The tracking contract uses the following concepts - -1. **RateLimit** - tracks the value flow transferred and the quota for a path. -2. **Path** - is a (denom, channel) pair. -3. **Flow** - tracks the value that has moved through a path during the current time window. -4. **Quota** - is the percentage of the denom's total value that can be transferred through the path in a given period of time (duration) - -## Parameters +#### Parameters The middleware uses the following parameters: @@ -55,23 +139,45 @@ The middleware uses the following parameters: 1. **ContractAddress** - The contract address is the address of an instantiated version of the contract provided under `./contracts/` -## Contract +### Cosmwasm Contract Concepts + +Something to keep in mind with all of the code, is that we have to reason separately about every item in the following matrix: + +| Native Token | Non-Native Token | +|----------------------|--------------------------| +| Send Native Token | Send Non-Native Token | +| Receive Native Token | Receive Non-Native Token | +| Timeout Native Send | Timeout Non-native Send | -### Messages +(Error ACK can reuse the same code as timeout) + +TODO: Spend more time on sudo messages in the following description. We need to better describe how we map the quota concepts onto the code. +Need to describe how we get the quota beginning balance, and that its different for sends and receives. +Explain intracacies of tracking that a timeout and/or ErrorAck must appear from the same quota, else we ignore its update to the quotas. + + +The tracking contract uses the following concepts + +1. **RateLimit** - tracks the value flow transferred and the quota for a path. +2. **Path** - is a (denom, channel) pair. +3. **Flow** - tracks the value that has moved through a path during the current time window. +4. **Quota** - is the percentage of the denom's total value that can be transferred through the path in a given period of time (duration) + +#### Messages The contract specifies the following messages: -#### Query +##### Query * GetQuotas - Returns the quotas for a path -#### Exec +##### Exec * AddPath - Adds a list of quotas for a path * RemovePath - Removes a path * ResetPathQuota - If a rate limit has been reached, the contract's governance address can reset the quota so that transfers are allowed again -#### Sudo +##### Sudo Sudo messages can only be executed by the chain. @@ -79,7 +185,7 @@ Sudo messages can only be executed by the chain. * RecvPacket - Increments the amount used out of the receive quota and checks that the receive is allowed. If it isn't, it will return a RateLimitExceeded error * UndoSend - If a send has failed, the undo message is used to remove its cost from the send quota -## Integration +### Integration The rate limit middleware wraps the `transferIBCModule` and is added as the entry route for IBC transfers. @@ -87,3 +193,30 @@ The module is also provided to the underlying `transferIBCModule` as its `ICS4Wr pointed to a channel, which also implements the `ICS4Wrapper` interface. This integration can be seen in [osmosis/app/keepers/keepers.go](https://github.com/osmosis-labs/osmosis/blob/main/app/keepers/keepers.go) + +## Testing strategy + +TODO: Fill this out + +## Known Future work + +Items that've been highlighted above: + +* Making automated rate limits get added for channels, instead of manual configuration only +* Improving parameterization strategies / data analysis +* Adding the USDC based rate limits +* We need better strategies for how rate limits "expire". + +Not yet highlighted + +* Making monitoring tooling to know when approaching rate limiting and when they're hit +* Making tooling to easily give us summaries we can use, to reason about "bug or not bug" in event of rate limit being hit +* Enabling ways to pre-declare large transfers so as to not hit rate limits. + * Perhaps you can on-chain declare intent to send these assets with a large delay, that raises monitoring but bypasses rate limits? + * Maybe contract-based tooling to split up the transfer suffices? +* Strategies to account for high volatility periods without hitting rate limits + * Can imagine "Hop network" style markets emerging + * Could imagine tieng it into looking at AMM volatility, or off-chain oracles + * but these are both things we should be wary of security bugs in. + * Maybe [constraint based programming with tracking of provenance](https://youtu.be/HB5TrK7A4pI?t=2852) as a solution +* Analyze changing denom-based rate limits, to just overall withdrawal amount for Osmosis \ No newline at end of file diff --git a/x/ibc-rate-limit/testdata/rate_limiter.wasm b/x/ibc-rate-limit/bytecode/rate_limiter.wasm similarity index 100% rename from x/ibc-rate-limit/testdata/rate_limiter.wasm rename to x/ibc-rate-limit/bytecode/rate_limiter.wasm diff --git a/x/ibc-rate-limit/testutil/wasm.go b/x/ibc-rate-limit/testutil/wasm.go index 2beabb9c02a..e9506059786 100644 --- a/x/ibc-rate-limit/testutil/wasm.go +++ b/x/ibc-rate-limit/testutil/wasm.go @@ -19,7 +19,7 @@ func (chain *TestChain) StoreContractCode(suite *suite.Suite) { osmosisApp := chain.GetOsmosisApp() govKeeper := osmosisApp.GovKeeper - wasmCode, err := ioutil.ReadFile("./testdata/rate_limiter.wasm") + wasmCode, err := ioutil.ReadFile("./bytecode/rate_limiter.wasm") suite.Require().NoError(err) addr := osmosisApp.AccountKeeper.GetModuleAddress(govtypes.ModuleName)