Safe Recovery Plugin (#96)

* Working recovery test, a lot of clean up needed

* Fix some warnings

* Clean up the updateOwner function in the ecdsaPlugin

* Clean up the recovery plugin

* Initial recovery plugin test clean up

* Minor clean up

* More clean up of the recovery tests

* Linting fixes

* Remove unnecessary code in plugin

* Update naming

* Eslint error in test

* Fixing more eslint issues

* Remove disable eslint issue

* Add note in read me about pre alpha state

account-integrations/safe/README.md

@@ -1,5 +1,7 @@
 # Safe plugins
 
+Please note, these plugins are in a pre-alpha state and are not ready for production use. In their current state, the plugins are meant for testing and experimentation.
+
 # Getting Started
 
 1. `cd account-integrations/safe`

account-integrations/safe/src/SafeBlsPlugin.sol

@@ -67,7 +67,7 @@ contract SafeBlsPlugin is BaseAccount {
         return IEntryPoint(_entryPoint);
     }
 
-    function owner() public pure returns (uint256[4] memory _blsPublicKey) {
+    function owner() public view returns (uint256[4] memory) {
         return _blsPublicKey;
     }
 
@@ -100,7 +100,7 @@ contract SafeBlsPlugin is BaseAccount {
      * This function prevents using a “key” different from the first “zero” key.
      * @param nonce to validate
      */
-    function _validateNonce(uint256 nonce) internal view override {
+    function _validateNonce(uint256 nonce) internal pure override {
         if (nonce >= type(uint64).max) {
             revert NONCE_NOT_SEQUENTIAL();
         }

account-integrations/safe/src/SafeECDSAPlugin.sol

@@ -22,13 +22,14 @@ interface ISafe {
 contract SafeECDSAPlugin is BaseAccount {
     using ECDSA for bytes32;
 
-    address public immutable myAddress;
-    address private immutable _owner;
+    address public immutable myAddress; // Module address
+    address private _owner; // Key address
     address private immutable _entryPoint;
 
     address internal constant _SENTINEL_MODULES = address(0x1);
 
     error NONCE_NOT_SEQUENTIAL();
+    event OWNER_UPDATED(address indexed oldOwner, address indexed newOwner);
 
     constructor(address entryPointAddress, address ownerAddress) {
         myAddress = address(this);
@@ -71,10 +72,17 @@ contract SafeECDSAPlugin is BaseAccount {
         return _owner;
     }
 
+    function updateOwner(address newOwner) public {
+        // TODO: Check if msg.sender is the Safe. We can't do that now because the Safe
+        // 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;
+    }
+
     function _validateSignature(
         UserOperation calldata userOp,
         bytes32 userOpHash
-    ) internal override returns (uint256 validationData) {
+    ) internal view override returns (uint256 validationData) {
         bytes32 hash = userOpHash.toEthSignedMessageHash();
         if (_owner != hash.recover(userOp.signature))
             return SIG_VALIDATION_FAILED;
@@ -86,7 +94,7 @@ contract SafeECDSAPlugin is BaseAccount {
      * This function prevents using a “key” different from the first “zero” key.
      * @param nonce to validate
      */
-    function _validateNonce(uint256 nonce) internal view override {
+    function _validateNonce(uint256 nonce) internal pure override {
         if (nonce >= type(uint64).max) {
             revert NONCE_NOT_SEQUENTIAL();
         }

account-integrations/safe/src/SafeECDSARecoveryPlugin.sol

@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+contract Enum {
+    enum Operation {
+        Call,
+        DelegateCall
+    }
+}
+
+interface ISafe {
+    /// @dev Allows a Module to execute a Safe transaction without any further confirmations.
+    /// @param to Destination address of module transaction.
+    /// @param value Ether value of module transaction.
+    /// @param data Data payload of module transaction.
+    /// @param operation Operation type of module transaction.
+    function execTransactionFromModule(
+        address to,
+        uint256 value,
+        bytes calldata data,
+        Enum.Operation operation
+    ) external returns (bool success);
+}
+
+contract SafeECDSARecoveryPlugin {
+    address private immutable storedEOA;
+    address public storedSafe;
+
+    error SENDER_NOT_STORED_EOA(address sender);
+    error ATTEMPTING_RESET_ON_WRONG_SAFE(address attemptedSafe);
+
+    constructor(address _safe, address _eoa) {
+        storedSafe = _safe;
+        storedEOA = _eoa;
+    }
+
+    modifier onlyStoredEOA {
+        if (msg.sender != storedEOA) {
+            revert SENDER_NOT_STORED_EOA(msg.sender);
+        }
+        _;
+    }
+
+    function resetEcdsaAddress(
+        address safe,
+        address ecdsaPluginAddress,
+        address newValidatingEcdsaAddress
+    ) external onlyStoredEOA {
+        if (safe != storedSafe) {
+            revert ATTEMPTING_RESET_ON_WRONG_SAFE(safe);
+        }
+
+        bytes memory data = abi.encodeWithSignature("updateOwner(address)", newValidatingEcdsaAddress);
+        ISafe(safe).execTransactionFromModule(ecdsaPluginAddress, 0, data, Enum.Operation.Call);
+    }
+}

account-integrations/safe/test/hardhat/SafeECDSARecoveryPlugin.test.ts

@@ -0,0 +1,136 @@
+import { ethers } from "hardhat";
+import { expect } from "chai";
+import { HDNodeWallet, Provider } from "ethers";
+import { setupTests, sendTx } from "./setupTests";
+
+import { executeContractCallWithSigners } from "./utils/execution";
+
+import { EntryPoint } from "../../typechain-types/lib/account-abstraction/contracts/core/EntryPoint";
+import { SafeECDSAPlugin } from "../../typechain-types/src/SafeECDSAPlugin.sol/SafeECDSAPlugin";
+
+const MNEMONIC = "test test test test test test test test test test test junk";
+
+describe("SafeECDSARecoveryPlugin", () => {
+  let provider: Provider;
+  let safeSigner: HDNodeWallet;
+  let entryPoint: EntryPoint;
+  let safeECDSAPlugin: SafeECDSAPlugin;
+  let safeCounterfactualAddress: string;
+
+  before(async () => {
+    const {
+      safeOwner,
+      entryPointContract,
+      safeEcdsaPluginContract,
+      counterfactualAddress,
+    } = await setupTests();
+
+    provider = ethers.provider;
+    safeSigner = safeOwner;
+    entryPoint = entryPointContract;
+    safeECDSAPlugin = safeEcdsaPluginContract;
+    safeCounterfactualAddress = counterfactualAddress;
+  });
+
+  it("Should enable a recovery plugin on a safe.", async () => {
+    const [, , recoverySigner] = await ethers.getSigners();
+
+    const recoveryPlugin = await (
+      await ethers.getContractFactory("SafeECDSARecoveryPlugin")
+    ).deploy(safeCounterfactualAddress, recoverySigner.address);
+    const recoveryPluginAddress = await recoveryPlugin.getAddress();
+
+    // Enable recovery plugin on safe
+
+    const deployedSafe = await ethers.getContractAt(
+      "Safe",
+      safeCounterfactualAddress,
+    );
+    const isModuleEnabledBefore = await deployedSafe.isModuleEnabled(
+      recoveryPluginAddress,
+    );
+
+    await executeContractCallWithSigners(
+      deployedSafe,
+      deployedSafe,
+      "enableModule",
+      [recoveryPluginAddress],
+      // @ts-expect-error safeSigner doesn't have all properties for some reason
+      [safeSigner],
+    );
+
+    const isModuleEnabledAfter = await deployedSafe.isModuleEnabled(
+      recoveryPluginAddress,
+    );
+
+    expect(isModuleEnabledBefore).to.equal(false);
+    expect(isModuleEnabledAfter).to.equal(true);
+  });
+
+  it("Should use recovery plugin to reset signing key and then send tx with new key.", async () => {
+    // Setup recovery plugin
+
+    const [, , , recoverySigner] = await ethers.getSigners();
+
+    const recoveryPlugin = await (
+      await ethers.getContractFactory("SafeECDSARecoveryPlugin")
+    ).deploy(safeCounterfactualAddress, recoverySigner.address);
+    const recoveryPluginAddress = await recoveryPlugin.getAddress();
+
+    const deployedSafe = await ethers.getContractAt(
+      "Safe",
+      safeCounterfactualAddress,
+    );
+
+    // Enable recovery plugin
+
+    await executeContractCallWithSigners(
+      deployedSafe,
+      deployedSafe,
+      "enableModule",
+      [recoveryPluginAddress],
+      // @ts-expect-error safeSigner doesn't have all properties for some reason
+      [safeSigner],
+    );
+
+    const isModuleEnabled = await deployedSafe.isModuleEnabled(
+      recoveryPluginAddress,
+    );
+    expect(isModuleEnabled).to.equal(true);
+
+    // Reset ecdsa address
+
+    const ecdsaPluginAddress = await safeECDSAPlugin.getAddress();
+    const newEcdsaPluginSigner = ethers.Wallet.createRandom().connect(provider);
+
+    const recoveryPluginSinger = recoveryPlugin.connect(recoverySigner);
+
+    await recoveryPluginSinger.resetEcdsaAddress(
+      await deployedSafe.getAddress(),
+      ecdsaPluginAddress,
+      newEcdsaPluginSigner.address,
+    );
+
+    // Send tx with new key
+
+    const recipientAddress = ethers.Wallet.createRandom().address;
+    const transferAmount = ethers.parseEther("1");
+    const userOpCallData = safeECDSAPlugin.interface.encodeFunctionData(
+      "execTransaction",
+      [recipientAddress, transferAmount, "0x00"],
+    );
+    const recipientBalanceBefore = await provider.getBalance(recipientAddress);
+    await sendTx(
+      newEcdsaPluginSigner,
+      entryPoint,
+      safeCounterfactualAddress,
+      "0x1",
+      "0x",
+      userOpCallData,
+    );
+
+    const recipientBalanceAfter = await provider.getBalance(recipientAddress);
+    const expectedRecipientBalance = recipientBalanceBefore + transferAmount;
+    expect(recipientBalanceAfter).to.equal(expectedRecipientBalance);
+  });
+});

account-integrations/safe/test/hardhat/integration/SafeWebAuthnPlugin.test.ts

@@ -63,10 +63,10 @@ describe("SafeWebAuthnPlugin", () => {
   const getPublicKeyAndSignature = () => {
     const publicKey: [BigNumberish, BigNumberish] = [
       BigInt(
-        "114874632398302156264159990279427641021947882640101801130664833947273521181002"
+        "114874632398302156264159990279427641021947882640101801130664833947273521181002",
       ),
       BigInt(
-        "32136952818958550240756825111900051564117520891182470183735244184006536587423"
+        "32136952818958550240756825111900051564117520891182470183735244184006536587423",
       ),
     ];
 
@@ -80,10 +80,10 @@ describe("SafeWebAuthnPlugin", () => {
     const clientChallengeDataOffset = 36;
     const signature: [BigNumberish, BigNumberish] = [
       BigInt(
-        "45847212378479006099766816358861726414873720355505495069909394794949093093607"
+        "45847212378479006099766816358861726414873720355505495069909394794949093093607",
       ),
       BigInt(
-        "55835259151215769394881684156457977412783812617123006733908193526332337539398"
+        "55835259151215769394881684156457977412783812617123006733908193526332337539398",
       ),
     ];