This repository has been archived by the owner on Sep 30, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 13
Safe Recovery Plugin #96
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
267ff7c
Working recovery test, a lot of clean up needed
blakecduncan e9a0fec
Fix some warnings
blakecduncan 5140254
Clean up the updateOwner function in the ecdsaPlugin
blakecduncan 1dd9e6b
Clean up the recovery plugin
blakecduncan 002bf76
Initial recovery plugin test clean up
blakecduncan 6b7dd33
Minor clean up
blakecduncan 280d9a9
More clean up of the recovery tests
blakecduncan c00dc6f
Merge remote-tracking branch 'origin/main' into safe-plugin-recovery
blakecduncan 2cc0d12
Linting fixes
blakecduncan aad1a42
Remove unnecessary code in plugin
blakecduncan 5066be4
Update naming
blakecduncan 8d58748
Eslint error in test
blakecduncan 5d7fbc1
Fixing more eslint issues
blakecduncan 775d078
Remove disable eslint issue
blakecduncan f05e834
Add note in read me about pre alpha state
blakecduncan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
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(); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} |
136 changes: 136 additions & 0 deletions
136
account-integrations/safe/test/hardhat/SafeECDSARecoveryPlugin.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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