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

token-2022: Zeroize account data on close #2764

Merged
merged 2 commits into from Jan 21, 2022

Conversation

joncinque
Copy link
Contributor

This the token-2022 version of #2763

Problem

Closing accounts doesn't null out all the data.

Solution

Zero out all data on account close.

Differences with the non-2022 version:

  • Reuse check_program_account instead of the other owner check
  • cmp_pubkeys refactored into lib.rs
  • sol_memset on close sets the entire data.len() to 0, and not Account:::LEN

@joncinque joncinque requested a review from mvines January 20, 2022 23:44

/// Checks two pubkeys for equality in a computationally cheap way using
/// `sol_memcmp`
pub fn cmp_pubkeys(a: &Pubkey, b: &Pubkey) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could manually implement PartialEq for Pubkey to make this optimization available to all Rust programs automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. I want to wait until the memcmp costs are sorted out and be sure that it's cheaper before making it the default behavior.

@@ -297,6 +298,11 @@ impl Processor {
)?,
};

if self_transfer || amount == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt about unconditionally checking as belts and suspenders? Slightly more compute for the false case seems to be the only downside

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how sensitive we want to be about raising compute costs. I'm a bit worried that the extensions will make things more expensive, and these owner checks are already quite pedantic.

How about this: we remove the checks now, and when we're ready to release, we revisit compute costs and see if the savings are worth it at that point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deal!

@@ -574,6 +580,11 @@ impl Processor {
COption::None => return Err(TokenError::FixedSupply.into()),
}

if amount == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, unconditionally check?

@@ -658,6 +669,11 @@ impl Processor {
)?,
}

if amount == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

....aaaand here?

mvines
mvines previously approved these changes Jan 21, 2022
@mergify mergify bot dismissed mvines’s stale review January 21, 2022 01:23

Pull request has been modified.

@joncinque joncinque merged commit f5a6dc6 into solana-labs:master Jan 21, 2022
@joncinque joncinque deleted the tk22-zeroize branch January 21, 2022 01:47
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.

None yet

2 participants