Add ERC-7579 Social Recovery Executor Module (In Progress)

[gonzaotc]

Add Social Recovery Executor Module for ERC-7579 (In Progress)

Overview

Introduces a Social Recovery Executor Module that implements account recovery functionality following the ERC-7579 standard. The module enables accounts to designate guardians who can collectively initiate and execute recovery operations in case of key loss or compromise.

Key Features

• Configurable Threshold: Customizable M-of-N threshold for recovery signatures
• Timelock Protection: Enforced delay between recovery initiation and execution
• Recovery Cancellation & configuration: Allows the ERC-7579 Account and designated Guardians to cancel ongoing recovery attempts and reconfigure the recovery module.
• Replay Attack Protection: Implements EIP-712 typed data signing with nonce protection

On-going

• Implemented core functionality
• Added comprehensive tests
• Enhance guardian signature security by including executionCalldata in the EIP-712 RECOVERY_TYPEHASH and validating at execution.
• Improve O(n^2) guardian signature verification
• Prepare module documentation
• Enhance module docs (remove @notice and @param in favor of @dev, clarify explanations)
• Gas testing & optimizations
• Improve and sophisticate testing

Note: updated submodules for making use of EnumerableSet.clear

[Amxx]

What is the rational for accepting 1 but refusing 0 ?

[Amxx]

IMO that is clearer (but I guess its subjective)

Suggested change

	if (recoveryStart == 0) {
	return RecoveryStatus.NotStarted;
	}
	if (block.timestamp < recoveryStart + _recoveryConfigs[account].timelock) {
	return RecoveryStatus.Started;
	}
	return RecoveryStatus.Ready;
	return recoveryStart == 0
	? RecoveryStatus.NotStarted
	: (block.timestamp < recoveryStart + _recoveryConfigs[account].timelock)
	? RecoveryStatus.Started
	: RecoveryStatus.Ready;

[gonzaotc]

I would personally choose current for readability. I'm not a big fan of nested ternaries, but I agree that is subjective. Let's take the approach more commonly used across OpenZeppelin contracts?, on which I will take your advice for sure

[Amxx]

What is the rational for storing the start, and doing that math when recovering the status versus storing the end (doing the add math when initiating a recovery) ?

I'm not sure if one is clearly better than the other, but we should documetn that choice with regard to the delay changing during the recovery.

[gonzaotc]

I agree, moving the addition from getRecoveryStatus to startRecovery, which is less frequently used, would slightly get us some gas. We would also be saving one load on getRecoveryStatus, as currently it needs to load both the recovery start and the timelock. In contrast, with this other approach, it would just need to load the recovery end.

Regarding documenting the decision, under the current approach a change in the timelock during an active recovery pushes the recoveryEnd further in time. In contrast, with this alternative approach, it would be fixed.

This being said, the account owner can always cancelRecovery if he wants or needs to, so both approaches are okay for me in this sense, as they don't present anything that wouldn't be achievable with the other. I would pick the cheapest

[Amxx]

what about using bytes[] instead of GuardianSignature[] and doing the lookup through

address signer = address(bytes20(guardianSignatures[i][0:20]));
bytes calldata signerSign = guardianSignatures[i][20:];

That would reduce the size of the calldata.

[Amxx]

What is the rational for having this function ? If someone for some reasons wants to set 100 guardians, would there be any technical issues that would cause this contrains to fail ?

[Amxx]

Why use maxGuardians here?

The unicity check will fail if guardianSignatures.length is greater than _recoveryConfigs[account].guardians.length anyway.

[Amxx]

This PR contains a lot of different visibility and overridability.

There are external, public and internal functions, some of which are virtual, some of which are not. Is there any undocummented logic to it? Note that at OZ we try to only use public virtual and internal virtual (and private)

[gonzaotc]

Thank you for raising the point about visibility and overridability. After further examination and understanding that external functions cannot be called upon inheritance with super, that they don't improve gas costs, and aiming at flexibility for module consumers, I opted to follow the OZ pattern of public virtual and external virtual, which I agree is great.

The only exceptions are:

• The isModuleType function, which I set as external pure because I don't expect consumers to extend the Module to another module type besides MODULE_TYPE_EXECUTOR.
• The two internal getters for the EIP-712 digests, as they fully depend on private constant typehashes.

Update: digest getters also made virtual based on the Ernesto recommendation for flexibility

[ernestognw]

An important guarantee of a timelock is that there shouldn't be a way to reset it or change it. The reason is that is usually there to have time in case an attacker gains control of the account. Here, I understand that the attacker having control of the account is essentially game over, but I think the general principle that the timelock shouldn't be accelerated must remain.

Consider taking a look to the logic in Time.sol

[ernestognw]

Currently, all these 3 are validating that the threshold can be met after the update has been done.

/// _addGuardian
if (_recoveryConfigs[account].guardians.length() >= maxGuardians()) {
    revert TooManyGuardians();
}

/// _removeGuardian
if (_recoveryConfigs[account].guardians.length() == _recoveryConfigs[account].threshold) {
    revert CannotRemoveGuardian();
}

/// _setThreshold
if (threshold == 0 || threshold > _recoveryConfigs[account].guardians.length()) {
    revert InvalidThreshold();
}

In order to keep the code dry, I think it's better to just do the update and have an internal _validateThreshold that does validate the state is still consistent. Concretely I'm thinking of:

function _validateThreshold(address account) internal view virtual {
    uint256 guardians = _recoveryConfigs[account].guardians.length()
    uint256 _threshold =_recoveryConfigs[account].threshold
    if (guardians < _threshold) revert UnreachableThreshold(account, guardians, _threshold);
}

Then, all these functions become essentially reduced to:

function _addGuardian(address account, address guardian) internal virtual {
    if (!_recoveryConfigs[account].guardians.add(guardian)) revert AlreadyGuardian();
    _validateThreshold(account);
    emit GuardianAdded(account, guardian);
}

function _removeGuardian(address account, address guardian) internal virtual {
    if (!_recoveryConfigs[account].guardians.remove(guardian)) revert GuardianNotFound();
     _validateThreshold(account);
    emit GuardianRemoved(account, guardian);
}

function _setThreshold(address account, uint256 threshold) internal virtual {
    _recoveryConfigs[account].threshold = threshold;
    _validateThreshold(account);
    emit ThresholdChanged(account, threshold);
}

[ernestognw]

I think this check is not required. The user must sign a transaction to set a signer as the address(0) and the bottom line is that there's very little we can do to protect them to set an unrecognized address as a guardian.

Suggested change

	if (guardian == address(0)) {
	revert InvalidGuardian();
	}

[ernestognw]

Also, in _validateGuardianSignatures we're doing:

if (uint160(signer) <= uint160(lastSigner)) {
    revert DuplicatedOrUnsortedGuardianSignatures();
}

So we can implicitly exclude the address(0) by just changing <= for <=

if (uint160(signer) < uint160(lastSigner)) {
    revert DuplicatedOrUnsortedGuardianSignatures();
}

[arr00]

If we have <, that removes the duplication check. We could increment the initial lastSigner from address(0) if this is a concern. isValidSignatureNow will always fail on sig from address 0 though since we are using ECDSA.tryRecover. Don't think we should think too hard here.

[gonzaotc]

Thanks to both, as Arr mentioned, <= is required to check for duplication on the sorted signatures.

About the guardian != address(0) condition, it's true that it doesn't add any required protection to signature validation, as _validateGuardianSignatures will definitively revert with InvalidGuardianSignature anyway, as no one has the corresponding private key to sign with it.

Nonetheless, I put this check on guardian addition mainly as a common practice to protect clients when they pass the ZeroAddress due to human or software error.

If we agree this is a validation we don't typically use on addresses in our codebase, I will remove it for consistency

[ernestognw]

Right, is a good observation that <= disallows duplication.

Still, I don't think is worth checking that guardian != address(0). Agree there could be a human/software error but I consider that out of scope of the library.

[ernestognw]

I think the signature of both the _addGuardian and _removeGuardian functions must allow batch adding/removing guardians natively. It'd be more elegant but it's also opinionated since the account can batch operations. It's just that batching operations at the account level may require extra work than just calling a batched function directly

for (uint256 i = 0; i < signers.length; i++) {
    if (!_associatedSigners[account].add(guardians[i]))
        revert AlreadyGuardian(account, guardians[i]);
}
emit GuardiansAdded(account, guardians);

[ernestognw]

It'd be better if this function returns a boolean instead of reverting. This way there can be a public view getter called validateGuardianSignatures that allows to test if the guardian signatures construction is correct

[gonzaotc]

Updated account readme.adoc with recent changes from pr #87