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.

Sign up for GitHub

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

Ensure rate limits are not applied to packets that aren't ics20s #7357

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Ensure rate limits are not applied to packets that aren't ics20s #7357

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6669825
rate limits are not applied to packets that aren't ics20s
nicolaslara Jan 23, 2024
84ddfae
added changelog
nicolaslara Jan 23, 2024
3955de1
using proper ibc hooks tag
nicolaslara Jan 23, 2024
1b82d9d
removed unnecesary circular import
nicolaslara Jan 25, 2024
26c8600
reset go.mod
nicolaslara Jan 25, 2024
e653f73
lint
nicolaslara Jan 25, 2024
f27218c
added contains check
nicolaslara Feb 1, 2024
bee2f5c
Merge branch 'main' into nicolas/only-rate-limit-ics20
nicolaslara Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### State Breaking

* [#7181](https://github.com/osmosis-labs/osmosis/pull/7181) Improve errors for out of gas
* [#7357](https://github.com/osmosis-labs/osmosis/pull/7357) Fix: Ensure rate limits are not applied to packets that aren't ics20s

### Bug Fixes

Expand Down
20 changes: 20 additions & 0 deletions x/ibc-rate-limit/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ibc_rate_limit_test

import (
"fmt"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -542,3 +543,22 @@ func (suite *MiddlewareTestSuite) TestUnsetRateLimitingContract() {
// N.B.: this panics if validation fails.
paramSpace.SetParamSet(suite.chainA.GetContext(), &params)
}

// Test rate limits are reverted if a "send" fails
func (suite *MiddlewareTestSuite) TestNonICS20() {
suite.initializeEscrow()
// Setup contract
suite.chainA.StoreContractCode(&suite.Suite, "./bytecode/rate_limiter.wasm")
quotas := suite.BuildChannelQuota("weekly", "channel-0", sdk.DefaultBondDenom, 604800, 1, 1)
addr := suite.chainA.InstantiateRLContract(&suite.Suite, quotas)
suite.chainA.RegisterRateLimitingContract(addr)

osmosisApp := suite.chainA.GetOsmosisApp()

data := []byte("{}")
_, err := osmosisApp.RateLimitingICS4Wrapper.SendPacket(suite.chainA.GetContext(), capabilitytypes.NewCapability(1), "wasm.osmo1873ls0d60tg7hk00976teq9ywhzv45u3hk2urw8t3eau9eusa4eqtun9xn", "channel-0", clienttypes.NewHeight(0, 0), 1, data)

suite.Require().Error(err)
// This will error out, but not because of rate limiting
suite.Require().NotContains(err.Error(), "rate limit")
Comment on lines +562 to +563
Copy link
Member

Choose a reason for hiding this comment

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

Instead of notContains, could we just match the error that we get from this? Just in case we update the rate limit error in the future.

Is a nit though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same but from a different direction: if the underlying error changes in ibc, this will continue to pass. But I can add the other check when I untangle the uncommitted changes on my current branch

}
15 changes: 12 additions & 3 deletions x/ibc-rate-limit/ics4_wrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
package ibc_rate_limit

import (
"encoding/json"

errorsmod "cosmossdk.io/errors"

wasmkeeper "github.com/CosmWasm/wasmd/x/wasm/keeper"
sdk "github.com/cosmos/cosmos-sdk/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"

authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
porttypes "github.com/cosmos/ibc-go/v7/modules/core/05-port/types"
"github.com/cosmos/ibc-go/v7/modules/core/exported"

"github.com/osmosis-labs/osmosis/v22/x/ibc-rate-limit/types"
)

Expand Down Expand Up @@ -57,6 +59,13 @@ func NewICS4Middleware(
// If the contract param is not configured, or the contract doesn't have a configuration for the (channel+denom) being
// used, transfers are not prevented and handled by the wrapped IBC app
func (i *ICS4Wrapper) SendPacket(ctx sdk.Context, chanCap *capabilitytypes.Capability, sourcePort, sourceChannel string, timeoutHeight clienttypes.Height, timeoutTimestamp uint64, data []byte) (uint64, error) {
var packetdata transfertypes.FungibleTokenPacketData
if err := json.Unmarshal(data, &packetdata); err != nil {
return i.channel.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
}
if packetdata.Denom == "" || packetdata.Amount == "" {
return i.channel.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
}
contract := i.GetContractAddress(ctx)
if contract == "" {
// The contract has not been configured. Continue as usual
Expand Down