[move][aptos-framework] Simplify account key rotation

[peekpi]

trafficstars

Description

This PR implements feature issue #4066. The scheme of this PR is fully compatible with existing schemes.

To simplify account key rotation, it introduces a new struct RotationProofChallengeSimplify and a new entry function rotate_authentication_key_simplify.

Struct RotationProofChallengeSimplify has same structure as RotationProofChallenge, just with different type_info.

Entry function rotate_authentication_key_simplify is similar to rotate_authentication_key, but:

1. it use account address instead of signer
2. it verify signature of current public key, but with struct RotationProofChallengeSimplify
3. it does not veirfy signatrue of new public key, so it doesn't require the cap_update_table
4. it does not update the OriginatingAddress table

Unit Test Log

➜  aptos-framework git:(rotation_key) ✗ aptos move test --filter=account
INCLUDING DEPENDENCY AptosStdlib
INCLUDING DEPENDENCY MoveStdlib
BUILDING AptosFramework
Running Move unit tests
[ PASS    ] 0x1::aptos_account::test_transfer
[ PASS    ] 0x1::genesis::test_create_account
[ PASS    ] 0x1::vesting::test_create_vesting_contract_with_missing_withdrawal_account_should_fail
[ PASS    ] 0x1::account::mock_sequence_numbers
[ PASS    ] 0x1::resource_account::test_create_account_and_retrieve_cap
[ PASS    ] 0x1::genesis::test_create_accounts
[ PASS    ] 0x1::account::test_cannot_control_resource_account_via_auth_key
[ PASS    ] 0x1::vesting::test_create_vesting_contract_with_unregistered_withdrawal_account_should_fail
[ PASS    ] 0x1::resource_account::with_coin
[ PASS    ] 0x1::account::test_create_resource_account
[ PASS    ] 0x1::vesting::test_set_beneficiary_with_missing_account_should_fail
[ PASS    ] 0x1::resource_account::without_coin
[ PASS    ] 0x1::account::test_duplice_create_resource_account
[ PASS    ] 0x1::vesting::test_set_beneficiary_with_unregistered_account_should_fail
[ PASS    ] 0x1::account::test_empty_public_key
[ PASS    ] 0x1::account::test_empty_signature
[ PASS    ] 0x1::account::test_invalid_check_signer_capability_and_create_authorized_signer
[ PASS    ] 0x1::account::test_invalid_offer_signer_capability
[ PASS    ] 0x1::account::test_invalid_revoke_signer_capability
[ PASS    ] 0x1::account::test_module_capability
[ PASS    ] 0x1::account::test_resource_account_and_create_account
[ PASS    ] 0x1::account::test_valid_check_signer_capability_and_create_authorized_signer
[ PASS    ] 0x1::account::test_valid_revoke_signer_capability
[ PASS    ] 0x1::account::test_valid_rotate_authentication_key_multi_ed25519_to_ed25519
[ PASS    ] 0x1::account::test_valid_rotate_authentication_key_multi_ed25519_to_ed25519_simplify
[ PASS    ] 0x1::account::test_valid_rotate_authentication_key_multi_ed25519_to_multi_ed25519
[ PASS    ] 0x1::account::test_valid_rotate_authentication_key_multi_ed25519_to_multi_ed25519_simplify
Test result: OK. Total tests: 27; passed: 27; failed: 0
{
  "Result": "Success"
}

---

This change is 

[0xchloe]

we have an AIP for domain-separated signature. this means that we're changing the signature verification scheme. we'll get back to this PR after the domain-separated signature work is done.

[JackyWYX]

Simplifying the key rotation interface and domain-separated signature looks like two different issues, and the only overlap I guess is the construction for keyRotationChallenge. I wonder how do you think the domain specific signature can be a blocker for this PR?

Is it possible that we may get this PR merge first, and change the signature verification later? There need to be a breaking change of the key rotation eventually as the merge of domain-separated signature anyway.

@alinush @chloeqjz @areshand

[0xchloe]

I don't think doing things ad-hoc is good for security and we'd only proceed with PRs with signature verifications once the domain separation work is done. Moreover, even if we get this PR into the codebase, we probably wouldn't release a mainnet update sooner than the domain separation update. That means, you wouldn't be able to use this function on mainnet for a while, regardless if this PR is merged into main.

Now here's the security concern I can think of: if you run this function on devnet / testnet (let's say with sequence number 10), you can replay this transaction on mainnet when the sequence number is matching (i.e. when the sequence number of this account is 10). This is not ideal. Previously we pass in the signer, so it can only be replayed by the person who owns the private key - which is safe, and they most likely don't have an incentive to do it.

[peekpi]

For replay attacks, we can add chainId to the challenge struct or type_info struct to prevent it. I don't think use signer to prevent replay attacks is a good mechanism. for example, attacker can hardcode the signatures collected from the testnet into a contract and induce signer to call it.

And for domain separation, is this similar to EIP-712? Can this prevent replay attacks? And do you have an estimated date for when it will be completed? in a month? or longer? @chloeqjz

[0xchloe]

For replay attacks, we can add chainId to the challenge struct or type_info struct to prevent it. I don't think use signer to prevent replay attacks is a good mechanism. for example, attacker can hardcode the signatures collected from the testnet into a contract and induce signer to call it.

And for domain separation, is this similar to EIP-712? Can this prevent replay attacks? And do you have an estimated date for when it will be completed? in a month? or longer? @chloeqjz

Yes, I was not saying that we're using signers to prevent replay attacks. I was just saying that in the case of rotate_authentication_key, having the signer is safer. If an attacker can induce a signer for malicious purpose, then the attacker can almost do anything (e.g. drain your wallet).

So yes, to prevent unexpected attacks, Alin is working on a new signature verification scheme that'll be more consistent and safe across the board and we'd prefer to wait for that before proceeding with any PRs that contain signature verification. Yes it's similar to EIP 712 - @alinush would you happen to have an estimated date of when it'll be completed?

[JackyWYX]

For replay attacks, we can add chainId to the challenge struct or type_info struct to prevent it. I don't think use signer to prevent replay attacks is a good mechanism. for example, attacker can hardcode the signatures collected from the testnet into a contract and induce signer to call it. And for domain separation, is this similar to EIP-712? Can this prevent replay attacks? And do you have an estimated date for when it will be completed? in a month? or longer? @chloeqjz

Yes, I was not saying that we're using signers to prevent replay attacks. I was just saying that in the case of rotate_authentication_key, having the signer is safer. If an attacker can induce a signer for malicious purpose, then the attacker can almost do anything (e.g. drain your wallet).

So yes, to prevent unexpected attacks, Alin is working on a new signature verification scheme that'll be more consistent and safe across the board and we'd prefer to wait for that before proceeding with any PRs that contain signature verification. Yes it's similar to EIP 712 - @alinush would you happen to have an estimated date of when it'll be completed?

So this is likely to be some extra work to sign the KeyRotationChallenge, right? How do you think this is a blocker for merging this PR?

[JackyWYX]

Is there any estimation on when will the feature be completed? @davidiw @chloeqjz @alinush

[github-actions[bot]]

This issue is stale because it has been open 45 days with no activity. Remove the stale label, comment or push a commit - otherwise this will be closed in 15 days.

[JackyWYX]

Waiting for domain specific signature.

[davidiw]

Closing this out as we align on an alternative across the groups :pray: