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

ica: AuthenticateTx/executeTx clean up #490

Merged
merged 5 commits into from
Nov 4, 2021
Merged

ica: AuthenticateTx/executeTx clean up #490

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
735e2db
cleaning up AuthenticateTx and executeTx to reduce unnecessary comple…
damiannolan Oct 13, 2021
0c51aff
Merge branch 'interchain-accounts' into damian/447-ica-sendtx-nits
damiannolan Nov 3, 2021
8e5664e
adding error wrapping to AuthenticateTx
damiannolan Nov 3, 2021
71d050e
updating err msg to include expected signer
damiannolan Nov 3, 2021
2742355
Merge branch 'interchain-accounts' into damian/447-ica-sendtx-nits
damiannolan Nov 4, 2021
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
53 changes: 19 additions & 34 deletions modules/apps/27-interchain-accounts/keeper/relay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,60 +75,46 @@ func (k Keeper) createOutgoingPacket(
return packet.Sequence, nil
}

func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portId string) error {
seen := map[string]bool{}
var signers []sdk.AccAddress
for _, msg := range msgs {
for _, addr := range msg.GetSigners() {
if !seen[addr.String()] {
signers = append(signers, addr)
seen[addr.String()] = true
}
}
}

interchainAccountAddr, found := k.GetInterchainAccountAddress(ctx, portId)
// AuthenticateTx ensures the provided msgs contain the correct interchain account signer address retrieved
// from state using the provided controller port identifier
func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portID string) error {
interchainAccountAddr, found := k.GetInterchainAccountAddress(ctx, portID)
if !found {
return sdkerrors.ErrUnauthorized
return sdkerrors.Wrapf(types.ErrInterchainAccountNotFound, "failed to retrieve interchain account on port %s", portID)
}

for _, signer := range signers {
if interchainAccountAddr != signer.String() {
return sdkerrors.ErrUnauthorized
for _, msg := range msgs {
for _, signer := range msg.GetSigners() {
if interchainAccountAddr != signer.String() {
return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "failed to authenticate interchain account transaction")
Copy link
Contributor

Choose a reason for hiding this comment

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

should we print the expected signer?

Copy link
Contributor

@seantking seantking Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My instinct is no as it could potentially allow a malicious party to find out what the associated address they're trying to access is (I guess it's public anyway, but perhaps we shouldn't advertise it).

Maybe that is overly paranoid though?

Copy link
Contributor

@colin-axner colin-axner Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I follow the concern. The information is public and this would be in the case the controller chain sent a wrong message, so it might be useful to indicate why the message failed to authenticate. It cannot extract any more information than is already known

Copy link
Contributor

@seantking seantking Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I can't think of anything either, but unless there is a clear benefit to logging it out I usually air on the side of caution for these things. Is there any reason why knowing the associated address would be helpful (in terms of debugging I mean)?

Just an instinctual feeling 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems fine to me to include it. If anything it will help debug an issue if it comes to it. But in reality something has gone fundamentally wrong if we ever reach this error, in terms of handshake flow and data stored in state.

I'll update to the following:

return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "unexpected signer address: expected %s, got %s", interchainAccountAddr, signer.String())

Copy link
Contributor

@seantking seantking Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Fine with me then 👍

But in reality something has gone fundamentally wrong if we ever reach this error

Won't this error be reached if a malicious controller chain tries to put a signer address that isn't an interchain account they own?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point! 💯

}
}
}

return nil
}

func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) error {
err := k.AuthenticateTx(ctx, msgs, sourcePort)
if err != nil {
if err := k.AuthenticateTx(ctx, msgs, sourcePort); err != nil {
return err
}

for _, msg := range msgs {
err := msg.ValidateBasic()
if err != nil {
if err := msg.ValidateBasic(); err != nil {
return err
}
}

cacheContext, writeFn := ctx.CacheContext()
// CacheContext returns a new context with the multi-store branched into a cached storage object
// writeCache is called only if all msgs succeed, performing state transitions atomically
cacheCtx, writeCache := ctx.CacheContext()
for _, msg := range msgs {
_, msgErr := k.executeMsg(cacheContext, msg)
if msgErr != nil {
err = msgErr
break
if _, err := k.executeMsg(cacheCtx, msg); err != nil {
return err
}
}

if err != nil {
return err
}

// Write the state transitions if all handlers succeed.
writeFn()
writeCache()

return nil
}
Expand Down Expand Up @@ -157,8 +143,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error
return err
}

err = k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs)
if err != nil {
if err = k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs); err != nil {
return err
}

Expand Down
1 change: 0 additions & 1 deletion modules/apps/27-interchain-accounts/types/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ func (iapd InterchainAccountPacketData) ValidateBasic() error {
if len(iapd.Memo) > MaxMemoCharLength {
return sdkerrors.Wrapf(ErrInvalidOutgoingData, "packet data memo cannot be greater than %d characters", MaxMemoCharLength)
}
// TODO: add type validation when data type enum supports unspecified type
damiannolan marked this conversation as resolved.
Show resolved Hide resolved

return nil
}
Expand Down