[Hebao] Potential issue with meta tx when txAwareHash != 0

[Brechtpd]

When txAwareHash != 0 metaTx.data is not part of the data signed by from. So I think the relayer has full control to fill in any data it wants. This can be used to either do a call with random data that simply fails, and the relayer still gets paid by from for this useless transaction. Or even worse, the relayer can call any function on to as if it is from metaTx.from even though no authorization was given for that.

I'm also not sure yet why this mechanism with txAwareHash is needed. But I'm still figuring out how some things work.

[dong77]

If txAwareHash != 0, Forwarder will leave the verification of data to each individual module. By default, each module will use SigngedReest's verify request method, which compares the real hash of the metaTx.data against txAwareHash the module.

inside SignedRequest

function verifyRequest(
        ControllerImpl               controller,
        bytes32                      domainSeperator,
        bytes32                      txAwareHash,
        GuardianUtils.SigRequirement sigRequirement,
        Request memory               request,
        bytes   memory               encodedRequest
        )
        public
    {
        require(now <= request.validUntil, "EXPIRED_SIGNED_REQUEST");

        bytes32 txHash = EIP712.hashPacked(domainSeperator, encodedRequest);

        controller.hashStore().verifyAndUpdate(txHash);

        // If txAwareHash from the mata-transaction is non-zero,
        // we must verify it matches the hash signed by the respective signers.
        require(
            txAwareHash == 0 || txAwareHash == txHash,
            "TX_INNER_HASH_MISMATCH"
        );

        require(
            txHash.verifySignatures(request.signers, request.signatures),
            "INVALID_SIGNATURES"
        );

        require(
            GuardianUtils.requireMajority(
                controller.securityStore(),
                request.wallet,
                request.signers,
                sigRequirement
            ),
            "PERMISSION_DENIED"
        );
    }

What if we do not support txAwareHash feature?

When a user wants to send an approved transfer that requires guardian signatures, he will not be able to sign the meta-transaction that wraps the approved transfer, because at the moment guardian signatures are not collected. Therefore, the operator has to send the transaction without charging the wallet owner.

With the txAwareHash support, the wallet owner can sign off the meta-tx which includes the txAwareHash, he doesn't have to wait for guardian signatures to become ready, and he will be paying the meta-transaction fees to the relayer.

[Brechtpd]

With the txAwareHash support, the wallet owner can sign off the meta-tx which includes the txAwareHash, he doesn't have to wait for guardian signatures to become ready, and he will be paying the meta-transaction fees to the relayer.

With the current system this is not safe though if I understand correctly. The user can sign a MetaTx with a txAwareHash != 0 to TransferModule to e.g. change the daily quota immediately. But the relayer can just choose whatever meta tx data he wants and e.g. call transferToken on TransferModule instead and transfer funds to wherever he wants, as if it was fully authorized by the wallet owner.

Maybe the system can work if the relayer is at least forced to call a specified function on to which is signed by from in the meta tx wnen txAwareHash != 0. But even that isn't really 100% safe because the relayer can still put in whatever data he wants, let the meta tx fail and still get paid.

[Brechtpd]

To make it safe when txAwareHash != 0 I think we could do the following:

• Let from also sign the function selector in the meta tx, and we check that this is indeed the function selector used in data.
• Don't allow the call to fail.

With the txAwareHash support, the wallet owner can sign off the meta-tx which includes the txAwareHash, he doesn't have to wait for guardian signatures to become ready, and he will be paying the meta-transaction fees to the relayer.

Looks like the functionality was removed to use the meta tx hash as replay protection so only a nonce can be used. Seems like we could run into our favourite problem here as well if it takes some time to collect all signatures: nonces and strict ordering of txs! Couldn't we just do a notification or something when all necessary data is collected for the meta tx?

[Brechtpd]

In validateMetaTx:

require(
     (to == from) ||
     (to == controller.walletFactory()) ||
     (to != address(this) && Wallet(from).hasModule(to)),
    "INVALID_DESTINATION"
);

One peculiarity with this is that e.g. a guardian smart wallet can call lock for a different wallet using a meta tx, but only if the guardian smart wallet also has that module installed, though that is not related or necessary at all because the guardian just needs to call the function for a different wallet. I'm not sure any limitations are needed at all, I think the only possible danger is if a module call is done for a wallet that hasn't actually installed the module. But in that case either the module updates data in a store or calls transact on the wallet, and both check if the wallet has the module. But the safest would be of course if we would also just add the check in the module function as well that indeed the wallet has the called module enabled.

I think all other methods to call functions from the smart wallet disable calls on modules (though not really necessary for a complete ban in most cases...) so this would be the only way for the guardian smart wallet to these module calls.

[dong77]

see https://github.com/Loopring/protocols/pull/1421/files and https://github.com/Loopring/protocols/pull/1444

[dong77]

fixed