nonce management issues; nit: missing comment, unusual style merits comments

[bennetyee]

kmaas/contracts/Account.sol

Line 107 in a4b924c

return

code taken from https://github.com/oasisprotocol/sapphire-paratime/blob/main/examples/onchain-signer/contracts/Gasless.sol#L23

contains the comment

// Call will invoke proxy().

which is useful. why delete it? a pointer to

kmaas/contracts/Account.sol

Line 117 in a4b924c

data: abi.encodeCall(this.proxy, data),

to indicate how it's done would be useful.

that nonce is incremented far from its use is bad from a stylistic viewpoint, since it's harder to notice if there are other uses of proxy that used a different nonce in the EthTx or if a future edit removed incrementing nonce accidentally, reviewers might not notice.

are there issues w/ re-entrancy in proxy 's addr.call possibly getting back to another invocation of proxy within the same transaction and causing the same value of nonce to be used? this is unclear, and if re-entrancy is prevented somehow there should be a comment, and if it is impossible for some other reason there should be a comment, and if signing two transactions with the same nonce is okay there should be a comment, etc.

[bennetyee]

reading this code more carefully, i think what is supposed to happen is that authorized user invokes makeProxyTx with the target contract and its abi call parameters to get a signed transaction -- signed using the key that the KMaaS contract manages -- and that concludes the top-level transaction. the user takes that signed transaction and submits it; when that executes, it invokes proxy which then in turn invokes the target contract with parameters as encoded earlier.

this means that nonce isn't incremented until the 2nd top-level transaction (the proxied transaction) commits. (and reentrancy is not possible, unless the controller authorizes the key being managed to use itself.)

a user who invokes makeProxyTx twice w/o submitting and waiting for the proxied transaction to finish will get two signed transactions that use the same nonce value. presumably this is bad, since only the first that is submitted and executed will validate okay; the second proxied transaction will be rejected -- in normal ethereum, people do this to "override" an earlier submitted that are still in the mempool, by specifying a gas price that's higher, but since makeProxyTx always signs the transaction using a fixed gasPrice of 100_000_000_000, this usage is not really possible. so not incrementing the nonce prevents generating signed transactions ahead of time without any apparent benefit.

is this intended behavior? do we want to allow users to create a proxied transaction but change their mind, so they can throw it away and create another? in this case a way for the caller to specify the gasPrice should be provided. or to create a bunch of proxied transactions and choose which one to submit later, without the latency of invoking makeProxyTx after making the decision?

comments/documentation would be nice.

[CedarMist]

Originally the pattern was used so the contract can submit transactions on behalf of a user while also encrypting the calldata (this is why we had a proxy function, as it would decrypt the data using a key only known by the contract), for the use case of gas subsidy for voting - in a view call the contract would decide if the user is allowed to vote and provide them with a transaction to submit their encrypted vote.

Since then we've implemented native calldata encryption: oasisprotocol/sapphire-paratime#449 so you can use encryptCalldata(abi.encodeCall(...)) etc. to call the contract directly with encrypted data without needing the proxy function.

But yes, there are many problems with nonce management and gas price / gas cost estimation that I haven't found a workable solution for (yet).