Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet2: when checking frozen multisig tx set, don't assume order #8950

Merged

Conversation

jeffro256
Copy link
Contributor

Resolves #8949

@0xFFFC0000
Copy link
Collaborator

Thanks jeffro256.

This PR solves this issue mentioned. I have a suggestion. Wouldn't it be better to check the vector?

For example, pseudocode would be:

exist i in 0 < max(vin) 
    sources[src_idx].multisig_kLRki.ki == vin[i].k_image
otherwise
    assert false;

This way, we keep the assert, and do not fail when there are legitimate order mismatches.

@woodser
Copy link
Contributor

woodser commented Jul 17, 2023

@jeffro256 Can you also open a PR against the release branch? Thanks.

@jeffro256
Copy link
Contributor Author

Thanks jeffro256.

This PR solves this issue mentioned. I have a suggestion. Wouldn't it be better to check the vector?

For example, pseudocode would be:

exist i in 0 < max(vin) 
    sources[src_idx].multisig_kLRki.ki == vin[i].k_image
otherwise
    assert false;

This way, we keep the assert, and do not fail when there are legitimate order mismatches.

This method is only checking if the key images specified in a multisig_tx_set are frozen. I added in the assertion initially because I thought the key images were ordered, and I figured "why not" keep the assertions conservative so I didn't have to insert twice. However, since insert is idempotent in this case, I cover the case that the lists are mismatched and a counterparty tries to bypass the restriction. At any rate, if mismatched key image key lists cause an actually issue with signing, that's an entire other issue that needs to be addressed in another PR.

@jeffro256
Copy link
Contributor Author

@jeffro256 Can you also open a PR against the release branch? Thanks.

To confirm, does this PR fix the issue your tests were having? It should, but I want to know

@woodser
Copy link
Contributor

woodser commented Jul 17, 2023

To confirm, does this PR fix the issue your tests were having? It should, but I want to know

I'm getting merge conflicts rebasing on the release branch to test.

@woodser
Copy link
Contributor

woodser commented Jul 17, 2023

I applied the changes manually and they fix the problem as far as I can tell.

@luigi1111 luigi1111 merged commit 1ab5939 into monero-project:master Aug 17, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wallet2 frozen incorrectly asserts key image ordering and causes error on multisig tx signing
5 participants