AztecProtocol / aztec-packages

Apache License 2.0
172 stars 178 forks source link

Improve authwit API #7513

Open nventuro opened 1 month ago

nventuro commented 1 month ago

We have a bunch of weird looking functions, such as the following:

#[aztec(public)]
fn rotate_npk_m(address: AztecAddress, new_npk_m: Point, nonce: Field) {
    // TODO: (#6137)
    if (!address.eq(context.msg_sender())) {
        assert_current_call_valid_authwit_public(&mut context, address);
    } else {
        assert(nonce == 0, "invalid nonce");
    }

    storage.npk_m_x_registry.at(address).write(new_npk_m.x);
    storage.npk_m_y_registry.at(address).write(new_npk_m.y);
}

In the above, nonce is apparently a useless value and could be removed. However, assert_current_call_valid_authwit_public performs authentication via authwits that depend on all arguments to the function call, and which once consumed cannot be used again (i.e. replay protection). And since the context object already carries the args_hash value, we're implicitly using nonce due to it being included there.

This is all highly non-obvious, which in turn makes it very error-prone. At the very least, we should document this behavior. But ideally we'd also be able to better express this concept without having to rely to lengthy prose at each callsite.

I'm actually unconvinced nonce is even required in the example above. That'd be the case if each authwit was only usable once (e.g. by emitting a nullifier), but what the auth contract does instead is simply turn off an authorized boolean flag. We're doing approvals via state, not signatures, so the authoizer needs to explicitly re-approve the caller to perform an action - the nonce is not actually doing anything.

nventuro commented 1 month ago

After some discussion with @spalladino we tentatively concluded that indeed it doesn't seem to make much sense to have a nonce in public. The private domain works different since authwits are consumed by emitting a nullifier, so we do need some nonce in order to be able to perform the same action multiple times (with a different nonce in each).

This huge different in the private and public case seems quite unfortunate. If I had to guess, I'd bet we started introducing nonces in public because someone copy-pasted the auth code from a private fn into a public one (or made a private fn public), and then people continued copy-pasting prior samples as references for how to do things, not noticing this because it's hidden behind so many layers. This is to me a clear symtom of the API being not great.

spalladino commented 1 month ago

If nonce is indeed only needed in private, I'd like to explore whether we can remove it from the args explicitly, and just inject it via an unconstrained oracle call. After all, it's "constrained" by the authwit itself.