Add ERC7579SignatureValidator

[ernestognw]

Description

Before writing some guides about ERC7579, we need this validator and also the executors in #121. The idea is that we write a guide for generalized signer modules with ERC-7913 and use these primitives as as building blocks for extended uses cases like social recovery

[gonzaotc]

Note that Module.onUninstall can be called directly perpetually by the Account, even if the module is not installed. In contrast, Module.onInstall reverts if the signer is already set, given the signer(msg.sender).length == 0 requirement included.

Should we also consider adding a valid signer set requirement at uninstallation and throw ERC7579SignatureValidatorUninstalled for consistency between both processes?

UPDATE:

After checking ERC-7579, it doesn't seem to enforce, either put the responsibility on Modules to revert installation if the module is already installed, the same logic applying for uninstallation. Instead, it states that it is the Account that MUST revert during Account.installModule and Account.uninstallModule, which we are correctly doing already in our AccountERC7579 implementation.

Therefore, the extra check at Module.onInstall has some degree of redundancy, but I understand that it fulfills the role of restricting the account from directly calling Module.onInstall indefinitely afterwards to arbitrarily change the signer via this means, so I think is okay and necessary to have this requirement here for this purpose, even if it is the Account that should be restricting installation and uninstallation.

[gonzaotc]

UPDATE: It leads me to think we shouldn't have two separate sources of truth determining if the module is installed or not at a given moment, and that these two sources should be synced perpetually it they do coexist, otherwise it would lead to invalid states between Module and Account, where one considers the module is installed in the Account, while the other doesn't.

An example of this division is the current implementation, where the Module is restricting Module.onInstall if it has a signer already set, thus it is using it's own state to determine if the Module is installed, while the Account determines the module is installed if it's registered on it's storage. Therefore, if the Account calls directly Module.onInstall by human error, the Module get's into an state where it considers itself to be installed, but the Account considers it's not.

Going further, If the Account does this direct Module.onInstall call, (which should not be possible in an ideal world), it doesn't mean that the module is installed in the standard ERC-7579 procedure, and therefore, the module should still be installable by the standard procedure in my opinion, it should not be blocked for installation. This means, that Module.onInstall should still be callable and succeed via Account.installModule.

Given the ERC design, I think the correct source of truth for this means should be the Account, and therefore, if the module isn't registered as installed into it's storage via the standard procedure, we shouldn't block the Module.onInstall, since it leads to this un-synced states between Account and Module.

Since we cannot freely leave Module.onInstall without any restriction, given the Account could potentially use it posteriorly to modify the signer arbitrarily, I propose to make Module.onInstall depend directly on Account storage to determine if the module is installed.

function onInstall(bytes calldata data) public virtual {
        require(
            !IERC7579ModuleConfig(msg.sender).isModuleInstalled(
                MODULE_TYPE_VALIDATOR,
                address(this),
                ""
            ),
            ERC7579SignatureValidatorAlreadyInstalled()
        );
        setSigner(data);
    }

This approach provides some warranties:

1. If the Account incorrectly calls directly Module.onInstall, it doesn't block the standard installation of the module via Account.installModule. (Not possible right now)
2. After standard installation, the Account is no longer able to directly call Module.onInstall and use it to modify the signer arbitrarily.

Same strategy can be applied to Module.onUninstall:

function onUninstall(bytes calldata) public virtual {
        require(
            IERC7579ModuleConfig(msg.sender).isModuleInstalled(
                MODULE_TYPE_VALIDATOR,
                address(this),
                ""
            ),
            ERC7579SignatureValidatorNotInstalled()
        );
        _setSigner(msg.sender, "");
    }

However, this approach would require a minor change in AccountERC7579 to work correctly, specifically, in _installModule and _uninstallModule. We should call first Module.onInstall, and then registry the module in the Account storage, not in the opposite order.

Note the current order:

function _installModule(uint256 moduleTypeId, address module, bytes memory initData) internal virtual {
        require(supportsModule(moduleTypeId), ERC7579Utils.ERC7579UnsupportedModuleType(moduleTypeId));
        require(
            IERC7579Module(module).isModuleType(moduleTypeId),
            ERC7579Utils.ERC7579MismatchedModuleTypeId(moduleTypeId, module)
        );

        if (moduleTypeId == MODULE_TYPE_VALIDATOR) {
            require(_validators.add(module), ERC7579Utils.ERC7579AlreadyInstalledModule(moduleTypeId, module));
        } else if (moduleTypeId == MODULE_TYPE_EXECUTOR) {
            require(_executors.add(module), ERC7579Utils.ERC7579AlreadyInstalledModule(moduleTypeId, module));
        } else if (moduleTypeId == MODULE_TYPE_FALLBACK) {
            bytes4 selector;
            (selector, initData) = _decodeFallbackData(initData);
            require(
                _fallbacks[selector] == address(0),
                ERC7579Utils.ERC7579AlreadyInstalledModule(moduleTypeId, module)
            );
            _fallbacks[selector] = module;
        }

        IERC7579Module(module).onInstall(initData);
        emit ModuleInstalled(moduleTypeId, module);
    }

Let me know what you think

[arr00]

When designing contracts, there is always a spectrum of deciding between efficiency and simplicity vs trying to ensure users don't shoot themselves in the foot. Our running assumption is that users won't be calling onInstall and onUninstall directly on modules--but if they do, that's their choice to do so.

Again, it's quite hard to draw the line, but IMO, we shouldn't make the onInstall and onUninstall flow overly complex or add another external call for this protection.

[gonzaotc]

A module should be reinstallable

Suggested change

	it('handles installation and uninstallation', async function () {
	await expect(this.mockFromAccount.onInstall(this.installData || '0x')).to.not.be.reverted;
	await expect(this.mockFromAccount.onUninstall(this.uninstallData || '0x')).to.not.be.reverted;
	});
	});
	it('handles installation, uninstallation and re-installation', async function () {
	await expect(this.mockFromAccount.onInstall(this.installData || '0x')).to.not.be.reverted;
	await expect(this.mockFromAccount.onUninstall(this.uninstallData || '0x')).to.not.be.reverted;
	await expect(this.mockFromAccount.onInstall(this.installData || '0x')).to.not.be.reverted;
	});
	});

[arr00]

Isn't successful execution implicit? Why add .to.not.be.reverted

[gonzaotc]

Yes it should behave equally, as the test would throw if any of its functions throw. However, IMO current version is more declarative and explicit about the intended behavior, I personally like it more

[arr00]

This is a little confusing to me. Couldn't we just say an account can only call onInstall once?

[gonzaotc]

IMO is not only that Module.onInstall can't be called twice, but also that it SHOULDN'T be called in other context than the standard module installation procedure (Account.installModule calling Module.onInstall) since non-standard installation (the Account calling Module.onInstall directly), has non desirable side effects (blocks posterior attempts of standard installation). I think the current warning fulfills this purpose

[ernestognw]

I agree with Aryeh that it can be simplified. Let's do:

Suggested change

	* IMPORTANT: A signer will be set for the calling account. In case the account calls this function
	* directly, the signer will be set to the provided data even if the account didn't track
	* the module's installation. Future installations will revert.
	* IMPORTANT: An account can only call onInstall once. If called directly by the account,
	* the signer will be set to the provided data. Future installations will revert.

[arr00]

When designing contracts, there is always a spectrum of deciding between efficiency and simplicity vs trying to ensure users don't shoot themselves in the foot. Our running assumption is that users won't be calling onInstall and onUninstall directly on modules--but if they do, that's their choice to do so.

Again, it's quite hard to draw the line, but IMO, we shouldn't make the onInstall and onUninstall flow overly complex or add another external call for this protection.

[arr00]

Would a social recovery module inherit from this? I assume not given the architecture enforces a 1:1 account to signer.

[ernestognw]

Would a social recovery module inherit from this? I assume not given the architecture enforces a 1:1 account to signer.

Correct, a social recovery module won't inherit from this. We agreed such module should be an executor instead of a validator but we need a generic validator and a guide before the executors.

In the future we can explore ERC7913 variants of validators if we have bandwidth

[gonzaotc]

Proposed a decomposition of ERC7579SignatureValidator in ERC7579Module and ERC7579ValidatorModule at #133