Loopring / protocols

A zkRollup DEX & Payment Protocol
https://loopring.org
329 stars 122 forks source link

[hebao 1.2] Meta transaction does not consume quota for successful inner txAware TX #1995

Closed dong77 closed 3 years ago

dong77 commented 3 years ago

In ForwardModule, we have the following:

            // Do not consume quota when call factory's createWallet function or
            // when a successful meta-tx's txAwareHash is non-zero (which means it will
            // be signed by at least a guardian). Therefor, even if the owner's
            // private key is leaked, the hacker won't be able to deplete ether/tokens
            // as high meta-tx fees.
            bool skipQuota = success && (
                metaTx.txAwareHash != 0 || (
                    data.toBytes4(0) == WalletFactory.createWallet.selector ||
                    data.toBytes4(0) == WalletFactory.createWallet2.selector) &&
                metaTx.to == walletFactory
            );

The comment doesn't ring a bell, @Brecht, we skip quota if success && metaTx.txAwareHash != 0? Do you recall why?

Brechtpd commented 3 years ago

Not sure. Maybe because these txAwareHash functions always needed majority internally anyway so updating the quota is unnecessary? Not sure why skipQuota was really added in general.

dong77 commented 3 years ago

I recall: this is to prevent low quota causing security-related multisig operators to fail, such as unlock/recovery...

Brechtpd commented 3 years ago

So I guess this was true, but with operations like lockWA in the new version it seems we can't do this any more because only the owner signature is needed for the operation, so only the owner can use all funds in the wallet regardless of quota.

Brechtpd commented 3 years ago

It wasn't even true before because it's possible to create a txAwareHash meta tx to e.g. a view function that would always be successful (txAwareHashNotAllowed is only added for functions that actually do stuff). It needs to be removed.