Skip to content

Commit dece503

Browse files
authored
fix(drive-abci)!: refuse IdentityCreate identity id and key proof failures before they consume the asset lock (#5431)
1 parent c1649d8 commit dece503

11 files changed

Lines changed: 1217 additions & 56 deletions

File tree

‎book/src/state-transitions/validation-pipeline.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,16 @@ version 14 a failure is therefore refused without a fee, like a failed witness,
311311
charging the inputs a penalty; check_tx runs this stage for the transition too, so such a
312312
transition does not reach a block.
313313

314+
For `IdentityCreate` this stage checks that the identity id is the one the asset lock's outpoint
315+
derives, the structure of the keys, and the proof of possession of each key. The asset lock
316+
signature covers the keys but neither the identity id nor the key signatures, which are left
317+
out of the signable bytes. From protocol version 14 an identity id or proof of possession
318+
failure is therefore refused without a fee instead of charging the asset lock a penalty, and
319+
the asset lock is left as it was; a key structure failure is still charged. A block holding
320+
such a transition is refused, as for any unpaid result. `check_tx` runs this stage for
321+
`IdentityCreate` too, at every protocol version, so such a transition does not reach a block; it
322+
checks the identity id first, before verifying the asset lock proof.
323+
314324
## Stage 10: State Validation
315325

316326
The final validation stage checks for state-level conflicts. Does a data contract

‎packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs‎

Lines changed: 32 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -3649,9 +3649,33 @@ mod tests {
36493649
}
36503650
.into();
36513651

3652-
let mut identity_create_transition: StateTransition =
3652+
// An identity without a master key fails the key structure check. The asset lock key
3653+
// signs the keys, so that failure is charged to the asset lock, which is then partially
3654+
// used
3655+
let (critical_key_in_place_of_master, critical_private_key_in_place_of_master) =
3656+
IdentityPublicKey::random_ecdsa_critical_level_authentication_key(
3657+
0,
3658+
Some(23),
3659+
platform_version,
3660+
)
3661+
.expect("expected to get key pair");
3662+
3663+
signer.add_identity_public_key(
3664+
critical_key_in_place_of_master.clone(),
3665+
critical_private_key_in_place_of_master,
3666+
);
3667+
3668+
let identity_without_master_key: Identity = IdentityV0 {
3669+
id: identifier,
3670+
public_keys: BTreeMap::from([(0, critical_key_in_place_of_master), (1, key.clone())]),
3671+
balance: 1000000000,
3672+
revision: 0,
3673+
}
3674+
.into();
3675+
3676+
let identity_create_transition: StateTransition =
36533677
IdentityCreateTransition::try_from_identity_with_signer_and_private_key(
3654-
&identity,
3678+
&identity_without_master_key,
36553679
asset_lock_proof.clone(),
36563680
pk.as_slice(),
36573681
&signer,
@@ -3662,21 +3686,6 @@ mod tests {
36623686
.await
36633687
.expect("expected an identity create transition");
36643688

3665-
let valid_identity_create_transition = identity_create_transition.clone();
3666-
3667-
// let's add an error so this fails on state validation
3668-
3669-
if let StateTransition::IdentityCreate(identity_create_transition_inner) =
3670-
&mut identity_create_transition
3671-
{
3672-
// let's create a new key that isn't signed
3673-
let master_key = identity_create_transition_inner
3674-
.public_keys_mut()
3675-
.get_mut(0)
3676-
.expect("expected master key");
3677-
master_key.set_signature(vec![].into());
3678-
};
3679-
36803689
let identity_create_serialized_transition = identity_create_transition
36813690
.serialize_to_bytes()
36823691
.expect("serialized state transition");
@@ -3689,7 +3698,7 @@ mod tests {
36893698
let transaction = platform.drive.grove.start_transaction();
36903699

36913700
let validation_result = platform
3692-
.execute_tx(identity_create_serialized_transition, &transaction)
3701+
.execute_tx(identity_create_serialized_transition.clone(), &transaction)
36933702
.expect("expected to execute identity_create tx");
36943703
assert!(matches!(validation_result, UnsuccessfulPaidExecution(..)));
36953704

@@ -3700,41 +3709,28 @@ mod tests {
37003709
.unwrap()
37013710
.expect("expected to commit transaction");
37023711

3703-
// now lets try to recreate the valid identity
3704-
3705-
// This one will use the balance from the transaction outpoint that was already saved
3706-
3707-
let valid_identity_create_serialized_transition = valid_identity_create_transition
3708-
.serialize_to_bytes()
3709-
.expect("serialized state transition");
3710-
3712+
// The same signed bytes again are a replay, so this is unpaid
37113713
let transaction = platform.drive.grove.start_transaction();
37123714

37133715
let validation_result = platform
3714-
.execute_tx(valid_identity_create_serialized_transition, &transaction)
3716+
.execute_tx(identity_create_serialized_transition, &transaction)
37153717
.expect("expected to execute identity_create tx");
37163718

3717-
platform
3718-
.drive
3719-
.grove
3720-
.commit_transaction(transaction)
3721-
.unwrap()
3722-
.expect("expected to commit transaction");
3723-
3724-
// this is unpaid because it will look like a replay attack
37253719
assert!(matches!(
37263720
validation_result,
37273721
UnpaidConsensusExecutionError(..)
37283722
));
37293723

3724+
// The valid identity's keys differ from the refused one's, so its signable bytes are not
3725+
// a replay; it uses the balance left in the partially used asset lock
37303726
let valid_identity_create_transition: StateTransition =
37313727
IdentityCreateTransition::try_from_identity_with_signer_and_private_key(
37323728
&identity,
37333729
asset_lock_proof,
37343730
pk.as_slice(),
37353731
&signer,
37363732
&NativeBlsModule,
3737-
1,
3733+
0,
37383734
platform_version,
37393735
)
37403736
.await
@@ -3750,7 +3746,6 @@ mod tests {
37503746
.execute_tx(valid_identity_create_serialized_transition, &transaction)
37513747
.expect("expected to execute identity_create tx");
37523748

3753-
// the user fee increase changed, so this is now passing
37543749
assert!(matches!(validation_result, SuccessfulPaidExecution(..)));
37553750

37563751
platform

‎packages/rs-drive-abci/src/execution/validation/state_transition/check_tx_verification/v0/mod.rs‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use crate::platform_types::platform::PlatformRef;
88
use crate::platform_types::check_tx_proof_verifier::CheckTxProofVerifier;
99
use crate::platform_types::platform_state::PlatformStateV0Methods;
1010
use crate::rpc::core::CoreRPCLike;
11-
use dpp::identity::state_transition::OptionallyAssetLockProved;
11+
use dpp::identity::state_transition::{AssetLockProved, OptionallyAssetLockProved};
1212
use dpp::prelude::ConsensusValidationResult;
1313
use dpp::serialization::Signable;
1414
use dpp::state_transition::signable_bytes_hasher::SignableBytesHasher;
@@ -42,7 +42,10 @@ use drive::state_transition_action::StateTransitionAction;
4242
use std::collections::BTreeMap;
4343
use dpp::address_funds::PlatformAddress;
4444
use dpp::fee::Credits;
45+
use dpp::consensus::basic::invalid_identifier_error::InvalidIdentifierError;
46+
use dpp::consensus::basic::BasicError;
4547
use dpp::identity::PartialIdentity;
48+
use dpp::state_transition::identity_create_transition::accessors::IdentityCreateTransitionAccessorsV0;
4649
use dpp::prelude::AddressNonce;
4750

4851
#[allow(clippy::too_many_arguments)]
@@ -207,6 +210,28 @@ pub(super) fn state_transition_to_execution_event_for_check_tx_v0<'a, C: CoreRPC
207210
}
208211
}
209212

213+
// An identity create's identity id must be the one its asset lock's outpoint
214+
// derives, and a block refuses one whose id is not. The id is outside the bytes the
215+
// asset lock key signs, so it is compared here, before the asset lock proof is
216+
// verified, and such a transition is refused without that verification. Basic
217+
// structure has checked that the outpoint exists. Admission is not consensus, so
218+
// every protocol version gets this.
219+
if let StateTransition::IdentityCreate(identity_create) = state_transition {
220+
if identity_create.asset_lock_proof().create_identifier()?
221+
!= identity_create.identity_id()
222+
{
223+
return Ok(
224+
ConsensusValidationResult::<Option<ExecutionEvent>>::new_with_error(
225+
BasicError::InvalidIdentifierError(InvalidIdentifierError::new(
226+
"identity_id".to_string(),
227+
"does not match created identifier from asset lock".to_string(),
228+
))
229+
.into(),
230+
),
231+
);
232+
}
233+
}
234+
210235
// Validate minimum fee for shielded spending transitions (stateless, uses public
211236
// value_balance). This is cheaper than proof verification so we check it first.
212237
// Only applies to ShieldedTransfer/Unshield/ShieldedWithdrawal — Shield pays from
@@ -425,6 +450,15 @@ pub(super) fn state_transition_to_execution_event_for_check_tx_v0<'a, C: CoreRPC
425450
);
426451
}
427452
result.data
453+
} else if matches!(state_transition, StateTransition::IdentityCreate(_)) {
454+
// An identity create keeps the action built for its advanced structure
455+
// validation, so its asset lock proof and signature are verified once. Every
456+
// other transition is transformed again below: keeping its action would change
457+
// its check_tx fee estimate, which counts the operations of both transforms (a
458+
// batch's transform adds the fees of the contracts and tokens it reads). Block
459+
// execution never runs this function, so this decides mempool admission only
460+
// and is inert for consensus at every protocol version.
461+
action
428462
} else {
429463
None
430464
};

‎packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/advanced_structure_with_state.rs‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -134,17 +134,23 @@ impl StateTransitionStructureKnownInStateValidationV0 for StateTransition {
134134
/// This means we should transform into the action before validation of the advanced structure,
135135
/// and that we must even do this on check_tx
136136
///
137-
/// An identity create from addresses is checked here so that one whose key proofs of
138-
/// possession fail never enters the mempool: the address witnesses do not sign those proofs,
139-
/// so their owners should not be charged for them. Admission is not consensus, so every
140-
/// protocol version gets this.
137+
/// An identity create from addresses is checked here because a block refuses one whose key
138+
/// proofs of possession fail, unpaid from protocol version 14, as the address witnesses do
139+
/// not sign those proofs. Admission is not consensus, so every protocol version gets this.
140+
///
141+
/// An identity create is checked here because a block refuses one whose identity id is not
142+
/// the one its asset lock derives, or whose key structure or key proofs of possession fail.
143+
/// From protocol version 14 the identity id and proof of possession failures are refused
144+
/// unpaid, as the asset lock signature covers neither field. Admission is not consensus, so
145+
/// every protocol version gets this.
141146
///
142147
/// A masternode vote is checked here for the same reason: a block refuses a vote with the
143148
/// wrong voting key unpaid. Its state validation, which check_tx also runs, needs the action.
144149
fn requires_advanced_structure_validation_with_state_on_check_tx(&self) -> bool {
145150
matches!(
146151
self,
147152
StateTransition::Batch(_)
153+
| StateTransition::IdentityCreate(_)
148154
| StateTransition::IdentityCreateFromAddresses(_)
149155
| StateTransition::MasternodeVote(_)
150156
)
@@ -275,9 +281,13 @@ mod tests {
275281
use super::*;
276282

277283
#[test]
278-
fn should_return_true_only_for_batch_identity_create_from_addresses_and_masternode_vote() {
284+
fn should_return_true_only_for_batch_identity_creates_and_masternode_vote() {
279285
let batch = StateTransition::Batch(BatchTransition::V0(BatchTransitionV0::default()));
280286
assert!(batch.requires_advanced_structure_validation_with_state_on_check_tx());
287+
let identity_create = StateTransition::IdentityCreate(IdentityCreateTransition::V0(
288+
IdentityCreateTransitionV0::default(),
289+
));
290+
assert!(identity_create.requires_advanced_structure_validation_with_state_on_check_tx());
281291
let identity_create_from_addresses = StateTransition::IdentityCreateFromAddresses(
282292
IdentityCreateFromAddressesTransition::V0(
283293
IdentityCreateFromAddressesTransitionV0::default(),
@@ -294,13 +304,15 @@ mod tests {
294304
#[test]
295305
fn should_return_false_for_other_transitions() {
296306
let transitions: Vec<(&str, StateTransition)> = vec![
307+
("DataContractCreate", make_data_contract_create_st()),
297308
(
298-
"IdentityCreate",
299-
StateTransition::IdentityCreate(IdentityCreateTransition::V0(
300-
IdentityCreateTransitionV0::default(),
301-
)),
309+
"AddressFundingFromAssetLock",
310+
StateTransition::AddressFundingFromAssetLock(
311+
AddressFundingFromAssetLockTransition::V0(
312+
AddressFundingFromAssetLockTransitionV0::default(),
313+
),
314+
),
302315
),
303-
("DataContractCreate", make_data_contract_create_st()),
304316
];
305317
for (name, st) in transitions {
306318
assert!(
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
pub(crate) mod v0;
2+
pub(crate) mod v1;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
use crate::error::Error;
2+
use crate::execution::types::execution_operation::ValidationOperation;
3+
use crate::execution::types::state_transition_execution_context::{
4+
StateTransitionExecutionContext, StateTransitionExecutionContextMethodsV0,
5+
};
6+
use crate::execution::validation::state_transition::identity_create::identity_and_signatures::v0::IdentityCreateStateTransitionIdentityAndSignaturesValidationV0;
7+
use dpp::consensus::basic::invalid_identifier_error::InvalidIdentifierError;
8+
use dpp::consensus::basic::BasicError;
9+
use dpp::consensus::ConsensusError;
10+
use dpp::identity::state_transition::AssetLockProved;
11+
use dpp::state_transition::identity_create_transition::accessors::IdentityCreateTransitionAccessorsV0;
12+
use dpp::state_transition::identity_create_transition::IdentityCreateTransition;
13+
use dpp::state_transition::public_key_in_creation::IdentityPublicKeyInCreation;
14+
use dpp::validation::ConsensusValidationResult;
15+
use dpp::version::PlatformVersion;
16+
use dpp::ProtocolError;
17+
use drive::state_transition_action::identity::identity_create::IdentityCreateTransitionAction;
18+
use drive::state_transition_action::system::partially_use_asset_lock_action::PartiallyUseAssetLockAction;
19+
use drive::state_transition_action::StateTransitionAction;
20+
21+
pub(in crate::execution::validation::state_transition::state_transitions::identity_create) trait IdentityCreateStateTransitionAdvancedStructureValidationV1
22+
{
23+
fn validate_advanced_structure_from_state_v1(
24+
&self,
25+
action: &IdentityCreateTransitionAction,
26+
signable_bytes: Vec<u8>,
27+
execution_context: &mut StateTransitionExecutionContext,
28+
platform_version: &PlatformVersion,
29+
) -> Result<ConsensusValidationResult<StateTransitionAction>, Error>;
30+
}
31+
32+
impl IdentityCreateStateTransitionAdvancedStructureValidationV1 for IdentityCreateTransition {
33+
/// Like v0, except that an identity id other than the one the asset lock derives and a key
34+
/// whose proof of possession fails are refused unpaid.
35+
///
36+
/// The asset lock signature covers the transition's signable bytes, which leave out the
37+
/// identity id and the key signatures, so an identity id that is not the derived one, or a
38+
/// key signature that does not verify, is in a field the asset lock signature does not
39+
/// cover. Such a failure is not charged to the asset lock: the transition is refused like
40+
/// one whose asset lock signature fails, and the asset lock is left as it was.
41+
///
42+
/// The data of a key that carries a proof of possession is signed, yet a key whose data
43+
/// does not parse fails that proof too, and is refused unpaid like any other proof failure.
44+
/// Only the holder of the asset lock key can sign such data, and check_tx refuses it. A key
45+
/// structure failure is still charged, because the keys are part of the signed bytes.
46+
fn validate_advanced_structure_from_state_v1(
47+
&self,
48+
action: &IdentityCreateTransitionAction,
49+
signable_bytes: Vec<u8>,
50+
execution_context: &mut StateTransitionExecutionContext,
51+
platform_version: &PlatformVersion,
52+
) -> Result<ConsensusValidationResult<StateTransitionAction>, Error> {
53+
// The outpoint was already checked when the transition was transformed into an action,
54+
// so failing to derive the identifier here is not a consensus error
55+
let identifier_from_outpoint = self.asset_lock_proof().create_identifier()?;
56+
57+
// Deriving the identifier costs one double SHA-256
58+
execution_context.add_operation(ValidationOperation::DoubleSha256(1));
59+
60+
if identifier_from_outpoint != self.identity_id() {
61+
// No action: the failure is refused unpaid
62+
return Ok(ConsensusValidationResult::new_with_error(
63+
ConsensusError::BasicError(BasicError::InvalidIdentifierError(
64+
InvalidIdentifierError::new(
65+
"identity_id".to_string(),
66+
"does not match created identifier from asset lock".to_string(),
67+
),
68+
)),
69+
));
70+
}
71+
72+
let validation_result =
73+
IdentityPublicKeyInCreation::validate_identity_public_keys_structure(
74+
self.public_keys(),
75+
true,
76+
platform_version,
77+
)
78+
.map_err(Error::Protocol)?;
79+
80+
if !validation_result.is_valid() {
81+
let penalty = platform_version
82+
.drive_abci
83+
.validation_and_processing
84+
.penalties
85+
.validation_of_added_keys_structure_failure;
86+
87+
let used_credits = penalty
88+
.checked_add(execution_context.fee_cost(platform_version)?.processing_fee)
89+
.ok_or(ProtocolError::Overflow("processing fee overflow error"))?;
90+
91+
let bump_action = StateTransitionAction::PartiallyUseAssetLockAction(
92+
PartiallyUseAssetLockAction::from_borrowed_identity_create_transition_action(
93+
action,
94+
used_credits,
95+
),
96+
);
97+
98+
return Ok(ConsensusValidationResult::new_with_data_and_errors(
99+
bump_action,
100+
validation_result.errors,
101+
));
102+
}
103+
104+
let validation_result = self.validate_identity_create_state_transition_signatures_v0(
105+
signable_bytes,
106+
execution_context,
107+
);
108+
109+
if validation_result.is_valid() {
110+
Ok(ConsensusValidationResult::new())
111+
} else {
112+
// No action: the failure is refused unpaid
113+
Ok(ConsensusValidationResult::new_with_errors(
114+
validation_result.errors,
115+
))
116+
}
117+
}
118+
}

0 commit comments

Comments
 (0)