Closed just-mitch closed 7 months ago
Personally, I'm leaning towards the second option as it seems cleaner and safer. The noinitargscheck
macro looks like it could lead to a bunch of bugs.
One problem I see is that the fee is designed to go at the beginning of the execution, marking itself as 'non-revertibe'. If we follow the "init, then pay the fee" flow then the initialisation becomes part of the non-revertible part of a tx. This becomes a problem if an account contract in the future requires public initialisation as that would be tagged for the setup phase, meaning it either has to be whitelisted by the sequencer or the sequencer takes on risk that init would take significant compute and eventually revert.
Ideally we'd setup the fee first inside AccountInitializer
, then run the account contract's init function as part of private app logic, but how do we validate the authwitness? We don't have an account contract to validate authwits.
I'll come back here once I have a solution
Ideally we'd setup the fee first inside AccountInitializer, then run the account contract's init function as part of private app logic, but how do we validate the authwitness? We don't have an account contract to validate authwits.
I'm afraid that if you go down this route you end up in a situation where you need a method in the account contract to validate a fee payload before initialization, which means you manually need to reconstruct the init args from the address preimage, which means you end up implementing something very similar to the first option.
I'd suggest restricting account contracts to only have private constructors if they are meant to be deployed this way. And if it needs a public constructor, then have it initialized from another account who pays the fee.
Coming back with an update after thinking through this a bit more and chatting with @spalladino on slack. We propose two flows.
This would be a new protocol contract that would be responsible for initialising individual account contracts. Like in the comment above:
contract AccountInitializer:
entrypoint(target_address, init_args, fee_payload):
private_call(target_address, "constructor", init_args)
private_call(target_address, "entrypoint", app_payload: empty, fee_payload)
The AccountInitializer
first initialises the target account contract, then it calls into its entrypoint to setup fee payment as normally.
Pros:
Cons:
endNonRevertible
)The initialiser would run as part of the private setup phase meaning whatever nullifiers/notes it emits become part of the non-revertible set. I don't immediately see an issue with this, a fee still gets paid for the data.
A malicious user could craft an account contract that enqueues public functions to be executed as part of the public setup phase. In this case the sequencer protects itself by whitelisting which contract instances/classes are allowed to run in setup and would reject the unrecongised account contract.
We can take Palla's initial suggestion, but instead of adding the macro aztec(noinitargscheck)
, we pass the FeePayload
as a capsule.
contract SomeAccountContract:
constructor(pub_key: PublicKey):
# get the fee and check authwit
fee_payload = FeePayload::deserialize(pop_capsule)
assert(is_valid(context, fee_payload.hash(), pub_key))
# execute fee, capture hwm
fee_payload.execute_calls(context)
context.capture_min_revertible_side_effect_counter()
# init storage in app logic
storage.pub_key.set(pub_key)
Pros:
Cons:
AccountActions
shared struct)Same security implications apply. If a malicious account contract tries to inject code into public setup, it would get rejected by the sequencer through the fact that it's not on the approved fee payment contracts.
PS: I'm starting to think that maybe the naming we chose for revertible/non-revertible side effects might not have been the best. If an action in either phase reverts then the tx "fails" but in one case it soft-reverts and goes on chain and pays a partial fee while in the other it hard-reverts and is rejected by the sequencer.
Thanks for the writeup! Just one more comment from me:
If a malicious account contract tries to inject code into public setup, it would get rejected by the sequencer through the fact that it's not on the approved fee payment contracts.
I don't think this is a security issue inherent of this task. Anyone can deploy a contract that behaves like this, regardless of what we do with account initialization.
After a bit of back and forth on capsules we've agreed to go the account contract initialiser route with one difference. Instead of deploying a new protocol contract whose sole responsibility is to intialise account contracts, we instead are using the canonical multi call entrypoint to do it.
The multi call contract will receive a payload with the following function calls:
In order to achieve this we have modified the entrypoint interface to accept transient authwits alongside function calls so that the yet-to-be-deployed-account can create an authwit authorizing the fee payload.
Today account contracts are initialized by calling into their initializer method directly, meaning they don't go through a traditional
entrypoint
that can handle fee payment. So account contract initialization does not pay fees at all today. To fix this, we have two options:Add fee payment to initializer. Along with the initialization arguments for the account contract (eg the authorization ECDSA public key), we can supply a
FeePaymentPayload
with instructions on how to pay for the initialization fee. This has a problem though: adding aFeePaymentPayload
to the initializer means it becomes part of the initialization hash that needs to be committed to when the contract address is precalculated. We probably don't want this, since we want flexibility in deciding how (and specially, how much) the fee will be paid, as it depends on the moment in which the tx is sent. Doing this requires adding a new macroaztec(noinitargscheck)
that disables the init args check, and instead manually callsassert_initialization_args_match_address_preimage
with the arguments we want to constrain. TheFeePaymentPayload
should be checked via an authwit, as usual, to prevent someone else from initializing the contract and paying a ridiculous amount of money without the owner's authorization.Add a canonical initializer contract. This contract does not need to be enshrined, but it should be well-known. This contract should provide a common entrypoint that takes care of initialization and fee payment in the account contract. The logic should be something along the lines of:
Note that this approach, while a lot cleaner since it does not need the dangerous
noinitargscheck
, will not work unless we can read a nullifier emitted earlier in the same tx (which is not yet supported), since theentrypoint
should have an init check to prevent it from being called before initialization. Alternatively, we could flag theentrypoint
asnoinitcheck
, and "manually" assert that the account contract has been initialized based on whether we can read the initialized data (eg the ECDSA pub key).