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

Logged targetClient object is always nil when error is returned #694

Closed
2 of 3 tasks
crodriguezvega opened this issue Jan 7, 2022 · 1 comment · Fixed by #721
Closed
2 of 3 tasks

Logged targetClient object is always nil when error is returned #694

crodriguezvega opened this issue Jan 7, 2022 · 1 comment · Fixed by #721
Assignees
Labels
audit Feedback from implementation audit core good first issue Good for newcomers

Comments

@crodriguezvega
Copy link
Contributor

crodriguezvega commented Jan 7, 2022

This issue was found by Trail of Bits during the audit of ICS27 Interchain Accounts

Problem Definition

The Keeper.ConnectionOpenTry function calls clienttypes.UnpackClientState to obtain a target client. When clienttypes.UnpackClientState returns an error, the received targetClient object is indicated in the error logs. However, clienttypes.UnpackClientState’s errors always indicate nil for the first returned value. As a result, the returned error will always indicate a nil target client. This reduces the utility of the error. Moreover, based on the error message format (and the %v format specifier), it appears that the developers may have intended the function to log the msg.ClientState instead of the targetClient.

Proposal

Correct the Keeper.ConnectionOpenTry function so that, on error, it returns a message with the intended object.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega crodriguezvega added 27-interchain-accounts audit Feedback from implementation audit labels Jan 7, 2022
@crodriguezvega crodriguezvega added this to the Interchain Accounts milestone Jan 7, 2022
@colin-axner colin-axner added the good first issue Good for newcomers label Jan 7, 2022
@crodriguezvega
Copy link
Contributor Author

We will not log when the error happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit Feedback from implementation audit core good first issue Good for newcomers
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants