fix: skip IBC escrow address beforesend hook #9549
Conversation
iboss-ptk
requested review from
mattverse,
nicolaslara and
p0mvn
as code owners
github-actions
bot
added
C:app-wiring
iboss-ptk
added
V:state/breaking
github-actions
bot
added
C:app-wiring
WalkthroughTokenFactory keeper now depends on an IBC ChannelKeeper. Keeper struct and constructor were extended to include ChannelKeeper. Before-send hook logic now suppresses errors when the sender is an IBC escrow address. Tests added for escrow-skip behavior and gas expectation adjusted. Types add ChannelKeeper interface. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as AppKeepers
participant TF as TokenFactoryKeeper
participant IBC as IBC ChannelKeeper
App->>TF: NewKeeper(..., channelKeeper=IBC, ...)
Note over TF: Keeper now holds ChannelKeeper reference
sequenceDiagram
autonumber
participant Sender as Sender
participant TF as TokenFactoryKeeper
participant IBC as ChannelKeeper
participant WASM as BeforeSendContract
Sender->>TF: TrackBeforeSend / callBeforeSendListener
alt channelKeeper available
TF->>IBC: GetAllChannels()
IBC-->>TF: [IdentifiedChannel...]
TF->>TF: IsIBCEscrowAddress(addr)?
alt Sender is IBC escrow
Note over TF: Suppress before-send errors
TF-->>Sender: nil (skip hook error)
else Not escrow
TF->>WASM: Execute before-send hook
alt Contract error
TF-->>Sender: propagate error (unless module/no-contract)
else Success
TF-->>Sender: nil
end
end
else No channelKeeper
TF->>WASM: Execute before-send hook (previous behavior)
TF-->>Sender: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
x/tokenfactory/keeper/before_send.go (1)
203-220: Minor: filter channels by port to cut unnecessary iterations.Only transfer port channels produce escrow addresses. Skip others for small savings.
func (k Keeper) IsIBCEscrowAddress(ctx sdk.Context, addr sdk.AccAddress) bool { @@ - for _, channel := range channels { + for _, channel := range channels { + if channel.PortId != transfertypes.PortID { + continue + } escrowAddr := transfertypes.GetEscrowAddress(channel.PortId, channel.ChannelId) if escrowAddr.Equals(addr) { return true } }x/tokenfactory/keeper/before_send_test.go (1)
318-373: Escrow skip test: good coverage.The channel setup and escrow address assertions exercise the intended path well. After implementing the early short‑circuit, consider asserting that no contract execution happens (if you can stub/meter it) to prevent regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/keepers/keepers.go(1 hunks)x/tokenfactory/keeper/before_send.go(2 hunks)x/tokenfactory/keeper/before_send_test.go(2 hunks)x/tokenfactory/keeper/keeper.go(3 hunks)x/tokenfactory/types/expected_keepers.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
x/tokenfactory/keeper/keeper.go (1)
x/tokenfactory/types/expected_keepers.go (1)
ChannelKeeper(47-49)
x/tokenfactory/keeper/before_send_test.go (2)
x/tokenfactory/types/msgs.go (2)
NewMsgCreateDenom(26-31)NewMsgSetBeforeSendHook(248-254)x/tokenfactory/types/expected_keepers.go (1)
ChannelKeeper(47-49)
app/keepers/keepers.go (2)
x/tokenfactory/types/expected_keepers.go (1)
ChannelKeeper(47-49)x/ibc-hooks/types/expected_keepers.go (1)
ChannelKeeper(10-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: go-split-test-files
- GitHub Check: e2e
- GitHub Check: Run golangci-lint
- GitHub Check: test
- GitHub Check: Summary
🔇 Additional comments (4)
x/tokenfactory/types/expected_keepers.go (1)
8-9: ChannelKeeper interface addition looks correct.Minimal surface area and sdk.Context usage are appropriate. Please just confirm ibc-go v8 ChannelKeeper exposes GetAllChannels with this exact signature.
Also applies to: 46-49
x/tokenfactory/keeper/keeper.go (1)
32-33: Keeper now depends on ChannelKeeper — dependency injection is fine.Field, constructor param, and assignment are consistent. Nil-safety is handled by callers via IsIBCEscrowAddress.
Also applies to: 44-45, 66-67
x/tokenfactory/keeper/before_send_test.go (1)
315-316: Gas number fragility.Hard-coding exact gas deltas can be brittle across toolchain/runtime bumps. If this starts flaking, prefer asserting within a tight range.
app/keepers/keepers.go (1)
536-544: Approve ChannelKeeper wiring in TokenFactoryKeeper
Verified there are no othertokenfactorykeeper.NewKeepercall sites needing the newChannelKeeperparameter.
| // Suppress errors when sending from IBC escrow addresses to prevent DoS attacks | ||
| if k.IsIBCEscrowAddress(ctx, from) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Short‑circuit: skip calling the hook entirely for IBC escrow senders.
Currently, the contract is still executed and errors are suppressed. This wastes up to BeforeSendHookGasLimit per coin and can amplify load during IBC unescrow. Skip the call up-front for escrow addresses (both Track and Block) and compute escrow once per call to avoid repeated channel scans.
Apply this diff:
func (k Keeper) callBeforeSendListener(context context.Context, from, to sdk.AccAddress, amount sdk.Coins, blockBeforeSend bool) (err error) {
@@
- ctx := sdk.UnwrapSDKContext(context)
+ ctx := sdk.UnwrapSDKContext(context)
+ // Compute once to avoid repeated channel scans
+ isEscrow := k.IsIBCEscrowAddress(ctx, from)
for _, coin := range amount {
cosmwasmAddress := k.GetBeforeSendHook(ctx, coin.Denom)
if cosmwasmAddress != "" {
+ // Skip hooks for IBC escrow-sourced sends to avoid DoS and wasted gas
+ if isEscrow {
+ if blockBeforeSend {
+ return nil
+ }
+ // For TrackBeforeSend, just skip processing
+ continue
+ }
cwAddr, err := sdk.AccAddressFromBech32(cosmwasmAddress)
if err != nil {
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Suppress errors when sending from IBC escrow addresses to prevent DoS attacks | |
| if k.IsIBCEscrowAddress(ctx, from) { | |
| return nil | |
| } | |
| func (k Keeper) callBeforeSendListener( | |
| context context.Context, | |
| from, to sdk.AccAddress, | |
| amount sdk.Coins, | |
| blockBeforeSend bool, | |
| ) (err error) { | |
| ctx := sdk.UnwrapSDKContext(context) | |
| // Compute once to avoid repeated channel scans | |
| isEscrow := k.IsIBCEscrowAddress(ctx, from) | |
| for _, coin := range amount { | |
| cosmwasmAddress := k.GetBeforeSendHook(ctx, coin.Denom) | |
| if cosmwasmAddress != "" { | |
| // Skip hooks for IBC escrow-sourced sends to avoid DoS and wasted gas | |
| if isEscrow { | |
| if blockBeforeSend { | |
| return nil | |
| } | |
| // For TrackBeforeSend, just skip processing this coin | |
| continue | |
| } | |
| cwAddr, err := sdk.AccAddressFromBech32(cosmwasmAddress) | |
| if err != nil { | |
| return err | |
| } | |
| // … rest of hook invocation logic unchanged … | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In x/tokenfactory/keeper/before_send.go around lines 191-194, avoid executing
the contract hook (and suppressing its errors) for IBC escrow senders and
prevent repeated expensive escrow-address checks per coin: compute a single
boolean isEscrow := k.IsIBCEscrowAddress(ctx, from) once at the start of
BeforeSend handling and if isEscrow is true return nil immediately (skip both
Track and Block hook paths); for non-escrow senders, reuse that boolean and
ensure you do not call IsIBCEscrowAddress inside per-coin loops or twice, so the
hook is only invoked when appropriate and escrow address scanning is done once
per call.
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
1 participant
Closes: CHAIN-708
What is the purpose of the change
According to the document, anyone can create a token and set a send hook, where
BlockBeforeSendcan causebankKeeper.SendCoinsto return an error.https://docs.osmosis.zone/overview/features/tokenfactory#bank-hooks-trackbeforesend-blockbeforesend
In the
OnRecvPacketof ibc transfer, when the chain reclaims the previously cross-chained token via ibc, it will callunescrowCoinand then callbankKeeper.SendCoins.Then, an attacker can set a send hook to make
unescrowCoinreturn an error, causing the relayer's message execution to fail. The relayer will repeatedly attempt the failed message, especially when the channel is ordered, eventually rendering the channel unusable and the relayer being subjected to a DoS attack.Testing and Verifying
Unit tested
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)