Fix audit feedback on committee creation #16909

dariorussi · 2024-03-27T19:27:42Z

Description

As reported by audit if a validator registers with an already used pubkey for the bridge (whether maliciously or not) the creation of the committee will fail and as such an end of epoch would fail attempting to create the committee.
Moreover there is no way at the moment to flush the proposed committee (in that respect, should we have an API for it?) and it would require an emergency release to take care of the problem.
This should prevent the problem from happeing.

Test Plan

Added unit test

If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.

Type of Change (Check all that apply)

protocol change
user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

vercel · 2024-03-27T19:27:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 9:16pm
multisig-toolkit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 9:16pm
mysten-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 9:16pm
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 9:16pm
sui-kiosk	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 9:16pm
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 28, 2024 9:16pm

dariorussi · 2024-03-27T19:28:29Z

crates/sui-framework/packages/bridge/sources/committee.move

+        // `try_create_next_committee` will abort if bridge_pubkey_bytes are not unique and
+        // that will fail the end of epoch transaction (possibly "forever", well, we
+        // need to deploy proper validator changes to stop end of epoch from failing).
+        assert!(check_uniqueness_bridge_keys(self, bridge_pubkey_bytes), EDuplicatePubkey);


I could just call the API here and have the assert inside check_uniqueness_bridge_keys
if anybody feels strongly about it let me know

assert inside check_uniqueness_bridge_keys is slightly preferential to me, but not a strong opinion

manolisliolios · 2024-03-28T13:58:39Z

crates/sui-framework/packages/bridge/sources/committee.move

+        test_utils::destroy(committee);
+        test_scenario::return_shared(system_state);
+        test_scenario::end(scenario);


nit: You could avoid non-needed code on failure-expecting tests by aborting (abort 1337)

I could, just leaving that in line with the other tests.
Having said so and given I need to review tests and code coverage I may apply that story to all existing tests in that future PR.
Thanks for the hint, I keep forgetting that and go trough tricks every time I write tests :)

manolisliolios · 2024-03-28T14:07:56Z

crates/sui-framework/packages/bridge/sources/committee.move

+            if (registration.bridge_pubkey_bytes == bridge_pubkey_bytes) {
+                if (found) {
+                    return false
+                };
+                found = true;


nit: I'd probably pass down the sender's address too and check as the "found" thing messed with my brain (but could be me malfunctioning)

if (registration.bridge_pubkey_bytes == bridge_pubkey_bytes && registration.sui_address != sender ) { return false }

not sure I follow, the found is to make sure that bridge_pubkey_bytes is unique in the list of proposed committee upgrade.
I may be missing something here

it messed with my brain as well and could be me malfunctioning too.

renamed the field and added comments that should hopefully help fix the confusion

manolisliolios

LGTM

longbowlu · 2024-03-28T18:33:55Z

crates/sui-framework/packages/bridge/sources/committee.move

@@ -164,9 +165,35 @@ module bridge::committee {
            registration
        };

+        // check uniqeuness of the bridge pubkey.


longbowlu · 2024-03-28T18:35:22Z

crates/sui-framework/packages/bridge/sources/committee.move

+        // `try_create_next_committee` will abort if bridge_pubkey_bytes are not unique and
+        // that will fail the end of epoch transaction (possibly "forever", well, we
+        // need to deploy proper validator changes to stop end of epoch from failing).
+        assert!(check_uniqueness_bridge_keys(self, bridge_pubkey_bytes), EDuplicatePubkey);


assert inside check_uniqueness_bridge_keys is slightly preferential to me, but not a strong opinion

longbowlu · 2024-03-28T18:45:28Z

crates/sui-framework/packages/bridge/sources/committee.move

+            if (registration.bridge_pubkey_bytes == bridge_pubkey_bytes) {
+                if (found) {
+                    return false
+                };
+                found = true;


it messed with my brain as well and could be me malfunctioning too.

longbowlu · 2024-03-28T18:46:19Z

crates/sui-framework/packages/bridge/sources/committee.move

+        test_utils::destroy(committee);
+        test_scenario::return_shared(system_state);
+        test_scenario::end(scenario);


## Description As reported by audit if a validator registers with an already used pubkey for the bridge (whether maliciously or not) the creation of the committee will fail and as such an end of epoch would fail attempting to create the committee. Moreover there is no way at the moment to flush the proposed committee (in that respect, should we have an API for it?) and it would require an emergency release to take care of the problem. This should prevent the problem from happeing. ## Test Plan Added unit test --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes

dariorussi commented Mar 27, 2024

View reviewed changes

dariorussi requested review from patrickkuo, damirka, longbowlu, Bridgerz and manolisliolios March 27, 2024 19:28

dariorussi requested a review from ronny-mysten as a code owner March 28, 2024 13:00

vercel bot deployed to Preview – explorer March 28, 2024 13:00 View deployment

vercel bot deployed to Preview – sui-typescript-docs March 28, 2024 13:00 View deployment

vercel bot deployed to Preview – multisig-toolkit March 28, 2024 13:00 View deployment

vercel bot deployed to Preview – sui-kiosk March 28, 2024 13:01 View deployment

vercel bot deployed to Preview – sui-core March 28, 2024 13:01 View deployment

vercel bot deployed to Preview – mysten-ui March 28, 2024 13:01 View deployment

manolisliolios reviewed Mar 28, 2024

View reviewed changes

manolisliolios approved these changes Mar 28, 2024

View reviewed changes

longbowlu approved these changes Mar 28, 2024

View reviewed changes

dariorussi added 2 commits March 28, 2024 16:14

Fix audit feedback on committee creation

8876879

snapshots and other nonsense

11f9d9b

dariorussi force-pushed the bridge_reg branch from 2af4ac2 to 11f9d9b Compare March 28, 2024 21:14

vercel bot deployed to Preview – sui-typescript-docs March 28, 2024 21:15 View deployment

vercel bot deployed to Preview – sui-kiosk March 28, 2024 21:15 View deployment

vercel bot deployed to Preview – multisig-toolkit March 28, 2024 21:16 View deployment

vercel bot deployed to Preview – explorer March 28, 2024 21:16 View deployment

vercel bot deployed to Preview – mysten-ui March 28, 2024 21:16 View deployment

vercel bot deployed to Preview – sui-core March 28, 2024 21:16 View deployment

dariorussi merged commit d5346e7 into feature/native-bridge Mar 28, 2024
45 checks passed

dariorussi deleted the bridge_reg branch March 28, 2024 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix audit feedback on committee creation #16909

Fix audit feedback on committee creation #16909

dariorussi commented Mar 27, 2024

vercel bot commented Mar 27, 2024 •

edited

Loading

dariorussi Mar 27, 2024

longbowlu Mar 28, 2024

dariorussi Mar 28, 2024

manolisliolios Mar 28, 2024 •

edited

Loading

dariorussi Mar 28, 2024

longbowlu Mar 28, 2024

manolisliolios Mar 28, 2024

dariorussi Mar 28, 2024 •

edited

Loading

longbowlu Mar 28, 2024

dariorussi Mar 28, 2024

manolisliolios left a comment

longbowlu Mar 28, 2024

dariorussi Mar 28, 2024

longbowlu Mar 28, 2024

longbowlu Mar 28, 2024

longbowlu Mar 28, 2024

Fix audit feedback on committee creation #16909

Fix audit feedback on committee creation #16909

Conversation

dariorussi commented Mar 27, 2024

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manolisliolios Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariorussi Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manolisliolios left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Mar 27, 2024 •

edited

Loading

manolisliolios Mar 28, 2024 •

edited

Loading

dariorussi Mar 28, 2024 •

edited

Loading