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.

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

Check MsgChannelUpgradeInit is signed by authority #4186

Closed
crodriguezvega opened this issue Jul 27, 2023 · 3 comments
Closed

Check MsgChannelUpgradeInit is signed by authority #4186

crodriguezvega opened this issue Jul 27, 2023 · 3 comments
Assignees
Labels
channel-upgradability Channel upgradability feature gRPC Issues for gRPC endpoints

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jul 27, 2023

Reference: cosmos/ibc#999

Ideally we work on this after #4160 is merged, since that PR will bring the addition of authority to the core IBC keeper, and then the change would be quite simple:

if k.GetAuthority() != msg.Signer {
  return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "expected %s, got %s", k.GetAuthority(), msg.Signer)
}
@crodriguezvega crodriguezvega added channel-upgradability Channel upgradability feature gRPC Issues for gRPC endpoints labels Jul 27, 2023
@charleenfei charleenfei self-assigned this Aug 18, 2023
@charleenfei
Copy link
Contributor

I started working on this issue, but ran into complications while testing.

Basically, the way we currently run tests for the msg_server code relies on us passing the msg into the suite.chainA.SendMsgs(msg) function, which will call simapp.SignAndDeliver. However, the simapp function requires the private key of the signer in order to generate the mock signed tx. We typically grab from the TestChain struct because we have access to it as the sender account is generated from a private key which is custom generated for this use case and then saved in the TestChain struct.

However, in this testing case we set up the IBC keeper authority in the app.go to the gov module, but we do not have access to the private key for this account. Therefore, the test will fail when at the SignAndDeliver step because the private key will not match the sender account address.

one way around this would be to update the MsgChannelUpgradeInit to follow how MsgUpdateParams is set up, so that authority is a separate field on the msg from msg.Signer, and then we rely on the SDK proto message option extension to signal that the msg signer should be the IBC keeper authority, and have the check in the msg server on MsgChannelUpgradeInit just check against msg.Authority rather than checking against the msg.Signer field. The test would then look like the test for UpdateClientParams.

@damiannolan
Copy link
Member

We can modify the endpoint helper: Endpoint.ChanUpgradeInit to instead of routing the MsgChannelUpgradeInit to the application, route a new gov proposal containing the MsgChannelUpgradeInit.

Within the endpoint helper we can vote on the proposal and ensure it passes and executes the msg before returning.

@crodriguezvega
Copy link
Contributor Author

Closed by #4773

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channel-upgradability Channel upgradability feature gRPC Issues for gRPC endpoints
Projects
Archived in project
Development

No branches or pull requests

4 participants