add: recovery plugin contract #17
Conversation
src/validator/RecoveryPlugin.sol
Outdated
| ); | ||
| require(hash != bytes32(0), "RecoveryPlugin: hash is zero"); | ||
| require( | ||
| block.timestamp >= recoveryDelay[msg.sender], |
There was a problem hiding this comment.
accessing the timestamp will violate the 4337 rule so enable should not be called on validation phase right? which also makes it impossible for wallet to set this as default on deployment
There was a problem hiding this comment.
maybe we can do short circuit here like require(oldOwner == address(0) || ~)
src/validator/RecoveryPlugin.sol
Outdated
| changeOwner(_newOwner); | ||
| } | ||
|
|
||
| function validateUserOp( |
There was a problem hiding this comment.
Since enable can only be called after passing the validateUserOp(), I think we should do some more tricks that will enable guardians to call enable() function.
maybe adding a mode will do?
src/validator/RecoveryPlugin.sol
Outdated
| bool approved; | ||
| } | ||
|
|
||
| contract RecoveryPlugin is IKernelValidator { |
There was a problem hiding this comment.
To be consistent I recommend naming this contract & file as SocialRecoveryValidator.
src/validator/RecoveryPlugin.sol
Outdated
| emit OwnerChanged(msg.sender, oldOwner, _newOwner); | ||
| } | ||
|
|
||
| function addGuardian( |
There was a problem hiding this comment.
Since this function actually adds ALL guardians, it might be best to call it setGuardians.
Also I think setGuardians should actually reset all guardians which I don't think this function currently does. I think it's better that way since otherwise if you accidentally added a wrong guardian, as far as I can tell there's no way to remove it.
src/validator/RecoveryPlugin.sol
Outdated
| function enable(bytes calldata _data) external override { | ||
| //0x00 - to add guardians | ||
| //0x01 - to change owner | ||
| bytes memory mode = bytes(_data[0:1]); |
There was a problem hiding this comment.
You can use bytes1 for this and use
if(mode == 0x00) in next line
src/validator/RecoveryPlugin.sol
Outdated
| } | ||
|
|
||
| function divideBytes( | ||
| bytes memory data |
There was a problem hiding this comment.
we can use calldata for this which will make "chunck" much simpler
src/validator/RecoveryPlugin.sol
Outdated
| function initRecovery( | ||
| address _newOwner, | ||
| bytes32 hash, | ||
| bytes[] memory signatures |
src/validator/RecoveryPlugin.sol
Outdated
|
|
||
| function verifyGuardians( | ||
| bytes32 hash, | ||
| bytes[] memory signatures |
| bytes20(_userOp.callData[233:253]) | ||
| ); | ||
| require( | ||
| isGuardian[kernelAddress][_userOp.sender], |
There was a problem hiding this comment.
Not sure if this is ok, kernelAddress==userOp.sender right?
There was a problem hiding this comment.
Won't be in this mode, since kernelAddress is the address of the account which is to be recovered and userOp.sender is any guardian so kernelAddress!=userOp.sender
There was a problem hiding this comment.
so you mean guardian is another kernel?
| @@ -129,7 +131,7 @@ contract SocialRecoveryValidator is IKernelValidator { | |||
| ) | |||
| ); | |||
| require(weight > 0, "RecoveryPlugin: weight is zero"); | |||
| isGuardian[msg.sender][guardian]=true; | |||
| isGuardian[msg.sender][guardian] = true; | |||
There was a problem hiding this comment.
Shouldn’t this be isGuardian[guardian][msg.sender]?
According to 4337 storage access rule, the last key should be the userOp.sender not the first one. It’s quite counter intuitive but that is what we should follow
There was a problem hiding this comment.
It kinda helped me to add the user defined types for clarity instead of using plain address
In this case, i would define 1) Guardian type 2) Sender type for clarity
https://soliditylang.org/blog/2021/09/27/user-defined-value-types/
|
moving discussion to #31 |
No description provided.