Cowswap orders could be replayed for smart contract accounts having the same owners

Cow01.md

Bug Description

In its current state, if smart contract accounts don't include their account address manually in the signed message digest while validating isValidSignature(), Cowswap orders could be replayed for smart contract accounts having the same owners.

Cowswap accepts EIP1271 signatures.
The signed data contains the orderDigest from Cowswap's side.

GPv2Signing

    function recoverEip1271Signer(
        bytes32 orderDigest,
        bytes calldata encodedSignature
    ) internal view returns (address owner) {
        // NOTE: Use assembly to read the verifier address from the encoded
        // signature bytes.
        // solhint-disable-next-line no-inline-assembly
        assembly {
            // owner = address(encodedSignature[0:20])
            owner := shr(96, calldataload(encodedSignature.offset))
        }

        // NOTE: Configure prettier to ignore the following line as it causes
        // a panic in the Solidity plugin.
        // prettier-ignore
        bytes calldata signature = encodedSignature[20:];

        require(
            EIP1271Verifier(owner).isValidSignature(orderDigest, signature) ==
                GPv2EIP1271.MAGICVALUE,
            "GPv2: invalid eip1271 signature"
        );
    }

The order digest is nothing but the hash of the order data and domain separator: orderDigest = order.hash(domainSeparator);

If you check the GPv2Order.Data struct, you will see that the trader or smart contract account is not included in that order data.

This exposes smart contract accounts having the same owners as follows:

Consider Alice owns Account 1 and Account 2.
She signs a payload verifying her identity as Alice on Account 1 and asks for some selling order of X into Y.
Consider Signature as SIG

recoverEip1271Signer(orderDigest = X, encodedSignature [0:20 = Account1, 20: = SIG])
Account1.isValidSignature(orderDigest, SIG)
// Signed by Alice, hence return magic_value

Attack: Replay on Account 2 using SIG

recoverEip1271Signer(orderDigest = X, encodedSignature [0:20 = Account2, 20: = SIG])
Account1.isValidSignature(orderDigest, SIG)
// Signed by Alice, hence return magic_value

UID doesnt stop this attack either, since UID considers Account 1 or Account2.
Hence, UID will be different for Account 1 and Account 2.
source

Safes are not susceptible to this attack since they themselves add the account in the signed digest (source). Hence, SIG is tightly coupled with Account 1.

function isValidSignature(bytes32 _dataHash, bytes calldata _signature) public view override returns (bytes4) {
    // Caller should be a Safe
    Safe safe = Safe(payable(msg.sender));
    bytes memory messageData = encodeMessageDataForSafe(safe, abi.encode(_dataHash));
    bytes32 messageHash = keccak256(messageData);
    if (_signature.length == 0) {
        require(safe.signedMessages(messageHash) != 0, "Hash not approved");
    } else {
        safe.checkSignatures(messageHash, messageData, _signature);
    }
    return EIP1271_MAGIC_VALUE;
}

However, there are many known smart wallets with no mannual inclusion of account during validation and thereby are exposed to the above vector.

For example:

1. Ambire: 0x2A2b85EB1054d6f0c6c2E37dA05eD3E5feA684EF

function isValidSignature(bytes32 hash, bytes calldata signature) external view returns (bytes4) {
    if (privileges[SignatureValidator.recoverAddr(hash, signature)] != bytes32(0)) {
        // bytes4(keccak256("isValidSignature(bytes32,bytes)")
        return 0x1626ba7e;
    } else {
        return 0xffffffff;
    }
}

2. Biconomy: 0x00006B7e42e01957dA540Dc6a8F7C30c4D816af5

function isValidSignature(
    bytes32 _dataHash,
    bytes memory _signature
) public view override returns (bytes4) {
    if (owner.code.length > 0) {
        return ISignatureValidator(owner).isValidSignature(
            _dataHash,
            _signature
        );
    }
    if (owner == _dataHash.recover(_signature)) {
        return EIP1271_MAGIC_VALUE;
    }
    return bytes4(0xffffffff);
}

3. Argent: source

function isValidSignature(bytes32 _msgHash, bytes memory _signature) external view returns (bytes4) {
    require(_signature.length == 65, "TM: invalid signature length");
    address signer = Utils.recoverSigner(_msgHash, _signature, 0);
    require(_isOwner(msg.sender, signer), "TM: Invalid signer");
    return ERC1271_IS_VALID_SIGNATURE;
}

4. Thirdweb
5. Soul Wallet
6. Etherspot .....

All of them expect the external protocol to include the account address inside the passed hash of isValidSignature, like UniswapX: source.

Impact : High

Loss of Assets:
Forgery of a user’s signature that would allow them to execute a funded trade without using the user’s private key
There are many smart contract wallets in the wild due to the recent rush of EIP4337, most of which have complex logic for authorization.
Take this ThirdWeb implementation as an example. They have a notion of privileged addresses.
These privileged addresses represent their bots and are common for all of their accounts. Now, imagine the signature replay of one bot account on all accounts. An order could be replayed anytime until its validity.
The same is applicable for Ambire.

Recommedation

Consider asking all account implementations to do this verification on their own OR Consider including the account inside the passed data hash by your own.

Please note that
This is applicable for combination of EOA + Smart Contract Wallet as well.
For example, If Alice has a EOA 0xa and is operating a smart wallet Acc1 from 0xa, the signatures of 0xa, could be replayed on Acc1 as well.