EIP 2585: Minimal Native Meta Transaction Forwarder

[wighawag]

full EIP text here : https://github.com/wighawag/EIPs/blob/eip-2585/EIPS/eip-2585.md
Conversation here : #2585

[Amxx]

I've been doing that without a sigatureType using try catch. You may want to consider something like:

if (_isContract(message.from))
{
	try IERC1271(message.from).isValidSignature(dataToHash, _signature) returns (bytes4 value)
	{
		require(value == IERC1271(0).isValidSignature.selector, "SIGNATURE_1271_INVALID");
		return;
	}
	catch (bytes memory /*lowLevelData*/) {}
	try IERC1654(message.from).isValidSignature(keccak256(dataToHash), _signature) returns (bytes4 value)
	{
		require(value == IERC1654(0).isValidSignature.selector, "SIGNATURE_1654_INVALID");
		return;
	}
	catch (bytes memory /*lowLevelData*/) {}
	revert("NO_SUPPORTED_CONTRACT_SIGNATURE");
}
else
{
	require(message.from == SigUtil.recover(keccak256(dataToHash), signature, "SIGNATURE_WRONG_SIGNER");
}

[wighawag]

I thought of doing it that way but preferred being explicit, but I am open to change.

But actually thinking of it, the signature type should be part of the data to be signed.
The reasoning is mentioned here : #1271 (comment)

Basically if a wallet support both EIP-1654 and EIP-1271, then the signer should be protected from someone using the EIP-1654 version instead of the EIP-1271 one (which can according to the EIP-1271 standard have more strict rules like time based spending restriction, etc... (even though I am not sure how it practise it would be achieved).
So if the signature type is not part of the data, the relayer would be able to bypass the stricter 1271 one. In your implementation this would actually be hard to do since the call to EIP-1271 is done before the EIP-1654 but not impossible. Indeed, because of the EIP-150 1/64 rules there could be condition where a gas heavy 1271 call would fails due to not enough gas passed (maybe in purpose by the relayer), resulting in the call to EIP-1654 to succeed. Quite contrived I agree, but still theoretically possible

As for signature type, we could use EIP-2126 : https://ethereum-magicians.org/t/erc-2126-signature-type-recognition/3392 ?

[Amxx]

I having a hard time imagining a wallet implementing both erc1271 and erc1654 and not enforcing the same security level on both front. But you are right, almost no wallet implement either, so we are very far from seing both implementation waltking on each other's foot.

My approach was:

• if strict ERC1271 is available, we use that, and we are good (return, don't do further checks)
• if less strict ERC1654 is available, we use that, and we are good (return, don't do further checks)
• oherwize we don't know how to process the signature, so we revert

Also, I have a VERY string disagreament with ERC2126, which was never adressed despite my many comments on the ethmagician forum. ERC2126 ask you to decide between 0x4 (Wallet) and 0x2 (ERC712). So if you want to have a erc712 signature recognized by a wallet through either erc1271 or erc1654, you have no way to express that.
Knowing how to hash the data is an app issue, knowing how to check the signature of this hashed data is something different. ERC2126 is considering both as the same issue, and there is no way I'm going to support that until there is a clear distinction made.

[Amxx]

is that check really necessary ? are there cases where ecrecover returns address(0) other then the signature actually matched that of a PK that correspond the address(0) ?

[Amxx]

There is a case for sig.length == 64 (see EIP2098)

relevant code:

function _recover(bytes32 _hash, bytes memory _sign)
internal pure returns (address)
{
	bytes32 r;
	bytes32 s;
	uint8   v;

	if (_sign.length == 65) // 65bytes: (r,s,v) form
	{
		assembly
		{
			r :=         mload(add(_sign, 0x20))
			s :=         mload(add(_sign, 0x40))
			v := byte(0, mload(add(_sign, 0x60)))
		}
	}
	else if (_sign.length == 64) // 64bytes: (r,vs) form → see EIP2098
	{
		assembly
		{
			r :=                mload(add(_sign, 0x20))
			s := and(           mload(add(_sign, 0x40)), 0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff)
			v := shr(7, byte(0, mload(add(_sign, 0x40))))
		}
	}
	else
	{
		revert('invalid-signature-format');
	}

	if (v < 27) v += 27;
	require(v == 27 || v == 28, 'invalid-signature-v');
	return ecrecover(_hash, v, r, s);
}

[Amxx]

gas ?

[github-actions]

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[github-actions]

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.