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

Opt In Overwrite #13

Merged
merged 4 commits into from
Feb 6, 2024
Merged

Opt In Overwrite #13

merged 4 commits into from
Feb 6, 2024

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Feb 1, 2024

To protect users from overwriting their key, we have a boolean-flag (overwrite) which must be set to true to overwrite values.

Closes #5.

Adjusted Rust Tests.

@bh2smith bh2smith requested a review from tifrel February 1, 2024 10:33
#[near_bindgen]
impl EthKeys {
// Sets the encrypted key for the sender's account.
pub fn set_key(&mut self, encrypted_key: String) {
pub fn set_key(&mut self, encrypted_key: String, overwrite: bool) {
// TODO - would be nice if there was some way to validate
// that the encrypted key actualy contains expected data.
let account_id = env::signer_account_id();
Copy link
Member

@tifrel tifrel Feb 6, 2024

Choose a reason for hiding this comment

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

There are nuances around signer_account_id and predecessor_account_id, I would have to verify, but this way a rogue smart contract might be able to set a new key for a user. It's also common practice to require a yoctoNEAR deposit, which can only be done using FullAccessKeys, but not with FunctionCallKeys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very interesting. Thanks. I was under the impression that the yoco deposit would be a separate change that would also have to make the function payable. I had captured this in issue #2. However, your comment seems to mean two different things here (something about how other accounts can overwrite other's keys)... Are these two different things, or does having attached deposit somehow prevent this?

Copy link
Member

Choose a reason for hiding this comment

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

#2 covers everything, except the signer/predecessor. Most of this comment is just information about NEAR smart contracts and common practices.

Copy link
Contributor Author

@bh2smith bh2smith Feb 6, 2024

Choose a reason for hiding this comment

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

Sounds a bit like the analogue of sender vs something else when contracts make delegate calls to others. So then would you say it's "approved"?

Copy link
Member

Choose a reason for hiding this comment

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

Left a comment on #2, I think it's better addressed there.

@bh2smith bh2smith requested a review from tifrel February 6, 2024 12:53
@bh2smith bh2smith merged commit f91de1e into main Feb 6, 2024
2 checks passed
@bh2smith bh2smith deleted the opt-in-overwrite branch February 6, 2024 14:06
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.

[Contract] Overwrite Protection
2 participants