Skip to content

Commit

Permalink
Fix audit feedback on committee creation (#16909)
Browse files Browse the repository at this point in the history
## 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
  • Loading branch information
dariorussi authored and patrickkuo committed May 9, 2024
1 parent 5bfeb3a commit 2aac099
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 0 deletions.
50 changes: 50 additions & 0 deletions crates/sui-framework/docs/bridge/committee.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ title: Module `0xb::committee`
- [Function `create`](#0xb_committee_create)
- [Function `verify_signatures`](#0xb_committee_verify_signatures)
- [Function `register`](#0xb_committee_register)
- [Function `check_uniqueness_bridge_keys`](#0xb_committee_check_uniqueness_bridge_keys)
- [Function `try_create_next_committee`](#0xb_committee_try_create_next_committee)
- [Function `execute_blocklist`](#0xb_committee_execute_blocklist)
- [Function `committee_members`](#0xb_committee_committee_members)
Expand Down Expand Up @@ -271,6 +272,15 @@ title: Module `0xb::committee`



<a name="0xb_committee_EDuplicatePubkey"></a>



<pre><code><b>const</b> <a href="committee.md#0xb_committee_EDuplicatePubkey">EDuplicatePubkey</a>: u64 = 8;
</code></pre>



<a name="0xb_committee_EDuplicatedSignature"></a>


Expand Down Expand Up @@ -457,12 +467,52 @@ title: Module `0xb::committee`
registration
};

// check uniqueness of the <a href="bridge.md#0xb_bridge">bridge</a> pubkey.
// `try_create_next_committee` will <b>abort</b> <b>if</b> bridge_pubkey_bytes are not unique and
// that will fail the end of epoch transaction (possibly "forever", well, we
// need <b>to</b> deploy proper <a href="../sui-system/validator.md#0x3_validator">validator</a> changes <b>to</b> stop end of epoch from failing).
<a href="committee.md#0xb_committee_check_uniqueness_bridge_keys">check_uniqueness_bridge_keys</a>(self, bridge_pubkey_bytes);

emit(registration)
}
</code></pre>



</details>

<a name="0xb_committee_check_uniqueness_bridge_keys"></a>

## Function `check_uniqueness_bridge_keys`



<pre><code><b>fun</b> <a href="committee.md#0xb_committee_check_uniqueness_bridge_keys">check_uniqueness_bridge_keys</a>(self: &<a href="committee.md#0xb_committee_BridgeCommittee">committee::BridgeCommittee</a>, bridge_pubkey_bytes: <a href="../move-stdlib/vector.md#0x1_vector">vector</a>&lt;u8&gt;)
</code></pre>



<details>
<summary>Implementation</summary>


<pre><code><b>fun</b> <a href="committee.md#0xb_committee_check_uniqueness_bridge_keys">check_uniqueness_bridge_keys</a>(self: &<a href="committee.md#0xb_committee_BridgeCommittee">BridgeCommittee</a>, bridge_pubkey_bytes: <a href="../move-stdlib/vector.md#0x1_vector">vector</a>&lt;u8&gt;) {
<b>let</b> <b>mut</b> count = self.member_registrations.size();
// bridge_pubkey_bytes must be found once and once only
<b>let</b> <b>mut</b> bridge_key_found = <b>false</b>;
<b>while</b> (count &gt; 0) {
count = count - 1;
<b>let</b> (_, registration) = self.member_registrations.get_entry_by_idx(count);
<b>if</b> (registration.bridge_pubkey_bytes == bridge_pubkey_bytes) {
<b>assert</b>!(!bridge_key_found, <a href="committee.md#0xb_committee_EDuplicatePubkey">EDuplicatePubkey</a>);
bridge_key_found = <b>true</b>; // bridge_pubkey_bytes found, we must not have another one
}
};
}
</code></pre>



</details>

<a name="0xb_committee_try_create_next_committee"></a>
Expand Down
51 changes: 51 additions & 0 deletions crates/sui-framework/packages/bridge/sources/committee.move
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ module bridge::committee {
const ESenderNotActiveValidator: u64 = 5;
const EInvalidPubkeyLength: u64 = 6;
const ECommitteeAlreadyInitiated: u64 = 7;
const EDuplicatePubkey: u64 = 8;

const SUI_MESSAGE_PREFIX: vector<u8> = b"SUI_BRIDGE_MESSAGE";

Expand Down Expand Up @@ -164,9 +165,33 @@ module bridge::committee {
registration
};

// check uniqueness of the bridge pubkey.
// `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).
check_uniqueness_bridge_keys(self, bridge_pubkey_bytes);

emit(registration)
}

// Assert if `bridge_pubkey_bytes` is duplicated in `member_registrations`.
// Dupicate keys would cause `try_create_next_committee` to fail and,
// in consequence, an end of epoch transaction to fail (safe mode run).
// This check will ensure the creation of the committee is correct.
fun check_uniqueness_bridge_keys(self: &BridgeCommittee, bridge_pubkey_bytes: vector<u8>) {
let mut count = self.member_registrations.size();
// bridge_pubkey_bytes must be found once and once only
let mut bridge_key_found = false;
while (count > 0) {
count = count - 1;
let (_, registration) = self.member_registrations.get_entry_by_idx(count);
if (registration.bridge_pubkey_bytes == bridge_pubkey_bytes) {
assert!(!bridge_key_found, EDuplicatePubkey);
bridge_key_found = true; // bridge_pubkey_bytes found, we must not have another one
}
};
}

// This method will try to create the next committee using the registration and system state,
// if the total stake fails to meet the minimum required percentage, it will skip the update.
// This is to ensure we don't fail the end of epoch transaction.
Expand Down Expand Up @@ -406,6 +431,32 @@ module bridge::committee {
test_scenario::end(scenario);
}

#[test]
#[expected_failure(abort_code = EDuplicatePubkey)]
fun test_init_committee_dup_pubkey() {
let mut scenario = test_scenario::begin(@0x0);
let ctx = test_scenario::ctx(&mut scenario);
let mut committee = create(ctx);

let validators = vector[
create_validator_for_testing(@0xA, 100, ctx),
create_validator_for_testing(@0xC, 100, ctx)
];
create_sui_system_state_for_testing(validators, 0, 0, ctx);
advance_epoch_with_reward_amounts(0, 0, &mut scenario);
test_scenario::next_tx(&mut scenario, @0x0);

let mut system_state = test_scenario::take_shared<SuiSystemState>(&scenario);

// validator registration
register(&mut committee, &mut system_state, hex::decode(VALIDATOR1_PUBKEY), b"", &tx(@0xA, 0));
register(&mut committee, &mut system_state, hex::decode(VALIDATOR1_PUBKEY), b"", &tx(@0xC, 0));

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

#[test]
fun test_init_committee_validator_become_inactive() {
let mut scenario = test_scenario::begin(@0x0);
Expand Down

0 comments on commit 2aac099

Please sign in to comment.