Add ERC-7579 account extension #46
Conversation
| import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; | ||
| import {AccountCore} from "../AccountCore.sol"; | ||
|
|
||
| abstract contract AccountERC7579 is AccountCore, IERC7579Execution, IERC7579AccountConfig, IERC7579ModuleConfig { |
There was a problem hiding this comment.
We should add ERC7739Signer here since ERC-7579 requires ERC-1271. Instead of using a signer I'd implement _rawSignatureValidation as follows:
function _rawSignatureValidation(
bytes32 hash,
bytes calldata signature
) internal view virtual override returns (bool) {
address module = abi.decode(signature[0:20], (address));
if (!_isModuleInstalled(MODULE_TYPE_VALIDATOR, module, msg.data)) return false;
return IERC7579Validator(module).isValidSignatureWithSender(msg.sender, hash, signature) == bytes4(0x77390001);
}|
|
||
| function _fallback() internal virtual { | ||
| address handler = _fallbackHandler(msg.sig); | ||
| if (handler == address(0)) revert ERC7579MissingFallbackHandler(msg.sig); |
There was a problem hiding this comment.
Why wouldn't just return here? I don't find the specification to be clear about this but I'd say it's better to treat it as a noop and return right away.
|
This is ready for review but needs resolution from #54, docs can be added in a different PR |
| * contract MyAccountERC7579 is AccountERC7579, Initializable { | ||
| * constructor() EIP712("MyAccountRSA", "1") {} | ||
| * | ||
| * function initializeAccount(address validator, bytes calldata validatorData) public initializer { |
There was a problem hiding this comment.
It's worth noting that this is vulnerable to frontrunning if used as a 7702 account.
I wonder if it might be worth defining a special-purpose accountInitializer modifier that is "safe for accounts" whether used as 7702 templates or standalone contracts.
| * This function delegates the signature validation to a validation module if the first 20 bytes of the | ||
| * signature correspond to an installed validator module. |
There was a problem hiding this comment.
I've asked before but is this standard? Or is it at least conventional across existing projects?
There was a problem hiding this comment.
ERC-7579 explicitly states that "This standard does not dictate how validator selection is implemented." It seems wrong to implement a specific selection mechanism that is not standardized.
It might be just as or more common to have a single validator module installed and to use an unprefixed signature.
| (moduleTypeId != MODULE_TYPE_VALIDATOR || _validators.add(module)) && | ||
| (moduleTypeId != MODULE_TYPE_EXECUTOR || _executors.add(module)), |
There was a problem hiding this comment.
Side effects in a require condition is not a good pattern IMO, I would refactor this.
Based on OpenZeppelin/openzeppelin-contracts#5250