items from first channel upgrades security audit session #5664

crodriguezvega · 2024-01-18T23:02:30Z

Description

I have created this branch where we can tackle the issues from the audit. @DimitrisJim @charleenfei please pick any items from the list and push to this branch, I thought doing it like this would be easier and faster than opening individual issues (and then we only need to backport one PR). I think the issues are small enough that should be easy to review all of them in one PR.

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

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.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

codecov-commenter · 2024-01-18T23:36:37Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (40564ed) 81.16% compared to head (61bfcd5) 81.17%.
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5664   +/-   ##
=======================================
  Coverage   81.16%   81.17%           
=======================================
  Files         199      199           
  Lines       15278    15286    +8     
=======================================
+ Hits        12401    12409    +8     
  Misses       2408     2408           
  Partials      469      469

Files	Coverage Δ
modules/core/04-channel/keeper/keeper.go	`90.77% <100.00%> (ø)`
modules/core/04-channel/keeper/packet.go	`99.12% <100.00%> (+<0.01%)`	⬆️
modules/core/04-channel/keeper/upgrade.go	`92.27% <100.00%> (+0.04%)`	⬆️
modules/core/04-channel/types/upgrade.go	`100.00% <100.00%> (ø)`
modules/core/keeper/msg_server.go	`66.81% <90.90%> (-0.19%)`	⬇️
modules/core/24-host/packet_keys.go	`0.00% <0.00%> (ø)`

DimitrisJim · 2024-01-19T07:14:15Z

sgtm, would prefer PRs since its an easier workflow to not make mistakes in but lets give this a shot. Would be nice to sync up via slack when people plan to push so we don't end up with weird conflicts. also no force pushing!

Do not create a new errChanUpgradeFailed for logging/returning, it obfuscates the logic and does not yield much benefit.

Wrap the error before returning as is done in ChannelUpgradeInit.

…erations.

…voked _before_ core state is written.

…, add fmt.Errorf wrap test case.

modules/core/04-channel/types/upgrade_test.go

Move log into the if to only log if channel state actually changes. Could be moved outside for both cases but might then be logging when channel state doesn't change.

DimitrisJim · 2024-01-19T15:55:44Z

I see we've done all things from the list so opening this up for review/e2e run. Feel free to make draft if I missed something.

crodriguezvega

Changes look great to me, pending the decision on the order of calling the callback. Great work @DimitrisJim and @charleenfei completing all the audit issues in less than 24 hours! 🦾

modules/core/04-channel/keeper/upgrade.go

chatton

Looks great thank you! I left a couple of small suggestions

modules/core/04-channel/keeper/upgrade.go

modules/core/04-channel/keeper/upgrade_test.go

modules/core/04-channel/types/upgrade_test.go

chatton · 2024-01-22T12:16:23Z

modules/core/04-channel/types/upgrade.go

+	for {
+		_, ok := err.(*UpgradeError)
+		if ok {
+			return true
+		}
+
+		if err = errors.Unwrap(err); err == nil {
+			return false
+		}
+	}


the presence of this function, means we can actually update the Unwrap fn of the UpgradeError type, I believe it can just be

// Unwrap returns the base error that caused the upgrade to fail. func (u *UpgradeError) Unwrap() error { return errors.Unwrap(u.err) }

And the corresponding test can be updated to

// TestUpgradeErrorUnwrap tests that the underlying error is not modified when Unwrap is called. func (suite *TypesTestSuite) TestUpgradeErrorUnwrap() { baseUnderlyingError := errorsmod.Wrap(types.ErrInvalidChannel, "base error") wrappedErr := errorsmod.Wrap(baseUnderlyingError, "wrapped error") upgradeError := types.NewUpgradeError(1, wrappedErr) originalUpgradeError := upgradeError.Error() unWrapped := errors.Unwrap(upgradeError) postUnwrapUpgradeError := upgradeError.Error() suite.Require().Equal(baseUnderlyingError, unWrapped, "unwrapped error was not equal to base underlying error") suite.Require().Equal(originalUpgradeError, postUnwrapUpgradeError, "original error was modified when unwrapped") }

I think I like this better since it becomes more of a direct passthrough.

this would return nil on unwrapping a simple wrapped err no? UpgradeError { err: types.ErrInvalidChannel } would call errors.Unwrap(types.ErrInvalidChannel) which returns nil yea?

Yeah, nil is returned if err is just a single unwrapped error in that case

DimitrisJim · 2024-01-22T13:58:11Z

I can take care of addressing review(s)

Co-authored-by: Cian Hatton <cian@interchain.io>

- Prefer using SetChannelState over look-up channel, modify, set. - Add deeply test case with error wrapped multiple times.

colin-axner · 2024-01-22T15:54:41Z

modules/core/04-channel/keeper/upgrade.go

 		channel.State = types.FLUSHCOMPLETE
 		k.SetChannel(ctx, portID, channelID, channel)
+		k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", channel.State)


Hmm, I guess the reason we had to the logger at the bottom is that startFlushing might move the channel state from OPEN to FLUSHING, but, the previous channel state also could have been FLUSHING. A bit of a rock and a hard place

modules/core/04-channel/keeper/upgrade.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

colin-axner

Thanks everyone pushing commits! Big thanks to @DimitrisJim for following up on review comments!

damiannolan

Thank you all! 🚀

damiannolan · 2024-01-23T09:25:19Z

modules/core/04-channel/types/upgrade.go

+	for {
+		_, ok := err.(*UpgradeError)
+		if ok {
+			return true
+		}
+
+		if err = errors.Unwrap(err); err == nil {
+			return false
+		}
+	}


Yeah, nil is returned if err is just a single unwrapped error in that case

AdityaSripal

Nice work!

* some items from the security audit * update tests for cancelling when channel is in FLUSHCOMPLETE * remove duplicate test * nit: Move wrapping of returned error to direct return statement. Do not create a new errChanUpgradeFailed for logging/returning, it obfuscates the logic and does not yield much benefit. * nit: consistency in err return cbs.OnChanUpgradeTry Wrap the error before returning as is done in ChannelUpgradeInit. * add comment * nit: Call application callbacks before performing any state change operations. * chore: Add documentation indicating to app devs that callbacks are invoked _before_ core state is written. * upgrade error * nit: Add note on handling wrapped error in IsUpgradeError docu string, add fmt.Errorf wrap test case. * Update modules/core/04-channel/types/upgrade_test.go * chore: Use consistent logging in writeUpgradeAck + writeUpgradeConfirm Move log into the if to only log if channel state actually changes. Could be moved outside for both cases but might then be logging when channel state doesn't change. * Apply suggestions from code review Co-authored-by: Cian Hatton <cian@interchain.io> * nit: review comments from Cian. - Prefer using SetChannelState over look-up channel, modify, set. - Add deeply test case with error wrapped multiple times. * nit: Address Colin's comment * nit: just linter thangs. * Apply suggestions from code review Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * nit: Fix linter flatten if else. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Charly <charly@interchain.berlin> Co-authored-by: Charly <charly@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> (cherry picked from commit 41501a2) # Conflicts: # docs/docs/01-ibc/06-channel-upgrades.md

) (#5688) * items from first channel upgrades security audit session (#5664) * some items from the security audit * update tests for cancelling when channel is in FLUSHCOMPLETE * remove duplicate test * nit: Move wrapping of returned error to direct return statement. Do not create a new errChanUpgradeFailed for logging/returning, it obfuscates the logic and does not yield much benefit. * nit: consistency in err return cbs.OnChanUpgradeTry Wrap the error before returning as is done in ChannelUpgradeInit. * add comment * nit: Call application callbacks before performing any state change operations. * chore: Add documentation indicating to app devs that callbacks are invoked _before_ core state is written. * upgrade error * nit: Add note on handling wrapped error in IsUpgradeError docu string, add fmt.Errorf wrap test case. * Update modules/core/04-channel/types/upgrade_test.go * chore: Use consistent logging in writeUpgradeAck + writeUpgradeConfirm Move log into the if to only log if channel state actually changes. Could be moved outside for both cases but might then be logging when channel state doesn't change. * Apply suggestions from code review Co-authored-by: Cian Hatton <cian@interchain.io> * nit: review comments from Cian. - Prefer using SetChannelState over look-up channel, modify, set. - Add deeply test case with error wrapped multiple times. * nit: Address Colin's comment * nit: just linter thangs. * Apply suggestions from code review Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> * nit: Fix linter flatten if else. --------- Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com> Co-authored-by: Charly <charly@interchain.berlin> Co-authored-by: Charly <charly@interchain.io> Co-authored-by: Cian Hatton <cian@interchain.io> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> (cherry picked from commit 41501a2) # Conflicts: # docs/docs/01-ibc/06-channel-upgrades.md * nit: rm docs. --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

some items from the security audit

cec32d1

crodriguezvega requested review from colin-axner, AdityaSripal and damiannolan as code owners January 18, 2024 23:02

crodriguezvega changed the title ~~items from first security audit session~~ items from first channel upgrades security audit session Jan 18, 2024

crodriguezvega marked this pull request as draft January 18, 2024 23:04

crodriguezvega added the backport-to-v8.1.x label Jan 18, 2024

crodriguezvega added 2 commits January 19, 2024 00:27

update tests for cancelling when channel is in FLUSHCOMPLETE

ef28df7

remove duplicate test

0b76ef2

DimitrisJim and others added 9 commits January 19, 2024 09:19

nit: Move wrapping of returned error to direct return statement.

24cdd1d

Do not create a new errChanUpgradeFailed for logging/returning, it obfuscates the logic and does not yield much benefit.

nit: consistency in err return cbs.OnChanUpgradeTry

d9abb10

Wrap the error before returning as is done in ChannelUpgradeInit.

add comment

5807f05

nit: Call application callbacks before performing any state change op…

5d98593

…erations.

chore: Add documentation indicating to app devs that callbacks are in…

fcb8aac

…voked _before_ core state is written.

Merge branch 'main' into channel-upgrades/security-audit-day1

edc8f99

upgrade error

ac1ca8c

Merge branch 'main' into channel-upgrades/security-audit-day1

63f05fd

nit: Add note on handling wrapped error in IsUpgradeError docu string…

e13a382

…, add fmt.Errorf wrap test case.

DimitrisJim reviewed Jan 19, 2024

View reviewed changes

modules/core/04-channel/types/upgrade_test.go Outdated Show resolved Hide resolved

DimitrisJim added 2 commits January 19, 2024 17:17

Update modules/core/04-channel/types/upgrade_test.go

76b7ec9

chore: Use consistent logging in writeUpgradeAck + writeUpgradeConfirm

52542e5

Move log into the if to only log if channel state actually changes. Could be moved outside for both cases but might then be logging when channel state doesn't change.

DimitrisJim marked this pull request as ready for review January 19, 2024 15:55

DimitrisJim requested review from charleenfei, chatton and srdtrk as code owners January 19, 2024 15:55

crodriguezvega commented Jan 19, 2024

View reviewed changes

crodriguezvega added the priority PRs that need prompt reviews label Jan 21, 2024

crodriguezvega added this to the 04-channel upgrades RC milestone Jan 21, 2024

Merge branch 'main' into channel-upgrades/security-audit-day1

2dc7ebd

colin-axner reviewed Jan 22, 2024

View reviewed changes

modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved

chatton reviewed Jan 22, 2024

View reviewed changes

DimitrisJim self-assigned this Jan 22, 2024

DimitrisJim and others added 5 commits January 22, 2024 16:35

Merge branch 'main' into channel-upgrades/security-audit-day1

ff35306

Apply suggestions from code review

31cbfde

Co-authored-by: Cian Hatton <cian@interchain.io>

nit: review comments from Cian.

dbd8a28

- Prefer using SetChannelState over look-up channel, modify, set. - Add deeply test case with error wrapped multiple times.

nit: Address Colin's comment

9242b05

nit: just linter thangs.

aa9a71b

colin-axner reviewed Jan 22, 2024

View reviewed changes

modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved

modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved

modules/core/04-channel/keeper/upgrade.go Outdated Show resolved Hide resolved

DimitrisJim and others added 3 commits January 22, 2024 18:16

Apply suggestions from code review

a0705e6

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

nit: Fix linter flatten if else.

20db85c

Merge branch 'main' into channel-upgrades/security-audit-day1

b781477

colin-axner approved these changes Jan 22, 2024

View reviewed changes

Merge branch 'main' into channel-upgrades/security-audit-day1

61bfcd5

damiannolan approved these changes Jan 23, 2024

View reviewed changes

AdityaSripal approved these changes Jan 23, 2024

View reviewed changes

Merge branch 'main' into channel-upgrades/security-audit-day1

87c5344

DimitrisJim enabled auto-merge (squash) January 23, 2024 10:46

DimitrisJim merged commit 41501a2 into main Jan 23, 2024
63 of 64 checks passed

DimitrisJim deleted the channel-upgrades/security-audit-day1 branch January 23, 2024 10:49

mergify bot mentioned this pull request Jan 23, 2024

items from first channel upgrades security audit session (backport #5664) #5688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

items from first channel upgrades security audit session #5664

items from first channel upgrades security audit session #5664

crodriguezvega commented Jan 18, 2024 •

edited

Loading

codecov-commenter commented Jan 18, 2024 •

edited

Loading

DimitrisJim commented Jan 19, 2024

DimitrisJim commented Jan 19, 2024

crodriguezvega left a comment

chatton left a comment

chatton Jan 22, 2024

DimitrisJim Jan 22, 2024

damiannolan Jan 23, 2024

DimitrisJim commented Jan 22, 2024

colin-axner Jan 22, 2024

colin-axner left a comment

damiannolan left a comment

damiannolan Jan 23, 2024

AdityaSripal left a comment

items from first channel upgrades security audit session #5664

items from first channel upgrades security audit session #5664

Conversation

crodriguezvega commented Jan 18, 2024 • edited Loading

Description

Commit Message / Changelog Entry

codecov-commenter commented Jan 18, 2024 • edited Loading

Codecov Report

DimitrisJim commented Jan 19, 2024

DimitrisJim commented Jan 19, 2024

crodriguezvega left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

chatton Jan 22, 2024

Choose a reason for hiding this comment

DimitrisJim Jan 22, 2024

Choose a reason for hiding this comment

damiannolan Jan 23, 2024

Choose a reason for hiding this comment

DimitrisJim commented Jan 22, 2024

colin-axner Jan 22, 2024

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

damiannolan Jan 23, 2024

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

crodriguezvega commented Jan 18, 2024 •

edited

Loading

codecov-commenter commented Jan 18, 2024 •

edited

Loading