Allowance Module: use signTypedData to sign a Transfer Authorization

[mozrt2]

I was unable to create a valid Transfer Authorization signature for the Allowance module using ethers' signTypedData EIP-712 signing function.

Both the signature produced with the EIP-712 schema described in this module's readme as well as an adjusted version (based on the correction of an error I highlighted in this pull request) return:
execution reverted: expectedDelegate == signer && delegates[address(safe)][uint48(signer)].delegate == signer

I was able to find a workaround by producing a hash that, when signed, produces a valid signature. I've attached the code at the bottom of this issue.

Letting end users sign raw hashes is however not good UX- and security-wise, hence my question regarding a way to properly sign with signTypedData (or another EIP-712 method to sign it, I'm not set on using ethers).

This is the code with signTypedData that returns incorrect signatures (I have tried a few variations that also failed):

const domainData = {
	chainId: chainId,
	verifyingContract: verifyingContract,
}

const message = {
	safe: safe,
	token: token,
	to: to,
	amount: ethers.BigNumber.from(amount),
	paymentToken: paymentToken,
	payment: ethers.BigNumber.from(payment),
	nonce: ethers.BigNumber.from(nonce),
}

const types = {
	AllowanceTransfer: [
		{ name: "safe", type: "address" },
		{ name: "token", type: "address" },
		{ name: "to", type: "address" },
		{ name: "amount", type: "uint96" },
		{ name: "paymentToken", type: "address" },
		{ name: "payment", type: "uint96" },
		{ name: "nonce", type: "uint16" },
	],
}

const sig = await signer._signTypedData(domainData, types, message)

let v = parseInt(sig.slice(130, 132), 16)
v += 4
const finalSig = sig.slice(0, 130) + v.toString(16)

return finalSig

This is the code producing the raw hash and valid signatures:

const DOMAIN_SEPARATOR_TYPEHASH = ethers.utils.keccak256(
	ethers.utils.toUtf8Bytes(
		"EIP712Domain(uint256 chainId,address verifyingContract)"
	)
)
const ALLOWANCE_TRANSFER_TYPEHASH = ethers.utils.keccak256(
	ethers.utils.toUtf8Bytes(
		"AllowanceTransfer(address safe,address token,uint96 amount,address paymentToken,uint96 payment,uint16 nonce)"
	)
)

const domainSeparator = ethers.utils.keccak256(
	ethers.utils.defaultAbiCoder.encode(
		["bytes32", "uint256", "address"],
		[DOMAIN_SEPARATOR_TYPEHASH, chainId, verifyingContract]
	)
)

const transferHash = ethers.utils.keccak256(
	ethers.utils.defaultAbiCoder.encode(
		[
			"bytes32",
			"address",
			"address",
			"address",
			"uint96",
			"address",
			"uint96",
			"uint16",
		],
		[
			ALLOWANCE_TRANSFER_TYPEHASH,
			safe,
			token,
			to,
			amount,
			paymentToken,
			paymentBig,
			nonce,
		]
	)
)

const data = ethers.utils.solidityPack(
	["bytes1", "bytes1", "bytes32", "bytes32"],
	["0x19", "0x01", domainSeparator, transferHash]
)

const keccak = ethers.utils.keccak256(data)
const finalData = ethers.utils.arrayify(keccak)

const sig = await signer.signMessage(finalData)

let v = parseInt(sig.slice(130, 132), 16)
v += 4
const finalSig = sig.slice(0, 130) + v.toString(16)

return finalSig

I've tried to find any other discussions about this but was unfortunately unsuccessful - any help / pointers would be greatly appreciated. I saw answers on related discussions from you @rmeissner, I hope it's okay to tag you here :)

[mmv08]

You don't need to add 4 to the V byte in the first snippet. V > 30 is only required for EIP-191 signatures: https://docs.safe.global/learn/safe-core/safe-core-protocol/signatures

[mozrt2]

Thanks for the feedback! I actually initially tried without changing the v byte - the signature produced is also not valid in this case unfortunately.

i.e. the code below also doesn't produce a valid signature:

const domainData = {
	chainId: chainId,
	verifyingContract: verifyingContract,
}

const message = {
	safe: safe,
	token: token,
	to: to,
	amount: ethers.BigNumber.from(amount),
	paymentToken: paymentToken,
	payment: ethers.BigNumber.from(payment),
	nonce: ethers.BigNumber.from(nonce),
}

const types = {
	AllowanceTransfer: [
		{ name: "safe", type: "address" },
		{ name: "token", type: "address" },
		{ name: "to", type: "address" },
		{ name: "amount", type: "uint96" },
		{ name: "paymentToken", type: "address" },
		{ name: "payment", type: "uint96" },
		{ name: "nonce", type: "uint16" },
	],
}

const sig = await signer._signTypedData(domainData, types, message)

return sig

[mmv08]

Have you verified that the hash generated by your code matches the hash produced by contract's generateTransferHashData function?

[mmv08]

Also, I noticed that in the second snippet, the typehash is missing the to address:

const ALLOWANCE_TRANSFER_TYPEHASH = ethers.utils.keccak256(
	ethers.utils.toUtf8Bytes(
		"AllowanceTransfer(address safe,address token,uint96 amount,address paymentToken,uint96 payment,uint16 nonce)"
	)
)

Perhaps the contract contains an incorrect type hash if the second snippet works? 🤔

Edit:
The hash of AllowanceTransfer(address safe,address token,uint96 amount,address paymentToken,uint96 payment,uint16 nonce) is indeed 80b006280932094e7cc965863eb5118dc07e5d272c6670c4a7c87299e04fceeb

[mozrt2]

Yes, I verified the raw hash produced by my code with the _signTypedData function vs the hash produced by the contract / the second code snippet I shared in my first message (which both produce the same correct hash) and wasn't able to reproduce the same hash :(

I think you are right and there may be an error in the contract. To produce the right hash you do need to pass to in the message, but to is missing in ALLOWANCE_TRANSFER_TYPEHASH (as shown in my second code snippet).

It would therefore not be possible to produce a valid signature with standard EIP-712 signing. Does that make sense?

[mozrt2]

Also doubled checked the contract code and this seems to confirm this issue: address to is missing in the Typehash but included in generateTransferHashData()

bytes32 public constant ALLOWANCE_TRANSFER_TYPEHASH = 0x80b006280932094e7cc965863eb5118dc07e5d272c6670c4a7c87299e04fceeb;
    // keccak256(
    //     "AllowanceTransfer(address safe,address token,uint96 amount,address paymentToken,uint96 payment,uint16 nonce)"
    // );

[...]

function generateTransferHashData(
        address safe,
        address token,
        address to,
        uint96 amount,
        address paymentToken,
        uint96 payment,
        uint16 nonce
    ) private view returns (bytes memory) {
        uint256 chainId = getChainId();
        bytes32 domainSeparator = keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, chainId, this));
        bytes32 transferHash = keccak256(
            abi.encode(ALLOWANCE_TRANSFER_TYPEHASH, safe, token, to, amount, paymentToken, payment, nonce)
        );
        return abi.encodePacked(byte(0x19), byte(0x01), domainSeparator, transferHash);
    }

[mmv08]

We'll discuss this internally tomorrow and devise a plan to fix the issue. It's true that right now, the contract is unusable with eip-712. Thank you for flagging this sir!

[mozrt2]

Great! Thanks for taking this up so quickly :)