fix: enforce asset-value preservation at the callback boundary - #3443
fix: enforce asset-value preservation at the callback boundary#3443partylikeits1983 wants to merge 11 commits into
Conversation
Order the moved amounts as 200 then 100 to match the scenario described in issue #3442, and record that the executor's host-side note reconciliation already rejects this transaction today: `NOTE_BEFORE_ADD_ASSET_EVENT` is emitted before the callback runs, so the host keeps the pre-callback amounts and its output-notes commitment disagrees with the kernel's. That reconciliation is not proof-enforced, which is why the test asserts on the kernel error rather than on the commitment mismatch.
Add a negative test where on_before_asset_added_to_account rewrites the added amount so that offsetting rewrites keep the aggregate vault totals intact. On this path there is no host-side backstop at all: ACCOUNT_VAULT_BEFORE_ADD_ASSET_EVENT is emitted after the callback with the processed value, so without the callback-boundary assertion the transaction would succeed end-to-end. Also note in the asset docs that the kernel enforces the existing requirement that the processed value equals the input value.
onurinanc
left a comment
onurinanc
left a comment
There was a problem hiding this comment.
Thank you! The approach looks good to me. The kernel-side documents are updated, but it would be nice if the corresponding policy documents in miden-standards are updated as well specifically Panics if statements in invoke_{send/receive/transfer}_policy.
| pub proc on_before_asset_added_to_note | ||
| # Drop the asset ID and replace the input amount with 150. | ||
| dropw | ||
| push.150 swap drop |
There was a problem hiding this comment.
nit: drop push.150 is one cycle shorter, also applies to above one.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment inline.
| # save the original asset value for the post-callback equality check | ||
| dupw.1 loc_storew_le.CALLBACK_ASSET_VALUE_LOC dropw | ||
| # => [ASSET_ID, ASSET_VALUE, custom_data, was_foreign_context_started] |
There was a problem hiding this comment.
It would be more efficient to do something like this here (would save around 6 cycles):
swapw loc_storew_le.CALLBACK_ASSET_VALUE_LOC swapw
| # assert that the callback returned the asset value unchanged | ||
| dupw padw loc_loadw_le.CALLBACK_ASSET_VALUE_LOC | ||
| assert_eqw.err=ERR_FAUCET_CALLBACK_ASSET_VALUE_MUST_MATCH_INPUT | ||
| # => [PROCESSED_ASSET_VALUE, was_foreign_context_started] |
There was a problem hiding this comment.
As mentioned in #3442 (comment), I would remove the PROCESSED_ASSET_VALUE return parameter completely, as it's not clear to me how we would ever make it useful again without reintroducing the loophole this PR fixes.
There was a problem hiding this comment.
I don't mind changing the interface - but I'm also fine keeping it as is if changes are difficult.
4 participants
Closes #3442.
The transaction kernel now asserts that an asset callback returns the same asset value it received, failing with a dedicated error otherwise. Includes a regression test with offsetting rewrites that the epilogue alone would accept. Changes the kernel commitment.