Conversation
…oo long to be registered with amino
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8927 +/- ##
==========================================
- Coverage 66.52% 62.61% -3.91%
==========================================
Files 327 357 +30
Lines 17143 18567 +1424
==========================================
+ Hits 11404 11626 +222
- Misses 5048 6245 +1197
- Partials 691 696 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR introduces two new prototype modules: a TokenFactory (mint/burn/admin lifecycle for custom denoms) and an IFT (Inter-chain Fungible Token) bridge that burns tokens on the source chain, sends a GMP cross-chain call to mint them on the destination, and handles ack/timeout refunds via IBC callbacks.
Confidence Score: 2/5The IFT bridge's refund mechanism is broken: ack and timeout callbacks look up pending transfers by channel ID while the store is keyed by client ID, so every failed cross-chain transfer permanently destroys the sender's tokens. Three independent defects affect the correctness of the core token bridge flow. The callback key mismatch (SourceChannel vs clientID) means the refund path is entirely inoperative. The burn-before-sequence-persist ordering means a GMP response parse failure can result in an unrecoverable burn. The genesis validation bug blocks upgrades on any chain that has renounced at least one denom admin.
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pending, found, err := k.GetPendingTransferByClientSequence(cachedCtx, packet.SourceChannel, packet.Sequence) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !found { | ||
| k.Logger(cachedCtx).Debug("IFT ack callback: skipping non-IFT packet", | ||
| "clientId", packet.SourceChannel, | ||
| "sequence", packet.Sequence) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
packet.SourceChannel vs msg.ClientId key mismatch breaks refunds
PendingTransferStore is written in IFTTransfer with key (msg.ClientId, sequence) — a client ID such as "07-tendermint-0". The ack and timeout callbacks look up the same store using (packet.SourceChannel, sequence) — which in IBC v1 packet semantics is a channel ID such as "channel-0". These identifiers are distinct, so GetPendingTransferByClientSequence will always return found=false, the callbacks will silently log "skipping non-IFT packet", and every failed or timed-out transfer will permanently destroy the sender's tokens instead of refunding them.
| pending, found, err := k.GetPendingTransferByClientSequence(cachedCtx, packet.SourceChannel, packet.Sequence) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !found { | ||
| k.Logger(cachedCtx).Debug("IFT timeout callback: skipping non-IFT packet", | ||
| "clientId", packet.SourceChannel, | ||
| "sequence", packet.Sequence) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
packet.SourceChannel vs client ID mismatch in timeout callback
The timeout callback has the identical key mismatch as the ack callback (line 79): it looks up the pending transfer by packet.SourceChannel (channel ID) instead of the client ID used when writing the record. The end result is that timed-out transfers are never refunded.
| if _, err := sdk.AccAddressFromBech32(denom.AuthorityMetadata.Admin); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
ValidateGenesis rejects valid renounced-admin denoms
RenounceAdmin sets AuthorityMetadata.Admin to "". However, ValidateGenesis calls sdk.AccAddressFromBech32(denom.AuthorityMetadata.Admin) unconditionally, and AccAddressFromBech32("") returns an error. Any chain state containing a renounced-admin denom will fail genesis export/import, making upgrades impossible once any admin is renounced. The check should be skipped when Admin is empty.
| if _, err := sdk.AccAddressFromBech32(denom.AuthorityMetadata.Admin); err != nil { | |
| return err | |
| } | |
| if denom.AuthorityMetadata.Admin != "" { | |
| if _, err := sdk.AccAddressFromBech32(denom.AuthorityMetadata.Admin); err != nil { | |
| return err | |
| } | |
| } |
| if err := m.k.tokenFactoryKeeper.BurnFrom(ctx, msg.Denom, msg.Amount, sender); err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrBurnFailed, "failed to burn tokens: %s", err) | ||
| } | ||
|
|
||
| counterpartyInfo, found := m.k.ibcClientV2Keeper.GetClientCounterparty(ctx, msg.ClientId) | ||
| if !found { | ||
| return nil, errorsmod.Wrapf(types.ErrInvalidClientID, "counterparty for client %s not found", msg.ClientId) | ||
| } | ||
|
|
||
| // Construct mint call payload based on constructor type | ||
| var payload []byte | ||
| var encoding string | ||
| constructorType := types.ParseConstructorType(bridge.IftSendCallConstructor) | ||
|
|
||
| switch constructorType { | ||
| case types.ConstructorSolana: | ||
| encoding = gmptypes.EncodingProtobuf | ||
| constructor, err := types.NewSolanaConstructor(bridge.IftSendCallConstructor, bridge.CounterpartyIftAddress, m.k.GetModuleAddress().String(), counterpartyInfo.ClientId) | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrConstructMintCallFailed, "failed to build solana constructor: %s", err) | ||
| } | ||
| receiver := strings.TrimSpace(msg.Receiver) | ||
| payload, err = constructor.ConstructMintCall(m.k.cdc, receiver, msg.Amount, "", "") | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrConstructMintCallFailed, "failed to construct mint call: %s", err) | ||
| } | ||
|
|
||
| case types.ConstructorEVM: | ||
| var err error | ||
| payload, err = types.ConstructMintCall(m.k.cdc, msg.Receiver, msg.Amount, bridge.IftSendCallConstructor, "", "") | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrConstructMintCallFailed, "failed to construct mint call: %s", err) | ||
| } | ||
|
|
||
| case types.ConstructorCosmos: | ||
| // For CosmosTx, we need to know the ICA address that will execute the message | ||
| accountID := gmptypes.NewAccountIdentifier(msg.ClientId, m.k.GetModuleAddress().String(), nil) | ||
| icaAddr, err := gmptypes.BuildAddressPredictable(&accountID) | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrConstructMintCallFailed, "failed to compute ICA address: %s", err) | ||
| } | ||
| icaAddress := icaAddr.String() | ||
| payload, err = types.ConstructMintCall(m.k.cdc, msg.Receiver, msg.Amount, bridge.IftSendCallConstructor, msg.Denom, icaAddress) | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrConstructMintCallFailed, "failed to construct mint call: %s", err) | ||
| } | ||
|
|
||
| default: | ||
| return nil, errorsmod.Wrapf(types.ErrInvalidConstructorType, "unknown constructor type: %s", constructorType) | ||
| } | ||
|
|
||
| // Send via ICS27-GMP | ||
| sendMsg := &gmptypes.MsgSendCall{ | ||
| Sender: m.k.GetModuleAddress().String(), | ||
| SourceClient: bridge.ClientId, | ||
| Receiver: bridge.CounterpartyIftAddress, | ||
| Salt: nil, | ||
| Payload: payload, | ||
| TimeoutTimestamp: msg.TimeoutTimestamp, | ||
| Memo: "", | ||
| Encoding: encoding, | ||
| } | ||
|
|
||
| handler := m.k.msgRouter.Handler(sendMsg) | ||
| if handler == nil { | ||
| return nil, errorsmod.Wrap(types.ErrSendCallFailed, "no handler for MsgSendCall") | ||
| } | ||
|
|
||
| res, err := handler(ctx, sendMsg) | ||
| if err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrSendCallFailed, "failed to send GMP call: %s", err) | ||
| } | ||
|
|
||
| // Propagate events from the GMP handler (critical for relayer to detect send_packet) | ||
| ctx.EventManager().EmitEvents(res.GetEvents()) | ||
|
|
||
| // Extract sequence from response - fail if we cannot get the sequence | ||
| // since callbacks need it to match pending transfers | ||
| if len(res.MsgResponses) == 0 { | ||
| return nil, errorsmod.Wrap(types.ErrSendCallFailed, "no response from GMP send call") | ||
| } | ||
| var sendResp gmptypes.MsgSendCallResponse | ||
| if err := sendResp.Unmarshal(res.MsgResponses[0].Value); err != nil { | ||
| return nil, errorsmod.Wrapf(types.ErrSendCallFailed, "failed to unmarshal GMP response: %s", err) | ||
| } | ||
| sequence := sendResp.Sequence | ||
|
|
||
| // Store pending transfer | ||
| pending := types.PendingTransfer{ | ||
| Denom: msg.Denom, | ||
| ClientId: msg.ClientId, | ||
| Sequence: sequence, | ||
| Sender: msg.Signer, | ||
| Amount: msg.Amount, | ||
| } | ||
|
|
||
| if err := m.k.SetPendingTransfer(ctx, msg.ClientId, sequence, pending); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
IFTTransfer burns the caller's tokens (line 195) before dispatching the GMP MsgSendCall (line 263). If the handler dispatch succeeds but sendResp.Unmarshal fails (line 278), or if SetPendingTransfer returns an error (line 291), the function returns an error to the caller — but the tokens are already burned. The Cosmos SDK's transaction-level revert would undo the burn if the entire tx fails, but if the error originates inside the same tx (e.g. the handler writes state before the unmarshal fails) the revert semantics depend on whether a CacheContext was used. The operations should be reordered so the burn only occurs after the sequence is successfully obtained and persisted.
| func (k Keeper) MintTo(ctx context.Context, denom string, amount math.Int, to sdk.AccAddress) error { | ||
| coin := sdk.NewCoin(denom, amount) | ||
| if err := k.bankKeeper.MintCoins(ctx, types.ModuleName, sdk.NewCoins(coin)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, to, sdk.NewCoins(coin)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| sdkCtx.EventManager().EmitEvent( | ||
| sdk.NewEvent( | ||
| types.TypeEvtMint, | ||
| sdk.NewAttribute(types.AttributeKeyDenom, denom), | ||
| sdk.NewAttribute(types.AttributeKeyAmount, amount.String()), | ||
| sdk.NewAttribute(types.AttributeKeyMintTo, to.String()), | ||
| ), | ||
| ) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // BurnFrom burns tokens of denom from address. | ||
| // MUST fail if address does not have enough balance or burn is not permitted. | ||
| func (k Keeper) BurnFrom(ctx context.Context, denom string, amount math.Int, from sdk.AccAddress) error { | ||
| coin := sdk.NewCoin(denom, amount) | ||
| if err := k.bankKeeper.SendCoinsFromAccountToModule(ctx, from, types.ModuleName, sdk.NewCoins(coin)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := k.bankKeeper.BurnCoins(ctx, types.ModuleName, sdk.NewCoins(coin)); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| sdkCtx.EventManager().EmitEvent( | ||
| sdk.NewEvent( | ||
| types.TypeEvtBurn, | ||
| sdk.NewAttribute(types.AttributeKeyDenom, denom), | ||
| sdk.NewAttribute(types.AttributeKeyAmount, amount.String()), | ||
| sdk.NewAttribute(types.AttributeKeyBurnFrom, from.String()), | ||
| ), | ||
| ) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
MintTo and BurnFrom bypass admin authorization
The private mintToWithAdmin and burnFromWithAdmin helpers both call validateMintBurnPermission before touching the bank module. The public MintTo (line 16) and BurnFrom (line 41) exposed via the TokenFactoryKeeper interface skip that check entirely. Any module wired to this keeper can mint or burn an arbitrary registered denom without being its admin. This is intentional for the IFT bridge flow today, but as more modules adopt the same interface the authorization model becomes implicit and easy to violate in future integrations.
| return nil, err | ||
| } | ||
|
|
||
| receiverPubkey, _ := solana.PublicKeyFromBase58(receiver) |
There was a problem hiding this comment.
solana.PublicKeyFromBase58 at line 94
receiverPubkey, _ := solana.PublicKeyFromBase58(receiver) silently discards the error. The ValidateSolanaAddress(receiver) call two lines above would already have caught an invalid address, but the blank identifier here is a fragility: if the validation logic is ever relaxed or the code is reordered, a zero public key will be used without warning. Prefer capturing and returning the error for correctness.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for key := range wrapper { | ||
| return key | ||
| } |
dianab-cl
changed the title
2 participants
Description
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) if anything is changed.godoccomments if relevant.Files changedin the GitHub PR explorer.