Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Safe Recovery Plugin #96

Merged
merged 15 commits into from
Sep 20, 2023
Merged

Safe Recovery Plugin #96

merged 15 commits into from
Sep 20, 2023

Conversation

blakecduncan
Copy link
Contributor

@blakecduncan blakecduncan commented Sep 18, 2023

#71

Adding another plugin that we can use to reset a key for recovery. Right now this is a MVP to work with the ECDSA plugin.

List of changes:

  • Added an updateOwner() function to the SafeECDSAPlugin.
  • Added a RecoveryPlugin that can execute a tx from the Safe to reset the key.
  • Added a hardhat test to enable a RecoveryPlugin and test changing the key for the ECDSA plugin

Notes/questions from this work

  • After the SafeECDSAPlugin is enabled, the original owner of the safe can still execute transactions the normal 'safe' way. Do we want to change this so that when a plugin is enabled you can't still sign transactions the old way?
  • Since we deploy the SafeECDSAPlugin with the acutal Safe, the plugin doesn't know the address of the safe. So we can't do validation to check msg.sender is the safe.
  • I think it could be a nice UX improvement to make the ECDSA plugin a singleton or registry that anyone can enable. Rather than deploying a plugin for every Safe.

@blakecduncan blakecduncan marked this pull request as draft September 18, 2023 20:05
@blakecduncan blakecduncan self-assigned this Sep 19, 2023
@blakecduncan blakecduncan marked this pull request as ready for review September 19, 2023 15:15
address public immutable myAddress;
address private immutable _owner;
address public immutable myAddress; // Module address
address private _owner; // Key address
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be good to deploy this separately from the Safe so that we can store the safe address here to validate that transactions are coming from the right Safe.

Then we can initialize a variable like

address private immutable safeAddress;

@jzaki
Copy link
Contributor

jzaki commented Sep 19, 2023

After the SafeECDSAPlugin is enabled, the original owner of the safe can still execute transactions the normal 'safe' way. Do we want to change this so that when a plugin is enabled you can't still sign transactions the old way?
I spoke with Andrew about this earlier today, I think a default ECDSA plugin to begin, so original way is not required. Perhaps even an ECDSA multisig plugin that replicats the same functionality. For demos we don't need to remove the original safe multisig functionality, it's a helpful fallback if all validation plugins are removed or become unusable through user config error.

Since we deploy the SafeECDSAPlugin with the acutal Safe, the plugin doesn't know the address of the safe. So we can't do validation to check msg.sender is the safe.
There will be a way around this chicken/egg scenario, feel free to create an issue (or not if referring to #89).

I think it could be a nice UX improvement to make the ECDSA plugin a singleton or registry that anyone can enable. Rather than deploying a plugin for every Safe.
Yes, also mentioned this morning. Feel free to create an issue 👍

Copy link
Contributor

@jzaki jzaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for powering through this recovery start. Also check out #97 for some designs around helpers/wrappers of smart contracts that may eventually be able to be brought into modules and also used in solidity tests.

// and the plugin are deployed at the same time. So the plugin doesn't know the Safe address.
emit OWNER_UPDATED(_owner, newOwner);
_owner = newOwner;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make sure the readme is clear with its state of "pre-alpha" and "not for production use" etc.


bytes memory data = abi.encodeWithSignature("updateOwner(address)", newValidatingEcdsaAddress);
ISafe(safe).execTransactionFromModule(ecdsaPluginAddress, 0, data, Enum.Operation.Call);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start, will have to update and add to this, linking other verifications beyond onlyStoredEOA

@blakecduncan blakecduncan merged commit d3eb9c9 into main Sep 20, 2023
2 checks passed
@blakecduncan blakecduncan deleted the safe-plugin-recovery branch September 20, 2023 14:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants