Offset faucet procedures by 1 to enable correct authentication and reserved slot access

[phklive]

Feature description

Since this PR: #846 we have changed the reserved storage slot location of faucet accounts to 0. Hence breaking the standard that we had up to now for authentication storage slot. Where the pub_key of the account would be stored at location 0.

Goal of this issue:

Add a boolean flag to AccountCode checking if the account is a faucet or not and if it is offset all account procedures by 1 (added to the actual offset of the account procedures).

This will enable to keep the authentication at storage location 0 (offset) and reserved slot at storage location 0 (virtually).

Why is this feature needed?

Faucets now use storage slot 0 as their reserved slot.

We need to offset to the correct location in storage for authentication.

[phklive]

@bobbinth, thinking more about it I am not entirely sure I understood how this would solve our issue regarding faucets.

If I offset all procedures by 1 I won't be able to access location 0 of the storage anymore.

Furthermore we do not have the ability to add real offsets to procedures for now hence all faucets would just get their storage offset by 1. I am not sure how shifting the faucet storage by 1 would solve our issue here.

[bobbinth]

If I offset all procedures by 1 I won't be able to access location 0 of the storage anymore.

I think that's what we actually want: we want to make sure that user procedures cannot access slot 0 because this slot is reserved for internal faucet usage (we should still be able to access this slot from within the kernel because offsets are applied in the api.masm file).

Furthermore we do not have the ability to add real offsets to procedures for now hence all faucets would just get their storage offset by 1. I am not sure how shifting the faucet storage by 1 would solve our issue here.

There are two things we need to do:

1. In Rust, when account code is cerated, if the account is a faucet, we should add 1 to all procedure offsets. This is a temporary solution which we'll change once we have annotations in the assembler.
2. In MASM, when we process account data in the prologue for faucet procedures accounts, we should make sure that no procedure has offset equal to 0.

For point 2, we probably need to create a separate procedure which will also solve #863.

[phklive]

Hello @bobbinth,

I have started working on this issue and I have a few questions:

• Why do we need a temporary solution here? Does our current impl not serve us well? What would this patch improve? (except re-setting the pub_key at slots[0])
• This issue can go against our previous goal of going away from doing direct kernel procedure invocations in tests: fix: Remove caller ID workaround for root contexts #857. Because as you mentioned here:

I think that's what we actually want: we want to make sure that user procedures cannot access slot 0 because this slot is reserved for internal faucet usage (we should still be able to access this slot from within the kernel because offsets are applied in the api.masm file).

We could now only access the faucet reserved storage slot from inside the kernel. But for example in tests we are asserting the correct update of this storage slot:

here:

miden-base/miden-tx/src/tests/kernel_tests/test_faucet.rs

Line 35 in f4fce94

fn test_mint_fungible_asset_succeeds() {

here:

miden-base/miden-tx/src/tests/kernel_tests/test_faucet.rs

Line 296 in f4fce94

fn test_burn_fungible_asset_succeeds() {

Hence these tests are now failing and I do not see a simple solution to be able to refactor them other than doing in kernel procedure invocations again.

            exec.::kernel::account::get_item

This would enable me to bypass the storage_offset application, but we would end up calling in kernel procedures again. Maybe if it's isolated (only in a few tests) it can be alright.

Let me know your thoughts, thank you.

[bobbinth]

Why do we need a temporary solution here? Does our current impl not serve us well? What would this patch improve? (except re-setting the pub_key at slots[0])

The current solution incorrectly allows user procedures to access storage slots which are reserved by the faucet. The temporary solution is only for the Rust side. On the MASM side, once this issue is addressed, we won't have this vulnerability.

This issue can go against our previous goal of going away from doing direct kernel procedure invocations in tests: fix: Remove caller ID workaround for root contexts #857.

Yeah, we don't want to go back to doing that. You can probably re-work the tests to read the reserved slot directly from process memory (i.e., don't try to access it via MASM but directly via the Process struct). But I haven't thought it through completely to know if that'll work for sure.

[igamigo]

Related to this issue, it seems that the public key slot on basic.masm changed to 3 but create_basic_wallet() still sets it to slot 0.

I think this is causing some issues when running integration tests from the client against next. I added dummy values to account for this offset and it seems those errors were mitigated (cc @polydez). Wondering if we should merge a temporary fix until this is addressed or if maybe we can solve it in a different way.

[phklive]

Related to this issue, it seems that the public key slot on basic.masm changed to 3 but create_basic_wallet() still sets it to slot 0.

I think this is causing some issues when running integration tests from the client against next. I added dummy values to account for this offset and it seems those errors were mitigated (cc @polydez). Wondering if we should merge a temporary fix until this is addressed or if maybe we can solve it in a different way.

It should be fixed by this PR: #875