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

vp_user: consider being less permissive on unknown keys #408

Closed
tzemanovic opened this issue Aug 31, 2022 · 2 comments · Fixed by #2213
Closed

vp_user: consider being less permissive on unknown keys #408

tzemanovic opened this issue Aug 31, 2022 · 2 comments · Fixed by #2213

Comments

@tzemanovic
Copy link
Member

in VPs implementation, we decided to be permissive on changed storage keys that are unknown or irrelevant to a validity predicate that is being implemented.

In case of e.g. vp_token I think this makes a lot of sense, it shouldn't need to know or check what happens in PoS (a bond is just a transfer from source to PoS from its perspetive). I think in vp_user this is could be dangerous though, as this VP is used for signature authorization for certain actions (for e.g. receiving transparent tokens, it doesn't require signature from the receiver, but naturally it does for spending tokens), so the vp_user is coded to understand certain storage keys of other accounts.

Perhaps if the vp_user cannot make a sense of the storage change from another account, the safer default for it would be to still require a valid signature. Otherwise, one might have a transaction mistakenly thinking that's been authorized by a signature of a key associated with the account by vp_user, while instead the storage change(s) might just be unknown to it.

In vp_user, it's this case:

            KeyType::Unknown => {
                if key.segments.get(0) == Some(&addr.to_db_key()) {
                    // Unknown changes to this address space require a valid
                    // signature
                    *valid_sig
                } else {
                    // Unknown changes anywhere else are permitted
                    true
                }
            }

we'd change to:

            KeyType::Unknown => {
                // Unknown changes require a valid signature
                *valid_sig
            }
@cwgoes
Copy link
Contributor

cwgoes commented Jan 13, 2023

Agree, we should be very defensive here.

@tzemanovic
Copy link
Member Author

I'll add this a milestone as it's important

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants