OpenZeppelin / cairo-contracts

OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup
https://docs.openzeppelin.com/contracts-cairo
MIT License
813 stars 329 forks source link

Verify signer control on `setPublicKey` #922

Closed martriay closed 4 months ago

martriay commented 7 months ago

Fixes #469, #818.

I propose not adding a 2step flow, but to add a signature: Span<felt252> param to setPublicKey (*) used to validate that we control the new owner, and that it "accepts" this ownership -- all in a single step.

To make this more user-friendly, i'm using SNIP-12 (aka Starknet's EIP712) and defining the AddOwner operation for the Account.add_owner application.

#[derive(Copy, Drop, Serde)]
struct AddOwner {
    account: ContractAddress,
    owner: felt252
}

Even though the account is already present in the hash as required by SNIP-12 and that owner is already known by the owner, I decided to make the struct overly explicit so users know what they're signing, even if it adds a bit of cost/redundancy/complexity.

If we move forward with this proposal, we should probably tackle #409 first.

*: i'd probably go even further an rename it set_owner, since that's the term we're using in the OwnerAdded and that I'm using here too.

martriay commented 7 months ago

Maybe it's worth considering having a 2step mechanism anyway, since in the current design the flow requires a bit of coordination between old and new owner:

  1. new owner signs acceptance message
  2. new owner sends it to old owner
  3. old owner crafts and sends the tx to the account

While if it's two step:

  1. former owner proposes new owner
  2. new owner accepts ownership
ericnordelo commented 4 months ago

Closing in favor of #989