Bugfix allow account code change #784

hackaugusto · 2024-07-02T20:51:32Z

The account delta check only allows the nonce to change if the storage or the vault changes. This is a problem, because it forbids changes to the account's code.

This relaxes the test, to fix the immediate issue, allowing the account code to change. In the future, however, the correct fix is to also track code changes in the account delta and include that in the nonce check.

hackaugusto · 2024-07-02T20:51:49Z

miden-lib/asm/miden/account.masm

@@ -86,22 +86,19 @@ end

 #! Sets an item in the account storage. Panics if the index is out of bounds.
 #!
-#! Stack: [index, V']
+#! Stack: [index, V', 0, 0, 0]


this is because of #783

As I mentioned in the comment for #783, I'm not sure I understood the motivation for this change. I think it was working as intended (e.g., the user would be able to do exec.account::set_item and it would not insert any extra zeros into the stack. Or am I missing something?

hackaugusto · 2024-07-02T20:53:17Z

miden-lib/src/transaction/errors.rs

@@ -98,7 +98,7 @@ impl fmt::Display for TransactionKernelError {
                )
            },
            TransactionKernelError::UnknownAccountProcedure(proc_root) => {
-                write!(f, "account procedure with root {proc_root} is not in the advice provider")
+                write!(f, "Could not find account procedure with digest {proc_root} is in the advice provider, check the transactinon's account code")


this was confusing, it was unclear if it was the account procs hash, or the mast root of the procedure.

hackaugusto · 2024-07-02T20:55:40Z

miden-tx/src/testing/tx_context.rs

+        inputs: Vec<(Word, Vec<Felt>)>,
+        targets: Vec<ScriptTarget>,
+    },
+}


This is a bit annoying, the tx script doesn't allow phantom calls, so the script must be compiled with an assembler which had the account loaded into it.

As the tests are currently setup, the assembler is kinda "hidden" inside the context. This structure is just to allow the test to pass script code that calls the account, and to have it compiled with the same assembler. Which then finally allows the tests to run against main.masm instead of stand alone scripts, which is leading me to find issues like the one this PR is fixing.

hackaugusto · 2024-07-02T20:56:47Z

miden-tx/src/testing/tx_context.rs

-
-    pub fn set_tx_args(&mut self, tx_args: TransactionArgs) {
-        self.tx_args = tx_args;
-    }


The args are no longer created when instantiating the context, now it is created just before executing the transaction, this too is because of the tx script (because one needs the tx script object to create the tx arg one).

hackaugusto · 2024-07-02T20:57:54Z

miden-tx/src/tests/kernel_tests/mod.rs

@@ -29,77 +29,56 @@ pub fn read_root_mem_value<H: Host>(process: &Process<H>, addr: u32) -> Word {
    process.get_mem_value(ContextId::root(), addr).unwrap()
 }

+/// Given an slice of [Note]s, generates a procudere to initialize the VM's memory with the note's
+/// data.
 pub fn output_notes_data_procedure(notes: &[Note]) -> String {


This should allow one to simplify the context builder, and remove notes which are no longer needed, since the number of notes now can vary.

In the future it should also support notes with a variable number of assets.

hackaugusto · 2024-07-02T21:00:29Z

objects/src/accounts/delta/mod.rs

-    } else if nonce.is_some() {
-        return Err(AccountDeltaError::InconsistentNonceUpdate(
-            "nonce updated for empty delta".to_string(),
-        ));


This is the temporary bug fix.

I think this check specifically has been discussed in the context of one of the pioneer's projects and a possibility was to remove it altogether (#765)

even better :)

hackaugusto · 2024-07-02T21:03:47Z

objects/src/accounts/code.rs

+        let new_acct_code_ast =
+            ModuleAst::parse(source).map_err(AccountError::AccountCodeParsingError)?;
+        AccountCode::new(new_acct_code_ast, &Assembler::default())
+    }


Just a convenience constructor. We never construct code directly from the AST, always from a string builder or str slice.

hackaugusto · 2024-07-02T21:04:45Z

objects/src/testing/account_code.rs


        # acct proc 2
        export.incr_nonce
-            push.0 swap


This is done by account::incr_nonce already.

bobbinth · 2024-07-03T09:55:43Z

miden-lib/asm/miden/contracts/wallets/basic.masm

+#! Increments the account nonce by the provided value.
+#!
+#! Stack: [value]
+#! Output: []
+#!
+#! - value is the value to increment the nonce by. value can be at most 2^32 - 1 otherwise this
+#!   procedure panics.
+export.incr_nonce
+    exec.account::incr_nonce
+    # => []
+end


Adding this procedure would lead to a security vulnerability: it will allow anyone to bypass an account's authentication mechanism. What is the motivation for adding it?

The authentication mechanism is done in the kernel:

miden-base/miden-lib/asm/kernels/transaction/api.masm

Lines 131 to 132 in 89437d1

# authenticate that the procedure invocation originates from the account context

exec.authenticate_account_origin

This is not skipping it (just like the other methods in this file)

I mean the transaction authentication (e.g., where we require the user to provide a signature against an account's public key). Exposing this method publicly would allow anyone to bypass signature verification and update the account state at will.

Ah, so the plan is to have the signature verification code calling the nonce increment? That makes sense, I was unaware of that flow.

Moved it back to the test only.

bobbinth · 2024-07-03T10:03:38Z

miden-lib/asm/miden/contracts/wallets/basic.masm

+#! Sets the code of the account the transaction is being executed against. This procedure can only
+#! executed on regular accounts with updatable code. Otherwise, this procedure fails.
+#!
+#! Stack: [CODE_ROOT]
+#! Output: []
+#!
+#! - CODE_ROOT is the hash of the code to set.
+export.set_code
+    exec.account::set_code
+    # => []
+end


Ability to update account code is not yet supported. Making it work properly is a relatively big task and I'm not sure we need to tackle it yet.

In either case though, adding it to the basic wallet interface is probably not the right approach (i.e., when we do make it work, it should probably be a part of a separate interface).

Moved it back to the test only.

miden-lib/asm/miden/account.masm

bobbinth · 2024-07-05T09:24:04Z

@hackaugusto - could you rebase this from the latest next?

This is needed to have the account's procedures available, because the tx_script is compiled without fantom calls.

Changes to the account code were forbidden in the Rust code, because the account delta nonce validation assumed the nonce would only change with changes to the vault or storage. Currently the account delta doesn't track changes to the account code, so as an intermediary solution this changes the check to allow code changes anytime. The correct solution would be to introduce the code change to the account delta too, and introduce that as another case in which the delta must change, and then forbid changes otherwise.

hackaugusto · 2024-07-05T12:28:11Z

@hackaugusto - could you rebase this from the latest next?

done

hackaugusto requested review from bobbinth and polydez July 2, 2024 20:51

hackaugusto commented Jul 2, 2024

View reviewed changes

hackaugusto changed the title ~~Hacka bugfix allow account code change~~ Bugfix allow account code change Jul 2, 2024

hackaugusto force-pushed the hacka-bugfix-allow-account-code-change branch from 4dbbb6b to 8f6df50 Compare July 2, 2024 23:35

bobbinth reviewed Jul 3, 2024

View reviewed changes

miden-lib/asm/miden/account.masm Outdated Show resolved Hide resolved

hackaugusto force-pushed the hacka-tests-cleanup-2 branch from 273d0c0 to 04e42d2 Compare July 3, 2024 14:10

Base automatically changed from hacka-tests-cleanup-2 to next July 3, 2024 14:26

hackaugusto force-pushed the hacka-bugfix-allow-account-code-change branch 2 times, most recently from a704e72 to 15b07a3 Compare July 3, 2024 16:14

bobbinth force-pushed the next branch from 9adfc85 to 1cd939e Compare July 4, 2024 06:31

hackaugusto added 9 commits July 5, 2024 14:23

tests: comment test transaction_executor_witness

7cc1c3d

tests: renamed and commented execute_with_custom_main

0ed5d1c

tests: added execute_transaction method to TransactionContext

28e7727

account_code: added constructor from str slice

686844c

chore: updated comment format

34bd095

tests: args used only in build()

ce2dd9b

tests: passing explicit tx args from tests

78f3178

tests: removed length assumption from output_notes_data_procedure

58e3db8

tests: compiling script with the executor's compiler

c017a68

This is needed to have the account's procedures available, because the tx_script is compiled without fantom calls.

hackaugusto added 4 commits July 5, 2024 14:23

bugfix: added maybe async

405e985

tests: simplified executed_transaction_output_notes

ec85593

bugfix: incr_nonce and set_code should not be part of the public API

dcdc4c9

hackaugusto force-pushed the hacka-bugfix-allow-account-code-change branch from 5f9641a to dcdc4c9 Compare July 5, 2024 12:23

hackaugusto closed this Jul 17, 2024

bobbinth deleted the hacka-bugfix-allow-account-code-change branch November 6, 2024 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix allow account code change #784

Bugfix allow account code change #784

hackaugusto commented Jul 2, 2024 •

edited

Loading

hackaugusto Jul 2, 2024

bobbinth Jul 3, 2024

hackaugusto Jul 2, 2024

hackaugusto Jul 2, 2024

hackaugusto Jul 2, 2024 •

edited

Loading

hackaugusto Jul 2, 2024

hackaugusto Jul 2, 2024

igamigo Jul 2, 2024 •

edited

Loading

hackaugusto Jul 2, 2024

hackaugusto Jul 2, 2024

hackaugusto Jul 2, 2024

bobbinth Jul 3, 2024

hackaugusto Jul 3, 2024

bobbinth Jul 3, 2024

hackaugusto Jul 3, 2024 •

edited

Loading

hackaugusto Jul 3, 2024

bobbinth Jul 3, 2024

hackaugusto Jul 3, 2024

bobbinth commented Jul 5, 2024 •

edited

Loading

hackaugusto commented Jul 5, 2024

	# authenticate that the procedure invocation originates from the account context
	exec.authenticate_account_origin

Bugfix allow account code change #784

Bugfix allow account code change #784

Conversation

hackaugusto commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Jul 5, 2024 • edited Loading

hackaugusto commented Jul 5, 2024

hackaugusto commented Jul 2, 2024 •

edited

Loading

hackaugusto Jul 2, 2024 •

edited

Loading

igamigo Jul 2, 2024 •

edited

Loading

hackaugusto Jul 3, 2024 •

edited

Loading

bobbinth commented Jul 5, 2024 •

edited

Loading